[#17019] Targeted movement is now ignored by units with UNIT_FLAG_DIS…

[adeutscher]

Changes proposed:

• Units with UNIT_FLAG_DISABLE_MOVE should not be subject to targeted movement via TargetedMovementGenerator.

Target branch(es): 335, 6.x (?, see note)

Issues addressed: Partial progress towards #17019, though database adjustments are still required (see issue).

This should also make it easier to solve similar cases where a creature summons another creature that is intended to stay in one place. Some similar "Summoned X moves but shouldn't" issues that I could find:

• [3.3.5] (Ward of Laze) Mosh'ogg Witch Doctor totems not stationary and missing aura #15304
• [3.3.5] "Earthgrab Totem" chases target #15651

Tests performed:

To add UNIT_FLAG_DISABLE_MOVE to an entry without interfering with any other flags (replace FOO):

UPDATE creature_template SET `unit_flags`=`unit_flags` | 4 WHERE entry=@FOO;

• With the database adjustments brought up in [Instance] Dark Iron Land Mines #17019, teleport to Gnomeregan and fight a Dark Iron Agent (entry 6212) inside of the instance. The Dark Iron Land Mines should NOT run to meet you.
• Add a unit_flag value of 4 to Earthgrab Totem (NPC entry 6066, brought up in [3.3.5] "Earthgrab Totem" chases target #15651), and fight a Thistleshrub Rootshaper in Tanaris. The Earthgrab Totem should NOT chase you.

Known issues and TODO list:

• If 6.x has a similar problem, this should also be applied to 6.x. However, I do not have a 6.x server or client to test with.

[Killyana]

Here some sniffs, it looks like the UNIT_FLAG_DISABLE_MOVE is not used
https://gist.github.com/Killyana/1344e0838be054427f78c84413dc621d

[Shauren]

[3] Movement Flags: Root (2048)
As expected.

[Killyana]

So this Movement flag must be implemented?

[adeutscher]

While this PR is open, I'd like to confirm that people like this solution the best as opposed to any alternative and resolve any doubts that came from giving this another look.

My understanding of the problem is it's better to cut problem cases off when they're getting their location, since movement generators seem to have more double-checks in their DoInitialize or _setTargetLocation (e.g. "is the mover alive?") instead of in MotionMaster::UpdateMotion, which just checks for states like UNIT_STATE_ROOT or UNIT_STATE_STUNNED. Is this the correct approach?

If this change is accepted, I'd also like to apply the same to the other movement generators, since a unit with a "Disable Move" flag should never move at all. Should that similar of a change be made in separate PRs, since this change was originally made for a specific problem (Dark Iron Land Mines), or should the PR be renamed and expanded upon since it is essentially the same pair of lines each time?

[Aokromes]

@adeutscher
This branch has conflicts that must be resolved
Use the command line to resolve conflicts before continuing.

If adeutscher don't answers, anyone can takeover this work?

[adeutscher]

Sorry about that. I'll look into the conflicts this weekend.

[adeutscher]

Doh, or right now!

[Shauren]

This flag is not meant to be used for generic "disable movement", its related to movement behaving like player or like creature (for example, applying it allows the player to climb steeper hills/wallclimb)
This flag needs to be renamed

[adeutscher]

Could I confirm the location where this is used for steepness/wallclimbing? If this is the case, then the wiki also and good number of other places might also need to be corrected and/or clarified.

The comment field for UNIT_FLAG_DISABLE_MOVE in the creature_template page outright says "Prevents the creature from moving at all", dating back to v125 of the page in January of 2016. I mostly based my decision to use this flag based on this comment.

The flag is also used in a few different places in 3.3.5 that suggest to me that it's already being used for disabling movement (code lines not directly involving UNIT_FLAG_DISABLE_MOVE are for context).

In the Player and Unit classes, it mostly seems to be used when dealing with flight paths or possession (the possessor gets the flag set for them).

For Kel'thuzad (initial aggro?):

Talk(SAY_SUMMON_MINIONS);
me->SetFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_NON_ATTACKABLE | UNIT_FLAG
_DISABLE_MOVE | UNIT_FLAG_NOT_SELECTABLE);

For Ionar in Halls of Lightning when dispersing:

me->AttackStop();
me->SetVisible(false);
me->SetFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_NON_ATTACKAB
LE|UNIT_FLAG_NOT_SELECTABLE|UNIT_FLAG_DISABLE_MOVE);

Ionar resetting:

lSparkList.DespawnAll();
Initialize();
me->RemoveFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_NON_ATTACKABL
E|UNIT_FLAG_NOT_SELECTABLE|UNIT_FLAG_DISABLE_MOVE);

Hodir's Flash Freeze blocks:

struct npc_flash_freezeAI : public ScriptedAI
    {
        npc_flash_freezeAI(Creature* creature) : ScriptedAI(creature)
        {
            Initialize();
            instance = me->GetInstanceScript();
           me->SetDisplayId(me->GetCreatureTemplate()->Modelid2);
           me->SetFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_DISABLE_MOVE | UNIT_FL
AG_STUNNED | UNIT_FLAG_PACIFIED);
    }

It is also already in practice in places like MotionMaster::MoveChase and MotionMaster::MoveFollow in a way that suggests "if the flag is here, then don't bother continuing further into the method":

void MotionMaster::MoveChase(Unit* target, float dist, float angle)
{
    if (!target || target == _owner || _owner->HasFlag(UNIT_FIELD_FLAGS, UNIT_FLAG_DISABLE_MOV
E))
    return;

In addition, the flag is also set in the database for a number of other creature_template entries that sound like they should not be moving, separate from my meddling with land mines:

• Argent Cannon
• Eye Stalk
• Combat Dummy
• Legion Flak Cannon
• Wintergrasp Tower Cannon
• Riven Widow Cocoon
• Rocket Strike
• VX-001 (Mimiron's phase 2)
• Toasty Fire (Hodir?)
• Snowpacked Icicle Target

[Shauren]

All of them are wrong and were assigned by developers basing on the incorrect name.
As for the location - this is a clientside flag so reversing the client.

[r00ty-tc]

I think the real question to be asking is, what do we call it? What is an apt name for it? Does it have a different effect when applied to a creature?

This piqued my interest though.

(for example, applying it allows the player to climb steeper hills/wallclimb)

I wonder if this is part of the fear falling through floor/running through mountains problem that I was always stuck with when trying to fix the general Z position problem? For sure, much of it was inappropriate path generation, and the client not being too happy about that generated path. Other oddities were where on the client you seem to be "jumping" through the air. But the generated path definitely had you on ground level.

In fact, I think that server side it was never falling through the ground. But, if fear finished at a point the client "thought" it was in a mountain or underground, then the client takes over sending location and thus you're now falling.

No idea if it's the case or not. But, something worth looking at, at some point. Unless that flag is already set of course.

[Shauren]

@r00ty-tc b36da77#diff-21815ce03789521b9bbf6217e3b97d64R618

[r00ty-tc]

Yep, makes sense.

[adeutscher]

Dang. To confirm, if UNIT_FLAG_DISABLE_MOVE is entirely off the table, does that make setting a rooted state via smart_scripts the only accepted way of disabling all movement for non-bosses?

[Shauren]

Anything that ends up calling Unit::SetRooted (be it SAI or C++ script) is the way to go

[adeutscher]

Will do. Going back to UNIT_FLAG_DISABLE_MOVE, perhaps the new name could be something like UNIT_FLAG_IGNORE_SLOPES or UNIT_FLAG_DIRECT_PATHING?

[Shauren]

It's already renamed

[ghost]

@adeutscher : #17040 (comment) (b36da77#diff-21815ce03789521b9bbf6217e3b97d64L618)

[adeutscher]

Sorry about that, I should've checked the commit history before putting forward name ponderings.

If it's alright with everyone, I'd like to take a stab at tackling all of the spots where the former DISABLE_MOVE flag was used incorrectly, and updating them to root the subject instead. I'd be submitting the work in a new PR, of course.