Incorrect event order - playerEnteredNewGalaxy after shipWillExitWitchspace

[AndSDev]

According to the documentation:

playerEnteredNewGalaxy
The playerEnteredNewGalaxy handler is called just before shipWillExitWitchspace.
the sequence of events for a player jumping to a different galaxy is as follows:

shipWillEnterWitchspace (world event)
playerWillEnterWitchspace (NPC ship event)
playerEnteredNewGalaxy (world event)
shipWillExitWitchspace (world event)
shipExitedWitchspace (world event)

But in log:

19:41:21.420 [test]: shipWillEnterWitchspace
19:41:21.426 [test]: playerWillEnterWitchspace
19:41:23.309 [test]: shipWillExitWitchspace
19:41:23.486 [test]: playerEnteredNewGalaxy
19:41:27.415 [test]: shipExitedWitchspace

Tested on oolite-trunk-1.85.0.160812-dev.mac (Mac OS X).

[AnotherCommander]

Behaviour confirmed and it is because of this in PlayerEntity -enterGalacticWitchspace:

[self witchEnd]; // sets coordinates

[self doScriptEvent:OOJSID("playerEnteredNewGalaxy") withArgument:[NSNumber numberWithUnsignedInt:galaxy_number]];

The [self witchEnd] part ends up calling the PlayerEntity -leaveWitchspace method, which contains the call to shipWillExitWitchspace. The question is, is this a bug in the code or a bug in the documentation?

[AndSDev]

I think it is logically correct behavior described in the wiki.
But in the code it should look something like this:

[self witchEnd]; // sets coordinates

[self doScriptEvent:OOJSID("playerEnteredNewGalaxy") withArgument:[NSNumber numberWithUnsignedInt:galaxy_number]];
[self doScriptEvent:OOJSID("shipWillExitWitchspace")];

[AndSDev]

Current system don't set in playerEnteredNewGalaxy. It's correct?

18:19:42.169 [test]: shipWillEnterWitchspace [G:0 S:33]
18:19:42.173 [test]: playerWillEnterWitchspace [G:0 S:33]
18:19:42.330 [test]: playerEnteredNewGalaxy [G:1 S:-1]
18:19:47.163 [test]: shipWillExitWitchspace [G:1 S:89]
18:19:54.292 [test]: shipExitedWitchspace [G:1 S:89]

[AnotherCommander]

No (dang it). Re-opening issue. We'll need to think a bit more about it.

[AnotherCommander]

I was hoping that just executing the playerEnteredNewGalaxy handler before the witchEnd call would be all that is needed to have this fixed, but unfortunately it seems that the truth is quite different. The methods called by witchEnd actually set up plenty of system stuff necessary for concluding a jump. Because of this, I don't think it's possible to put the playerEnteredNewGalaxy handler anywhere early enough to ensure that it runs before shipWillExitWitchspace, without changing existing functionality at the same time.

Unless I have done my searches wrongly, it looks like that particular playerEnterednewGalaxy wiki entry was inserted in 2009, which implies that the intention was to have the order of events as described. But it looks like we've been having the event handler switch going on since at least 2011, which is plenty of years ago. Not sure when or why it was changed, or even whether it was intended or not, but it seems that this has been the status quo for a long time. That's why I hinted earlier that maybe the wiki entry change could be a much less painful fix.

I think that the changes we may have to apply just to get the switch working without affecting anything could be substantial. They may even introduce other unwanted side -effects. Therefore, the best way forward is in my opinion:

• Revert commit 05ec85b to ensure that no unintended side effects like the one pointed to by AndSDev occur.
• Try to see if there could be a transparent way to apply the handler switch.
• If no such way can be found or is too complicated to feel comfortable, do not change anything and just update the wiki to reflect the order of events that we've had for the last five years or so. I don't think that logic is violated if we accept the playerEnteredNewGalaxy handler running after shipWillExitWitchspace. It's just a different point of view.

[AndSDev]

This patch can help? Not tested!

diff --git a/src/Core/Entities/PlayerEntity.m b/src/Core/Entities/PlayerEntity.m
index f16aba0..2d21620 100644
--- a/src/Core/Entities/PlayerEntity.m
+++ b/src/Core/Entities/PlayerEntity.m
@@ -7254,8 +7254,6 @@ - (void) enterGalacticWitchspace

    [self setBounty:0 withReason:kOOLegalStatusReasonNewGalaxy];    // let's make a fresh start!
    cursor_coordinates = PointFromString([[UNIVERSE systemManager] getProperty:@"coordinates" forSystem:system_id inGalaxy:galaxy_number]);
-   
-   [self doScriptEvent:OOJSID("playerEnteredNewGalaxy") withArgument:[NSNumber numberWithUnsignedInt:galaxy_number]];

    [self witchEnd]; // sets coordinates, calls exiting witchspace JS events
 }
@@ -7530,6 +7528,10 @@ - (void) leaveWitchspace
            [self clearRoleFromPlayer:NO];
        }
    }
+   if (galactic_witchjump)
+   {
+       [self doScriptEvent:OOJSID("playerEnteredNewGalaxy") withArgument:[NSNumber numberWithUnsignedInt:galaxy_number]];
+   }
    [self doScriptEvent:OOJSID("shipWillExitWitchspace")];
    [UNIVERSE setUpBreakPattern:[self breakPatternPosition] orientation:orientation forDocking:NO];
 }

[AnotherCommander]

Oh dear, I had completely forgotten about the galactic_witchjump variable. I tested the patch and it looks like it works. So it looks like we went straight to the second bulletpoint of my earlier comment. OK, I'll commit this fix and keep the issue open for a while to give a chance for testing to happen. @AndSDev: Thanks for lookiing into it.

[AnotherCommander]

If there are no objections, I would like to proceed with closing this issue some time tomorrow.

[AnotherCommander]

Issue is considered fixed. Closing.