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

A bit of SMSG packet structure #24

Merged
merged 4 commits into from Jun 28, 2017
Merged

A bit of SMSG packet structure #24

merged 4 commits into from Jun 28, 2017

Conversation

Olion17
Copy link
Contributor

@Olion17 Olion17 commented Jun 24, 2017

  1. DetailsEmote. Thanks to brotalnia and the Elysium Project. The code resembles the Elysium's solution closely except one minor improvement. The placeholder for DetailsEmote array in the SMSG_QUESTGIVER_QUEST_DETAILS packet has also variable length.

2+3. Racial Leaders. Minor fix of SMSG_CREATURE_QUERY_RESPONSE packet, which includes now bool RacialLeader flag. Prevent the leaders dropping PvP due to usual faction rules in Creature::UpdateEntry().

  1. MSG_QUEST_PUSH_RESULT had 1 excessive uint32 field.

All this info comes from 5875 client build.

@brotalnia
Copy link
Contributor

Why is PvP even being removed in UpdateEntry, without taking unitflags into account? I was trying to figure out why many NPCs don't show as PvP, even though they have the PvP unitflag (4096) in their template. Turns out the database value is ignored because of this, and it's just setting PvP based on the faction. This is wrong, because it leads to many, many NPCs not being toggled for PvP, when it can clearly be seen that they should be in retail videos.

Just some examples:
Kaltunk (10176)
Vanilla - https://youtu.be/jTSyexUNN9E?t=1m22s

Gornek (3143)
Vanilla - https://youtu.be/jTSyexUNN9E?t=1m33s

Duokna (3158)
https://youtu.be/jTSyexUNN9E?t=5m4s

Zureetha Fargaze (3145)
https://youtu.be/jTSyexUNN9E?t=5m36s

Jen'shan (3154)
https://youtu.be/jTSyexUNN9E?t=5m39s

The if in UpdateEntry ought to be changed to something like this:
if ((factionTemplate->factionFlags & FACTION_TEMPLATE_FLAG_PVP) || (unitFlags & UNIT_FLAG_PVP))

@Olion17
Copy link
Contributor Author

Olion17 commented Jun 25, 2017

Why is PvP even being removed in UpdateEntry?

My simple answer: I don't know :) Now some details.
Basically, it may mean that all the mobs mentioned have wrong faction(template) set in creature_template. Part of the functions of FactionTemplate/Faction mechanic is determining relations: aggressive, neutral, friendly. So if a mob has PvP flag, it must be aggressive to the players of opposing faction. This is ensured along with set FACTION_TEMPLATE_FLAG_PVP (0x800) flag in some templates.

So, try to ignore the present change to Creature::UpdateEntry() and to modify creature_template instead changing FactionXX fields in the following way: 28->85, 126->877, 12->11, 55->57, 104->105, 80->79, 125->85, 124->79, 118->71 (data for the racial leaders also). And plz do not forget to clean the client cache before testing.

In this scheme setting UNIT_FLAG_PVP in the DB has no effect. Strangely. But the TC335 code contains exactly the same patch, and this is a good reason to trust it.

@brotalnia
Copy link
Contributor

Changing faction may be a solution for some NPCs, but there are factions without a faction template that has FACTION_TEMPLATE_FLAG_PVP.

The Night Watch for example:
https://youtu.be/4aWrUYgGe84?t=2m30s
https://youtu.be/rS0rjcQ6YMc?t=41s
https://youtu.be/ZfxATDICoNE?t=3m32s
https://youtu.be/ZkiT0FotDow?t=1m15s

How to fix it in this case? There are only 2 faction templates for faction The Night Watch (49), and both 53 and 56 don't mark the NPC as PvP. They don't show as PvP in TrinityCore too, yet in all retail videos they are flagged as PvP. I'll report this to them too. Seems pretty obvious to me that taking only the faction into account is wrong. Why would the unitflag even exist if it doesn't do anything and only the faction matters.

@Olion17
Copy link
Contributor Author

Olion17 commented Jun 27, 2017

Why would the unitflag even exist if it doesn't do anything and only the faction matters.

This I do know :) You're free to set it ingame anytime, since UpdateEntry() is called just once at unit init and shouldn't be called any later (someone, remove that ancient EventAI interface to it plz). The flag is just a bit in the dword kept in the DB, while (most of) other bits are efficient.

So, it looks possible that such mobs (without PvP enabled faction template) were scripted on the retail, their relations were defined by the scripts. An "internal" blizz correction to the DBC looks possible the same.

Though wrong faction template can have consequnces, I agree with you in general. Maybe the best way is:

  • change the faction templates in the DB whenever it is possible to find a PvP enabled one;
  • do not drop the PvP unitflag if set in the DB for a wrong template, printing diagnostics instead.

@billy1arm
Copy link
Member

@Olion17 @brotalnia - Thank you

@billy1arm billy1arm merged commit 6da4dcf into mangoszero:master Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants