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

Fix transition panic while currently in transition #8

Merged
merged 9 commits into from
Jun 2, 2024

Conversation

harbdog
Copy link
Contributor

@harbdog harbdog commented May 28, 2024

Fixes issue #7

@harbdog
Copy link
Contributor Author

harbdog commented May 28, 2024

Forgot that it also needs to fix the director panic, getting that now

@harbdog harbdog marked this pull request as draft May 28, 2024 01:45
@harbdog harbdog force-pushed the fix-transition-panic branch from 29c0fe2 to e09482a Compare May 28, 2024 02:52
@harbdog harbdog marked this pull request as ready for review May 28, 2024 02:54
@joelschutz
Copy link
Owner

joelschutz commented May 28, 2024

Great, thank you for the fix. I'm writing an acknowledgment so people know that the expected behaviour is to cancel the current transition.

I prefer to simplify the conditions so everytime a switch or trigger is processed while a transition is running cancels the transiton. I think that cancelling the transition is better because if you need to prevent this behaviour you can prevent switches and triggers to occur using using a transitionAware scene. But I'd like to know why did you add the return on the ProcessTrigger, so I'm not missing anything.

@harbdog
Copy link
Contributor Author

harbdog commented May 28, 2024

The PR as currently written will not just end the current transition but skip on to the transition of the next scene (as if you had clicked to start that next scene after waiting for the transition to finish).

However, to answer your question about the return in SceneDirector.ProcessTrigger was because without the early return it would just end up replaying the same transition over, and I'm not sure why just that playing with it and putting a return in made it behave like the SceneManager.SwitchWithTransition I just described rather than replaying it.

@joelschutz
Copy link
Owner

Strange. I'll create some tests and investigate further. I'm afraid that any losing ends on that part could kill the library for large projects. Luckily you spoted it early. Thank you.

@harbdog
Copy link
Contributor Author

harbdog commented May 28, 2024

I should note I was using a modified examples/director where I gave both scenes a transition instead of just the one, that might give you a better idea of what happens when you see it.

@joelschutz
Copy link
Owner

I decided to remove the trigger checking that you added since the I figured that if the ProcessTrigger is called by a different trigger the bug would still occur. Maybe what you found was because of a pointer to the same transitions or scene, idk, I couldn't reproduce it.

I added new tests and they are running fine, checkout if they help you find the problem with the example. If everything is fine we can release the patch.

director.go Show resolved Hide resolved
harbdog and others added 2 commits May 28, 2024 20:48
@joelschutz
Copy link
Owner

Thanks for the updated example. I found the problem and explained better on the revision. To prevent this double call I added some conditions to the example. Now it seens to work as expected, a double click will bring you to the ThirdScene instead of the FirstScene.

@joelschutz joelschutz merged commit 577c465 into joelschutz:master Jun 2, 2024
1 check passed
@harbdog harbdog deleted the fix-transition-panic branch June 7, 2024 19:57
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.

2 participants