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

Allow NPC Protect Effect #488

Merged
merged 3 commits into from
Nov 8, 2024
Merged

Conversation

tegstewart
Copy link
Contributor

A while back, I gave custom pets the protect ability to apply to their owners, but it never worked properly as ProtectEffect assumed both the source and target of protect effects were players. This remedies that by allowing the effect to be applied by and on GameLivings.

The ability handler already limits players to applying the effect to other players in their group, so vanilla behaviour hasn't changed, and the only other possible application currently is custom pets protecting their owners. That said, I tried to future proof it so that other contexts are possible. Somebody could create a group vs group fight between NPCs, and have tank NPCs protect healing NPCs, for example.

While I was there, I cleaned up the code and addressed compiler warnings and recommendations.

Corrected ProtectEffect not working as the wrong Start() method was being called.
Remedied protect effect code assuming the source was a player and causing a null exception when that is not the case.
ProtectEffect no longer assumes both the source and target are players, allowing the effect to work and no longer throw exceptions when the effect is being applied by and/or to NPCs.

Removed a superfluous player group check, as that is already done in ProtectAbilityHandler.Execute().

I'm specifically using this for custom pets that protect their owner, but it could in theory be used in other contexts.  Somebody could create a group vs group fight between NPCs, and have tank NPCs apply protect to healing NPCs, for example.

While I was there, I cleaned up the code and addressed compiler warnings.
@NetDwarf
Copy link
Contributor

NetDwarf commented Nov 2, 2024

Thank you for your contribution!

Storm breaks with this PR. As far as I can tell it's probably a necessary change in Storm, but I am currently a bit too ill to think straight. Just a small heads-up. 🙂

@tegstewart
Copy link
Contributor Author

Storm breaking is weird. That implies there are NPCs with the protect ability, and whose brain applies it. Mercenary/henchmen mobs, maybe?

@NetDwarf
Copy link
Contributor

NetDwarf commented Nov 8, 2024

Storm breaking is weird. That implies there are NPCs with the protect ability, and whose brain applies it. Mercenary/henchmen mobs, maybe?

Have you played the custom dungeons in Storm? I think it is the Midwinter dungeon with the wonky aggro (among others). That's probably why, but I might be totally wrong.

@NetDwarf NetDwarf merged commit 1495c93 into Dawn-of-Light:master Nov 8, 2024
1 check passed
@tegstewart
Copy link
Contributor Author

I haven't done much on Storm, and was thinking a few days ago I should spend some time on it.

If anything, previous code should have been more likely to break things. The effect was being throwing an exception that was being caught when the effect was applied, but throwing an unhandled exception when the effect was removed.

DOL-Avalonia added a commit to DOL-Avalonia/GondwanaServer that referenced this pull request Nov 22, 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.

2 participants