Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer initialization of addons until after campaign load #5231

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fishface60
Copy link
Contributor

@fishface60 fishface60 commented Feb 12, 2025

Identify the Bug or Feature request

Fixes #5178

Description of the Change

This changes the register/reregister functions to conditionally initialize and adds a method to the Library Manager to initialize all addons, so that when an addon is added to a running campaign it is initialized immediately, but when initializing from connecting to a server or loading from a file addon initialization is deferred until after the campaign is loaded.

Possible Drawbacks

There may be a cleaner way to handle this than changes all over the place.

Release Notes

  • Deferred running Add-on onInit/onFirstInit scripts until after the campaign is loaded so that their changes aren't trampled during campaign load and they can depend on campaign state.

This change is Reviewable

@github-actions github-actions bot added the bug label Feb 12, 2025
@cwisniew
Copy link
Member

A better approach is to create a list of "pending" AddOns; if the campaign hasn't been loaded yet, add the registered library to the pending list; otherwise, register it. Once the campaign has been loaded, process the pending add-ons from the list and register them.

This means that we wouldn't end up with partially processed add-ons (like this change would introduce), and any new things added to add-ons would automatically be fixed if they needed to wait for the campaign to be loaded. Also, people modifying the code later would not need to figure out whether they must pass true or false to the register library function.

Of course, some locking would need to occur for the variable campaign loaded/list (well, I guess you can use a ConcurrentLinkedQueue for the list and not lock that). You would also need two new methods in the LibraryManager, one to set the Campaign as not loaded (called first thing in the MapTool.setCampaign()) and one to set it as loaded (called last thing in MapTool.setCampaign()

@fishface60
Copy link
Contributor Author

CampaignManager.clearCampaignData() looks like a good place to flag that add-on loading should be deferred.

Having dug a bit further, I don't think inside MapTool.setCampaign itself is the best place to initialize deferred add-ons, since it's sometimes used to set temporary campaigns and is sometimes used to update the map, but we only want to initialize the add-ons when the campaign is finally loaded and not re-initialize if we don't need to.

There's currently code to handle campaign macro init in MapTool.setCampaign but that doesn't matter for temporary campaigns because they don't have any and setting the new campaign will load new campaign macro init.
I don't think add-ons get this luxury, and I'm not sure if the existing code is calling onCampaignLoad at times I wasn't expecting.

This is hard.

@cwisniew
Copy link
Member

CampaignManager.clearCampaignData() looks like a good place to flag that add-on loading should be deferred.

Having dug a bit further, I don't think inside MapTool.setCampaign itself is the best place to initialize deferred add-ons, since it's sometimes used to set temporary campaigns and is sometimes used to update the map, but we only want to initialize the add-ons when the campaign is finally loaded and not re-initialize if we don't need to.

There's currently code to handle campaign macro init in MapTool.setCampaign but that doesn't matter for temporary campaigns because they don't have any and setting the new campaign will load new campaign macro init. I don't think add-ons get this luxury, and I'm not sure if the existing code is calling onCampaignLoad at times I wasn't expecting.

This is hard.

What I would suggest is to do this work

  1. Create a new CampaignLoaded event in net/rptools/maptool/client/events
  2. in MapTool.setCampaign()
    a. create a copy of the reference to the existing campaign var oldCampaign = client.getCampaign()
    b. At the end of the method, compare the campaign to the old one, if they do not match, send out the CampaignLoaded event
  3. In the LibraryManager, subscribe to the CampaignLoaded event and use this to empty the queued-up add-ons (from the previous comment).
  4. In CampaignManager.clearCampaignData(), call the method that sets the campaign flag to queue add-ons. (I wouldn't do this as an event as its very much sequence dependant where as the final one where the campaign is loaded isnt)

This should mean that it only gets called once per campaign load, it doesn't matter if its a temporary campaign or not it will just "process" the empty queue in that case. Technically it doesn't matter if it gets called more than once per campaign as the queue will be empty on all but the first call (but best to try avoid doing it :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Awaiting-Review
Development

Successfully merging this pull request may close these issues.

[Bug]: Add-on onInit fails if it requires data from e.g. a table
2 participants