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

#5139 added prior mapID to onChangeMap #5140

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

Jmr3366
Copy link
Contributor

@Jmr3366 Jmr3366 commented Jan 18, 2025

Identify the Bug or Feature request

#5139 to add prior mapID.

Description of the Change

When using the onChangeMap event, the output will now include both the going to mapID and the coming from mapID

Possible Drawbacks

There is a change in the macro.args passed output so any macro that does not account for two string parameters will likely fail and/or generate an error.

Release Notes

When the onChangeMap macro code is executed per the wiki,
it will now return two map ID's
landing mapID, launched from mapID


This change is Reviewable

Jmr3366 and others added 2 commits January 18, 2025 10:41
I placed the priorMapID above some error checks causing inconsistent results.
@cwisniew
Copy link
Member

@Jmr3366 have you tested this when loading a new campaign because I think this might send the zone id of the previous campaign

@Jmr3366
Copy link
Contributor Author

Jmr3366 commented Jan 19, 2025

Yes Sir, you are correct. I did test loading a campaign several times but it was the same one and I didn't catch that. Let me dig deeper

@Jmr3366
Copy link
Contributor Author

Jmr3366 commented Jan 19, 2025

@cwisniew I added the checks to priorMapID.
It seems at times the ZoneLoadedListener event is NOT triggered every time

When I load a campaign fresh from a MT closed state, it triggers every time (eg close MT, open it, load a camp.).
When I have MapTool open and load a new campaign, it triggers.
If I open another campaign, it stops triggered until I change maps (then it does trigger).
However loading a new campaign again it does NOT trigger (until I change maps).

-edit/updated-

Test process:
campaignA: 3mb
campaignB: 14mb (a copy of A with more stuff )
campaignC: 62mb

fresh build running via ide
Load 1 campaignA: onChangeMap 0F1F5C9FEFF34A9C8022602214146A13
Load 2 campaignB: onChangeMap 0F1F5C9FEFF34A9C8022602214146A13
Load 3 campaignC: onChangeMap D167B57E0AB94AF69E430F21665CA215
Load 4 campaignA: nothing
Load 5-7 campB,C,A: nothing

New Camp, add library token, add onChangeMap macro,
add Map (triggered): onChangeMap 689AECFA1A7E40458614C7DD65D720EF

Closing MT resets this.
This may be a new bug (not certain) and believe it's unrelated to what I've done re: priorMapID addition

@kwvanderlinde
Copy link
Collaborator

I forget if I brought this up before - if I have I apologize for rehashing the thought.

I've been thinking it is strange for onChangeMap to be fired in response to ZoneLoaded instead of ZoneActivated. Because ZoneActivated has to wait for assets to load, there is a gap where the current map has changed, but onChangeMap has not fired. Here's the sequence, where we're switching from "Map A" to "Map B":

  1. To start, getCurrentMapName() returns "Map A".
  2. ZoneDeactivated("Map A")/ZoneActivated("Map B") fire. Now getCurrentMapName() returns "Map B", but no onChangeMap is triggered.
  3. ZoneLoaded("Map B") fires, triggering onChangeMap.

Between (2) and (3) there can be quite a gap especially if syncing large assets.

The reason I'm bringing this up here is that a prior map ID would be more natural to implement in response to ZoneActivated as the event could carry that old zone if there was one. And it would close the gap mentioned above.

@kwvanderlinde
Copy link
Collaborator

@Jmr3366

When I load a campaign fresh from a MT closed state, it triggers every time (eg close MT, open it, load a camp.).
When I have MapTool open and load a new campaign, it triggers.
If I open another campaign, it stops triggered until I change maps (then it does trigger).
However loading a new campaign again it does NOT trigger (until I change maps).

...

This may be a new bug (not certain) and believe it's unrelated to what I've done re: priorMapID addition

Yes, we have a timing bug here. It is not related to this PR, but has always been a weakness of onChangeMap since it was introduced. Not that onChangeMap is at fault - we really ought to have a better lifecycle for campaign loads in general.

Here's the summary. The LibraryTokenManager detects new Lib: tokens using the TokensAdded event, and queues up a task to consume the tokens using SwingUtilities.invokeLater() (so it will run in the next event loop iteration). In the meantime, the first zone is activated, which leads to two possibilities:

  1. If we don't have to wait for assets, the zone is loaded immediately and ZoneLoaded fires (this is before the LibraryTokenManager is able to add the token). So when ZoneLoadedListener receives ZoneLoaded, the LibraryTokenManager still doesn't think there are any Lib: tokens to check.
  2. If we do have to wait for assets, the zone is not loaded. Then LibraryTokenManager's task is able to complete and the Lib: tokens are recognized. At some point in the future, the zone loads, firing ZoneLoaded. This time, when ZoneLoadedListener receives the ZoneLoaded event, the LibraryTokenManager recognizes the Lib: tokens and thus executes their onChangeMap.

The reason closing MT reset the behaviour you're seeing is that a fresh MT instance doesn't have the first zone's assets loaded into memory yet. So if the first zone has assets to load, there will consistently be a delay in firing the ZoneLoaded event and we're in case (2). But if we're opening the campaign as second time, the assets are already in memory, we don't have a delay for ZoneLoaded, and we're in case (1). Another example of case (1) is loading a campaign where the first zone is a blank map with a solid colour background, so no assets to load.

@cwisniew
Copy link
Member

cwisniew commented Feb 9, 2025

@Jmr3366

When I load a campaign fresh from a MT closed state, it triggers every time (eg close MT, open it, load a camp.).
When I have MapTool open and load a new campaign, it triggers.
If I open another campaign, it stops triggered until I change maps (then it does trigger).
However loading a new campaign again it does NOT trigger (until I change maps).
...
This may be a new bug (not certain) and believe it's unrelated to what I've done re: priorMapID addition

Yes, we have a timing bug here. It is not related to this PR, but has always been a weakness of onChangeMap since it was introduced. Not that onChangeMap is at fault - we really ought to have a better lifecycle for campaign loads in general.

Here's the summary. The LibraryTokenManager detects new Lib: tokens using the TokensAdded event, and queues up a task to consume the tokens using SwingUtilities.invokeLater() (so it will run in the next event loop iteration). In the meantime, the first zone is activated, which leads to two possibilities:

  1. If we don't have to wait for assets, the zone is loaded immediately and ZoneLoaded fires (this is before the LibraryTokenManager is able to add the token). So when ZoneLoadedListener receives ZoneLoaded, the LibraryTokenManager still doesn't think there are any Lib: tokens to check.
  2. If we do have to wait for assets, the zone is not loaded. Then LibraryTokenManager's task is able to complete and the Lib: tokens are recognized. At some point in the future, the zone loads, firing ZoneLoaded. This time, when ZoneLoadedListener receives the ZoneLoaded event, the LibraryTokenManager recognizes the Lib: tokens and thus executes their onChangeMap.

The reason closing MT reset the behaviour you're seeing is that a fresh MT instance doesn't have the first zone's assets loaded into memory yet. So if the first zone has assets to load, there will consistently be a delay in firing the ZoneLoaded event and we're in case (2). But if we're opening the campaign as second time, the assets are already in memory, we don't have a delay for ZoneLoaded, and we're in case (1). Another example of case (1) is loading a campaign where the first zone is a blank map with a solid colour background, so no assets to load.

This is one of the reasons that new features will be bypassing lib:tokens, their life cycle is just too unreliable and fixing them all has larger implications for how things work currently

@Jmr3366
Copy link
Contributor Author

Jmr3366 commented Feb 10, 2025

Did you want me to back out the PR or update the wiki to explain the issue, if this gets implemented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants