Skip to content

Conversation

@LordMidas
Copy link
Member

Vanilla use function has checks built in which may cause it to return early and not actually execute the skill. Hooking onUse ensures that we only fire it when the skill is being executed, however in this case we still need a flag to switch from use as onUse can be called manually as well.

This will require us to update our documentation as it currently says that these events are fired before and after use. So now we'd have to say they fire before and after onUse.

Vanilla `use` function has checks built in which may cause it to return early and not actually execute the skill. Hooking onUse ensures that we only fire it when the skill is being executed, however in this case we still need a flag to switch from `use` as `onUse` can be called manually as well.
@Darxo
Copy link
Contributor

Darxo commented Feb 6, 2025

An event for the initial onBeforeAnySkillExecuted timing is very much worth keeping.
At this point no Action Points nor Fatigue have yet been spent so this is the perfect event to snapshot those values, if we want to create effects that are based on the amount of AP/Fatigue you have spent, or which reimburse you your spent resources.

With this mods proposal we also have a small issue that there might already be mods out there using onBeforeAnySkillExecuted for exactly the above purpose, which would break when MSU moves the timing.
Yet onBeforeAnySkillExecuted fits as a name for both timings

@LordMidas
Copy link
Member Author

LordMidas commented Feb 6, 2025

An event for the initial onBeforeAnySkillExecuted timing is very much worth keeping. At this point no Action Points nor Fatigue have yet been spent so this is the perfect event to snapshot those values, if we want to create effects that are based on the amount of AP/Fatigue you have spent, or which reimburse you your spent resources.

With this mods proposal we also have a small issue that there might already be mods out there using onBeforeAnySkillExecuted for exactly the above purpose, which would break when MSU moves the timing. Yet onBeforeAnySkillExecuted fits as a name for both timings

Actually your point is valid and this means that this PR would break the current behavior, so we cannot do it like this. Currently onBeforeAnySkillExecuted runs before AP/Fatigue is spent, so we'd have to run it earlier. Perhaps one has to go in a very weird way and hook spawnOverlay and check stack trace to confirm that it is coming from use and if yes then trigger the event. Because that function is called from use just before AP/Fatigue is spent. But that seems very ugly.

@LordMidas LordMidas force-pushed the development branch 2 times, most recently from 6677ac8 to ae825b7 Compare June 17, 2025 23:43
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.

[BUG] onBeforeAnySkillExecuted and onAfterAnySkillExecuted do not guarantee that the skill was actually executed

3 participants