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/Script/Spell: Fix bug when Mirror Images could not start attack standin… #14896

Merged
merged 1 commit into from Dec 12, 2015

Conversation

Ofinka
Copy link

@Ofinka Ofinka commented Jun 15, 2015

fixes problem with attack to standing characters #14877

@Keader
Copy link
Member

Keader commented Jun 15, 2015

Hello Ofinka \o
First thanks for the fix. I tested your fix, it's working. However're a little problem. The spell says the mirrors must attack the target of greatest threat, with its fix this is not happening. If you use a mirror image of the dummy ironforge (.tele themilitaryward), will notice that the images will attack random targets

@Goatform
Copy link
Contributor

I think that all images should to attack same target if they detect him, and if owner start attacking another target, they should to switch target and attack owner's target.

@ghost
Copy link

ghost commented Jun 16, 2015

@Keader : if you read the code, it is written to do only 1 of 2 things; either attack owner's victim (owner->GetVictim()) or attack a random victim (AnyUnfriendlyUnitInObjectRangeCheck). If you want the mirror images to attack a target/victim with highest threat, some extra code needs to be added.
(I don't know if it can be added in the same place, though.)

@Ofinka
Copy link
Author

Ofinka commented Jun 16, 2015

ye guys i realize victim choosing of this code is based on gargoyle targeting system.. i dont know why did I think it is right to do that this way. I will try to update this PR asap with threat victim finding if I find some way how to do that effectively.. It is a problem do it like a spellscript because i dont know how to make threat list for owner yet.. maybe if someone can give me some hint it would be faster :D

@Keader
Copy link
Member

Keader commented Jun 16, 2015

The real problem is that sometimes you own target, use the spell and 1 or 2 images attack other random target. If we ever attack the mage target maybe the prejudice "not to attack the first in the list of threat" goes unnoticed.

You can test this next giving Duel in a player near the NPCs another faction

@Killyana
Copy link
Member

Some informations from wowwiki:

  • The Mirror Images inherit the Mage's threat lists
  • Patch 3.0.8 (2009-01-20): In addition, Mirror Images will no longer have excessive threat values assigned to them when they are created.
  • Patch 3.1.2 (2009-05-19): The images summoned by this spell will now target the creature that most hates the Mage, and should no longer cast Fire Blast or Frostbolt on targets that are affected by crowd control debuffs that break immediately on damage unless they are already casting these spells when crowd control is applied.
  • The mirror images will no longer complete casts of Frostbolt on targets which are Polymorphed at the time their Frostbolt channel finishes.

Other informations from forums:

  • They attack same target as you, and follow you always - the only way they behave, and only way to control them
  • Didn't read everything but, they seem to attack whatever you have most threat on. This is why if you pop them before a boss is pulled, they attack nothing as you have threat on nothing till you first attack the boss yourself. In my experience they change target when you Invi to drop threat and such.

@Goatform
Copy link
Contributor

AnyUnfriendlyUnitInObjectRangeCheck function isn't good solution for this problem at all because they shouldn't attack random unit. They just should to attack your target once you start attacking him, and they shouldn't stop attacking that target unless you CC him or switch on new target.

@Ofinka
Copy link
Author

Ofinka commented Jun 18, 2015

try this update pls, any feedback is welcomed

the main idea is choosing target based on threat after summon like references above, then changing targets with owner.. this update should also solve problem with interrupting cc after summon in #14877

@Ofinka Ofinka changed the title Core/Spell: Fix bug when Mirror Images could not start attack standin… Core/Script/Spell: Fix bug when Mirror Images could not start attack standin… Jun 18, 2015
@Keader
Copy link
Member

Keader commented Jun 19, 2015

@jackpoz Do you have any suggestions for how to let code less?

@Ofinka The rotation stood very good \o/

@Keader
Copy link
Member

Keader commented Jun 22, 2015

@Ofinka The CC mechanics are without work. Whenever you are in CC they attack, totally ignoring the mechanics.

Polymorph First : https://youtu.be/SZzK6eTL4bk

Mirror Image First: https://youtu.be/OpgsWbl1IVc

@Ofinka
Copy link
Author

Ofinka commented Jun 22, 2015

works for me

@Keader
Copy link
Member

Keader commented Jun 22, 2015

Work for me too 👍

@mik1893
Copy link
Contributor

mik1893 commented Jun 30, 2015

does Init() need to be called also in the reset? (asking)
Also, please squash the commits into one!
Thanks, nice job!


std::list<Unit*> targets;
Trinity::AnyUnfriendlyUnitInObjectRangeCheck u_check(me, me, 30.0f);
Trinity::UnitListSearcher<Trinity::AnyUnfriendlyUnitInObjectRangeCheck> searcher(me, targets, u_check);
Copy link
Contributor

Choose a reason for hiding this comment

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

if(!targets.empty())
{

Copy link
Member

Choose a reason for hiding this comment

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

not necessary

@Ofinka
Copy link
Author

Ofinka commented Jun 30, 2015

@mik1893 I dont see calling Init() fn in reset()

@jackpoz
Copy link
Member

jackpoz commented Aug 10, 2015

is this complete, tested and ready to be merged ?
I see some required codestyle changes but I can take care of them when merging it.

@Ofinka
Copy link
Author

Ofinka commented Oct 19, 2015

@jackpoz Ye, I think so..

@FrancescoBorzi
Copy link
Member

any news? @jackpoz ?

@FrancescoBorzi
Copy link
Member

I've been testing this for days on cc685ac with players and they are ginving me only positive feedback about it. I think it may be merged.

// Prioritize units with threat referenced to owner
if (highestThreat > 0.0f && highestThreatUnit)
me->Attack(highestThreatUnit, false);

Copy link
Member

Choose a reason for hiding this comment

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

this empty line is not needed

@jackpoz jackpoz self-assigned this Dec 12, 2015
jackpoz added a commit to jackpoz/TrinityCore that referenced this pull request Dec 12, 2015
@jackpoz jackpoz merged commit ee9c9e6 into TrinityCore:3.3.5 Dec 12, 2015
@jackpoz
Copy link
Member

jackpoz commented Dec 12, 2015

thank you for the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants