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

refactor: change certain functions to only when alive #338

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

Conversation

LordMidas
Copy link
Member

@LordMidas LordMidas commented Feb 27, 2024

Some mods may kill certain entities during these functions, e.g. during onMovementFinished because of a certain skill/effect on the entity, which may then cause subsequently updated skills to crash. This change prevents such cases.

Example of a problem that is intended to be fixed by this PR: https://discord.com/channels/855921501286039582/855921501286039585/1210475458165084241

@LordMidas LordMidas added this to the 1.3.0 milestone Feb 27, 2024
Copy link
Member

@Enduriel Enduriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is fixing crashes without affecting 'working' code, then this should be correctly marked as a fix. If that is changed lgtm

@LordMidas LordMidas force-pushed the refactor-some-functions-to-only-when-alive branch from d9b5c38 to 1d97b4e Compare April 11, 2024 09:36
@LordMidas
Copy link
Member Author

LordMidas commented Apr 11, 2024

Actually on further deliberation I am not sure if this needs to be done. Looking at vanilla skill event functions, there are very few which have the isAlive check. I think ideally the modder should not kill the character during these functions.

In any case, there's also the issue of for example if a skill does the following

function onMovementFinished( _tile )
{
    ::Const.SomeGlobalCounter++;
}

If we make this event alive only then this function won't be called for this skill if a skill before this skill kills the target during that skill's onMovementFinished.

An idea could be that at the start of the skill_container's function we check for the character being alive, but during the call to the skills themselves we don't add that check. But that fails when a skill would kill the actor in onMovementFinished.

So it isn't immediately clear what the best course of action here is. So I'd wait on this PR for now.

@LordMidas LordMidas force-pushed the feat-port-to-modern-hooks branch from de55d30 to 983c30b Compare April 13, 2024 04:38
Base automatically changed from feat-port-to-modern-hooks to development April 13, 2024 04:39
Some mods may kill certain entities during these functions, e.g. during onMovementFinished because of a certain skill/effect on the entity, which may then cause subsequently updated skills to crash. This change prevents such cases.
@LordMidas LordMidas force-pushed the refactor-some-functions-to-only-when-alive branch from 1d97b4e to 6f18397 Compare April 13, 2024 05:10
@LordMidas LordMidas modified the milestones: 1.3.0, 1.4.0 Apr 24, 2024
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