Trinitycore: [3.3.5] Core/Spell: Creature spellcast facing. The creature is respawned and not updated

[chaodhib]

I found a bug introduced in this PR: https://github.com/TrinityCore/TrinityCore/pull/15641#issuecomment-199421377

When a unit casts a spell, SendUpdateToPlayer(player) is called here: https://github.com/TrinityCore/TrinityCore/pull/15641/files#diff-f3f953a63e2a008d22de69cb68f735c4R2726

From what I understood, it updates players of the new UNIT_FIELD_TARGET of the caster mob. The problem is that the wrong packet is sent. SendUpdateToPlayer() calls BuildCreateUpdateBlockForPlayer() to construct the update packet.

The problem lies in the type of update sent. A UpdateType: CreateObject1 is sent instead of an UpdateType: Values. In other words, the packet sent will re-spawn the caster mob from scratch instead of updating it. Visually, it looks like the mob is jumping, pretty weird stuff.

I asked @Treeston about it in IRC and apparently, it's related to the grid system. @r00ty-tc could you look into this when you find time please? Thanks.

bug observed in: 0cc1af721e05c89d82429aaf04c2b8241b768252

[Treeston]

What I meant is that the mentioned snippet that handles target updating should be re-written using a grid worker instead of the hacky loop it is now.

Don't have the time to do it myself right now, sadly.

[Treeston]

<chaodhib> treeston: checkout my gluth branch
<chaodhib> pull gluth, wait that he eat a zombie
<chaodhib> when he does, when he casts the killing spell (DoCast(zombieToBeEaten, SPELL_ZOMBIE_CHOW_SEARCH_SINGLE);)
<chaodhib> that's when the bug occurus
<chaodhib> ServerToClient: SMSG_COMPRESSED_UPDATE_OBJECT (0x01F6) Length: 208 ConnIdx: 0 EP: 127.0.0.1:60947 Time: 03/21/2016 23:37:12.577 Number: 20034
<chaodhib> Count: 1
<chaodhib> [0] UpdateType: CreateObject1
<chaodhib> [0] GUID: Full: 0xF130003E3C000309 Type: Creature Entry: 15932 Low: 777

for @r00ty-tc

14865 for reference

[r00ty-tc]

@Treeston that's fine. Does that also work as a test case for the spell facing change?

[Shauren]

This is rather simple
in this method https://github.com/TrinityCore/TrinityCore/blob/6.x/src/server/game/Entities/Object/Object.cpp#L251
change
BuildCreateUpdateBlockForPlayer(&upd, player);
to

if (player->HaveAtClient(this))
    BuildValuesUpdateBlockForPlayer(&upd, player);
else
    BuildCreateUpdateBlockForPlayer(&upd, player);

also
HOLY SHIT ITERATING ALL FKIN PLAYERS ON THE MAP
HOLY SHIT 2 CALLING CanSeeOrDetect - never do that, use HaveAtClient (cached result of previous visibility update)

[r00ty-tc]

HOLY SHIT ITERATING ALL FKIN PLAYERS ON THE MAP

Yeah I think I was paged for this reason rather than the create block issue.I mentioned in IRC that there should be a grid worker for this instead of a full map iterator.

Regarding changing send create. I presume there's actually already a "proper" way to send movement packets now. Although your way would ensure any other misplaced calls to the function to still work right.

[chaodhib]

fixed in https://github.com/TrinityCore/TrinityCore/issues/16839