server
server copied to clipboard
[core, lua, sql] Self-centered AoEs rework. Introduce mobskill finalizer.
I affirm:
- [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
- [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
- [x] I have read and understood the Contributing Guide and the Code of Conduct.
- [ ] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.
What does this pull request do?
Part 1 of several to clean all of the mobskills jank.
- Undo several changes introduced in #5745
- Revert the changed skills to just ENEMY in their validTargets
- Specifically the semantics of having SELF + ENEMY in validTargets hardly make sense and make the code hard to reason about.
- Using Touchdown as an example, it is a self-centered AoE with a certain radius that may or may not hit anyone. Tiamat is never targeting itself to deal damage, the only reason it was done was to ensure the animationsub update would occur in onMobWeaponskill which can be solved with finalizers instead.
- Specifically the semantics of having SELF + ENEMY in validTargets hardly make sense and make the code hard to reason about.
- Remove several if blocks that no longer make sense
- Revert the changed skills to just ENEMY in their validTargets
- Introduce onMobSkillFinalize - a finalizer function that ~~always runs~~ runs when the skill has not been interrupted, independently of targets being found
- This can be used to change an animationSub, kill mob on suicide etc even if the associated skill didn't hit anyone
- I have at least 3 separate captures showing that certain mobskills do not need to hit anyone for their side effect to occur
- Introduce a skill flag SKILLFLAG_ALWAYS_ANIMATE to replace what used to be a SELF + ANY_ALLEGIANCE check for the sake of checking if the animation needed to complete regardless of the mobskill outcome
- Removes a range check from Tiamat for Touchdown as it is no longer necessary
Steps to test these changes
Absolutely untested beyond Tiamat for now, just wanted to collect feedback for the time being. I still need to grab a few captures on retail first.
I'm gonna mark it as ready for review because there isn't much more I can do without massively increasing the scope to resolve the other issues. I need to rewrite targetfind and how mobskills are defined in a separate PR.
Changes since it was opened:
- Made finalizer execute only if the move wasn't stunned
- Couple of tweaks to the mobskill state to handle the stun check correctly
- Made Final String use the finalizer pattern
I've tested extensively Touchdown/Self-Destruct and they behave correctly in a retail-accurate fashion without the messy targetfind flags in DB.