[patch] Improve SelectAttackingTarget feature ;)

[Schmoozerd]

Added in [11793]

Description of the feature?

Put some Additional checks to SelectAttackingTarget

For which repository revision was the patch created?

11662

Is there a thread in the bug report section or at lighthouse?

none - But NoFantasy already marked the selection for players (though in different way)

Who has been writing this patch?

me

pretty self explaining, on the long term I think this will be usefull to iE use the corresponding LoS & Range Check as default for EventAI, but there might be some discussion related.

For SD2 this is surely helpful (and could even use more functions, but KISS)

Patch:

http://paste2.org/p/1489932 (11662)

History:

http://paste2.org/p/911613 Ver for 10179

http://paste2.org/p/949045 (for 10353) - additional check melee range flag, needs break;

http://paste2.org/p/951208 (R 10360) style issues, needs break;

http://paste2.org/p/1094614 (10728, last tested was 10439, but no code changes)

http://paste2.org/p/975457 (R 10439, tested)

[zergtmn]

Good idea but I don't like your naming style

Also I think better to use functors here. Something like this:

template<typename Check>
Unit* Creature::SelectAttackingTarget(AttackingTarget type, uint32 position, Check condition)
{
   ThreatList const& threatList = m_creature->getThreatManager().getThreatList();
   UnitList targetList;

   for (ThreatList::const_iterator itr = threatList.begin(); itr != threatList.end(); ++itr)
   {
       if (Unit *pUnit = Unit::GetUnit(*this, (*itr)->getUnitGuid()))
       {
           if (condition(pUnit))
               targetList.push_back(pUnit);
       }
   }

   UnitList::iterator itr = targetList.begin();
   UnitList::reverse_iterator ritr = targetList.rbegin();

   if (targetList.empty() || position >= targetList.size())
       return NULL;

   switch (target)
   {
       case ATTACKING_TARGET_RANDOM:
           std::advance(itr, urand(position, targetList.size() - 1));
           return *itr;

       case ATTACKING_TARGET_TOP:
           std::advance(itr, position);
           return *itr;

       case ATTACKING_TARGET_BOTTOM:
           std::advance(ritr, position);
           return *ritr;
   }

   return NULL;
}

Then calls would be like this:

if(Unit* target = m_creature->SelectAttackingTarget(ATTACKING_TARGET_TOP, 0, UnitInRangeCheck(m_creature, ATTACK_DISTANCE)))
   ...

This approach is more universal and simpler in use.

[Schmoozerd]

I don't think, that using a functor way is easier to read

but you are right, it is more powerfull.

perhaps this could be both be used (iE the SELECT_FLAGS defining a functor)

but with the more powerfull possibility to directly say which targets shall be selected, you will open the doors to "hack", iE hardcoding MELEE_RANGE instead of using the spell-property

[zergtmn]

I don't think, that using a functor way is easier to read

but you are right, it is more powerfull.

perhaps this could be both be used (iE the SELECT_FLAGS defining a functor)

but with the more powerfull possibility to directly say which targets shall be selected, you will open the doors to "hack", iE hardcoding MELEE_RANGE instead of using the spell-property

Functors allow to check targets in any way you want. You can create a functor that will accept SpellEntry and pointer to caster in constructor and check if targets are in proper range for that spell. I don't bother if someone wants to use hacks in his private fork.

[Schmoozerd]

I think the most common case would be:

in Los && inSpellRange

and this would (I think) be needed for nearly all calls of SelectAttackingTarget.

so it would be natural, to define this functor as a member of Creature. "CanTargetablWithSpell"

but If someone then needs an additional Player Check, he has to define a new functor containing CanTargetablWithSpell && isPlayer();

this is more overhead than to just add the Player flag.

Also the concept of flags is more natural if you think of EventAI - perhaps one or another Flag makes it into EventAI::CastFlags - if we have access by flags, it is 'just' passing the flags on, if we use (only) functor, we have to translate the CastFlags to functors before using them.

As I said already: A functor-approach could be a valid enhancement for the Flags, (that is the flags using internally pre-defined functors), but I don't know if it was the right thing, to provide such a generic solution for something that will be only used a few times.

[Schmoozerd]

updated, and added an additional flag, that I intend to use for SelectHostileTarget.

So in my view further progression:

* Move EventAI Casts to additional checks for SpellRange/ LoS (to prevent missing casts)

* SD2 can use it whereever needed

* SelectHostileTarget - I think this one needs updateing, as for some bosses there we need to only melee ranged targets, and if not cast some spells or similar; an idea (could also be handled different) would be for this http://paste2.org/p/949047 (though not yet tested)

[zergtmn]

How you would implement custom sorting order in target selection? in case you need n-th unit from list ordered by health/mana/distance/aura stacks... I did it using additional functor

For speedup I'm using two functions, with and without sorting. Also I have an idea of implementing function SelectUnits in similar way for choosing several targets that satisfy some criteria (functor).

[Schmoozerd]

how would I implement something special for one particular boss?

like we always did it: in SD2 directly

I had in mind to only implement the things that are needed in Core and Acid, not to provide a generic system for SD2 (because we always would be able to iterate the threat list there)

However if a Dev says something like "gee, I always wanted to do something with functors" then I am perfectly fine to add a functor version, and move my flags to functors

[zergtmn]

Not one boss, clearly.

I had in mind to only implement the things that are needed in Core and Acid

For ACID standard SelectAttackingTarget is enough. When do you plan to use expanded function in core?

Having multiple solutions with one purpose is bad (one in core, one in SD2 and many similar in scripts). With functors it's possible to have one solution suitable for every boss.

[Toinan67]

Little bump here, this patch will be quite appreciated when pushed to master

Schmoo and zergtmn, continue your debate about functors/flags it's quite interesting btw.

[Toinan67]

Anyone interested in this great patch?...