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

[CORE/SPELLS] Fix immuniry checks on apply aura. #4838

Closed
wants to merge 1 commit into from

Conversation

devilcoredev
Copy link
Contributor

Some AOE auras need a immunity checks before the handle of their effects. This commit will fix a bug introdouced on 08bcbc8

@Vincent-Michael
Copy link
Contributor

work fine :)

@QAston
Copy link
Contributor

QAston commented Jan 17, 2012

This won't work because HandleEffect here just turns on the effect of the aura, aura will be applied anyways on unit update. Imo, you should edit effectMask variable at the begining of the Spell::DoSpellHitOnUnit function, so everything will be handled properly and with minimal effort.

@devilcoredev
Copy link
Contributor Author

@QAston i've tested my fix and it works fine, it works for area auras like Desecration, it was part of a fix (#4229) that i've forgotten on this pull request: #4229

@QAston
Copy link
Contributor

QAston commented Jan 18, 2012

Ok, you're right, i didn't check your patch correctly @ first, there's still a problem however.Imo it's not a good idea to add immunity check in there - Aura::UpdateTargetMap is a place for such manipulations because there're immunity checks for area auras.
Also about 08bcbc8 :
08bcbc8#L2L1565
this check will only prevent aura effects being applied only on first update, because the check is run again in Aura::Update. You can avoid this problem by changing effMask in whole function, so aura will be created without effects to which target is immune, and then update problem is gone.
If you won't do that, aura will be readded to the target after immunity to a non-area-aura expires (dunno if it's intended behaviour or not)

@devilcoredev
Copy link
Contributor Author

Mmmm, i don't have the problems that you have mentioned :|

@Star-lion
Copy link
Contributor

we can has? :D

@QAston
Copy link
Contributor

QAston commented Jan 23, 2012

I've designed that part of the code (auras) so i know what i'm saying:P Anyways - it's not just a matter of bugs, putting immunity check in Unit::_ApplyAura is wrong by design, that should be handled in place where spell tatrgets are checked (Aura::UpdateTargetMap), because Unit::_ApplyAura is fully abstract and should be left that way (we don't want to have messy code, right?). I'll take care about this pull request in the middle of february, I'm too busy to apply and review now, sorry for that:(

@Star-lion
Copy link
Contributor

^that. QAston you should really go in the irc ^^

@ghost ghost assigned QAston Jan 24, 2012
@QAston QAston closed this Feb 4, 2012
@Star-lion
Copy link
Contributor

its mid feb :D

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.

None yet

4 participants