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

Add test and fix gen 7 onSwitchInPriority #10851

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

Conversation

andrebastosdias
Copy link
Contributor

I had this PR directly on the branch before the SwitchIn overhaul merge.

I'm not sure if all the changes are correct, particularly whether the Move: 2 is necessary. I changed the method calls to singleEvent simply because that's how it is done across all the other effects.

Healing Wish and Lunar Dance need their onSwitchInPriority updated in the Gen 7 mod to match Gen 9.

I also added a test for the Throat Spray/Magician race condition reported here.

@pyuk-bot
Copy link
Contributor

I used .call because that’s easier on the call stack, and because we have a built-in automatic crash when eventDepth gets too high. I guess it’s okay for onSwitchIn handlers to be using singleEvent because 'SwitchIn' is always eventDepth 1 the way we handle switches currently. As an example of what I’m worried about, Cloud Nine could set off an event chain like SwitchIn event -> cloudnine.onSwitchIn -> WeatherChange event -> protosynthesis.onWeatherChange -> Use event -> boosterenergy.onUse -> Start event -> protosynthesis.condition.onStart. Adding an extra singleEvent in there brings us up to eventDepth 5 which is uncomfortably close to the limit of 8. It would be nice if we didn’t need to do either, though, and could just define the alias directly in the data files, but JavaScript object literals apparently don’t support that in any way that isn’t weird and messy.

@andrebastosdias
Copy link
Contributor Author

andrebastosdias commented Jan 30, 2025

@pyuk-bot For the sake of consistency, we could use .call when it is used to avoid code repetition. Another example would be Flower Gift's Start event calling WeatherChange. I can code that tomorrow, I'm a bit busy right now.

@pyuk-bot
Copy link
Contributor

I originally had all the onStart handlers that call onWeatherChange use .call too. Looking back at #9118, I apparently swapped to singleEvent at @urkerab's suggestion without any discussion on why one would be better than the other.

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