Trinitycore: Core/Creature: creature_addon is a really bad place for visibilityDistanceType field

[offl]

Description:

visibilityDistanceType field was added in https://github.com/TrinityCore/TrinityCore/pull/22891 to creature_addon and creature_template_addon and I'd like to describe why this place is bad for such field.

1. creature_addon is not a stable place for anything, it can be removed accidentally without knowing about it or creature guid can be changed but creature_addon.guid not. Pretty sure some of them were removed in past 2 years and should be added again
2. you have to create basically empty row with only visibilityDistanceType filled and it makes DB dirty
3. it makes working with DB harder because you need to copy this field from creature_template_addon to creature_addon every time you create new creature_addon row. Or you may forgot to do it
4. originally there's no visibilityDistanceType field, it's handled by different flags
5. you don't really need to add visibilityDistanceType only to certain guids. see 4, it's handled by flags and applied to creature entry
6. let's pretend we moved it somewhere. can we remove all those empty addons now? no, you can't because people may use both creature_addon and creature_template_addon for one entry with different auras, stand states and stuff so now you need to check if all fields in both tables are empty. Pretty rare usage of addons but anyway

Expected behaviour:

Move it to extra_flags or create column in creature_template

Branch(es): both
TC rev. hash/commit: c2a0b78a051ea3301d8769b2c11fae00e14fe095

[Shauren]

No. Stop dumping everything into one table

[offl]

There are two options, move it to flags (as it should be) or move it to a new column as more easy way

[Ovahlord]

or just add the creature_difficulty table already and implement the flags one by one instead of having flags extra blow up further

[offl]

They still have unit_flags & unit_flags2 & type_flags and more, I have no idea why they also have(had) creature_difficulty that holds all those flags + multiple ones they don't actually send
And someone here already told that this table is really confusing because all flags are messed up

Also I don't get what is distance type 'Tiny' and 'Small', they're not used and there are no flags in creature_difficulty for that.

And I discovered that tons of creatures have bytes2 = 0. 0 is sheathed weapons and is not the default and common sheath state. Were all these addons added just to add visibilityDistanceType? If so, it's way much worse than 'dumping everything in one table'. Addons are not designed to hold such fields at all

SELECT * FROM creature_template_addon WHERE visibilitydistancetype IN (3,4,5) LIMIT 10000

[dr-j]

This would not work with flags or bitmasks since visibility distance type is not a boolean ie can have different values from 0-4

[offl]

or just add the creature_difficulty table already and implement the flags one by one instead of having flags extra blow up further

In case you meant moving all flags into one table: they haven't moved every flag to that table, only certain + added more previously never-sended serverside flags. So you simply can't use it and you shouldn't use it simply because that table was created not as replacement of classic way of handling flags. They just pulled multiple different flags into one table for better perfomance, perhaps

I wish they pulled all flags but they didn't

[xvwyh]

they haven't moved every flag to that table

They haven't moved any flag, there are no such serverside fields as "unit flags" or "type flags". unit_flags was created solely to pass certain flags to client that can change at runtime. type_flags was created to pass flags to client's cache that cannot change at runtime. Both these fields include ONLY the flags that client cares about. They didn't want to send absolutely everything to the client, so they made separate flag types that only selectively contained the flags that the client needed to know about. Flags like "unkillable" aren't stored in them, because the client doesn't need it, it doesn't affect how the unit is displayed or interacted with by the players, only the server needs it to prevent the HP from reaching 0. Neither is "sessile", because it's the server's responsibility to prevent the creature from starting any movement. Etc.

Same applies to most DBCs - many of them contain more columns and much, much more rows on the server, but they selectively choose which columns and rows to put into DBCs (the ones that are necessary for the client to function), both to hide the data from dataminers, and to reduce filesize/memory usage.

added more previously never-sended serverside flags

They didn't add anything either. "CreatureDifficultyFlags" or, as they should be called, "creature static flags" are THE flags that are and have always been used for everything creature related. They existed since the very beginning, the first 20 of them or so are in alpha's .pdb along with their WowEdit descriptions, all the ones that existed at that moment.

So you simply can't use it and you shouldn't use it simply because that table was created not as replacement of classic way of handling flags. They just pulled multiple different flags into one table for better perfomance, perhaps

Wrong. You can and should use it.

The reason they were moved from Creature record into CreatureDifficulty record is because blizzard wanted to have enemies in different instance difficulties have different flags. They don't need to be called anything "difficulty".

Initially they were making copies of every Creature record for every difficulty it should differ in. That was a huge waste, because a lot of data was being copied that didn't need to be copied, because it was never changing between difficulties, leading to issues like when a designer wanted to change, let's say, creature's name, they would have to change it in multiple records at once, for every difficulty. Cumbersome, hard to maintain, error prone. So they selected the fields that were the ones most commonly changed between difficulties (scripts, flags, loot, health/armor/damage multiplies, and probably a lot more, even if most of them weren't included in the DBC) and extracted them into a separate CreatureDifficulty record. Now Creature record only contains the data that's the same for all difficulties, and CreatureDifficulty contains only the data that can be different between difficulties.

|Can change at runtime|Needed by client|Where do they store it|
|---|---|---|
|No|No|CreatureStaticFlags inside CreatureDifficulty record, not sent to client|
|No|Yes|CreatureStaticFlags [...], sent to client inside Creature cache (`type_flags`)|
|Yes|No|Initial value from CreatureStaticFlags [...] (if the initial value can even be changed), not sent to client, stored in serverside a variable inside each unit|
|Yes|Yes|Initial value from CreatureStaticFlags [...] (if the initial value can even be changed), sent to client via update fields (`unit_flags`), stored on the server either in the same field that's sent to clients or in another variable and mirrored to update fields|

Not all flags that are in unit_flags come from CreatureStaticFlags. Flags like "in combat", "stunned", "feared" simply represent a runtime state of a unit, they don't have any initial value that could come from CreatureStaticFlags. They just got bundled together into the same field to save traffic. However, we shouldn't be setting such flags in our DB either: it makes zero sense to set "in combat" flag in creature_template. "Stunned" flag should be applied by auras, never from DB. And so on.

Not all flags that are in type_flags necessarily come from CreatureStaticFlags either. Most, I imagine, do, Maybe even they all do. But it is possible that DBC generator aggregates data from different records (for example from models) and bundles it all together into this single type_flags fields.

[offl]

added more previously never-sended serverside flags

They didn't add anything either. "CreatureDifficultyFlags" or, as they should be called, "creature static flags" are THE flags that are and have always been used for everything creature related. They existed since the very beginning, the first 20 of them or so are in alpha's .pdb along with their WowEdit descriptions, all the ones that existed at that moment.

Well, I didn't meant they created them from nothing
CreatureDifficultyFlags contains also levels and expansion fields, so the name CreatureDifficulty sounds more reasonable

So you simply can't use it and you shouldn't use it simply because that table was created not as replacement of classic way of handling flags. They just pulled multiple different flags into one table for better perfomance, perhaps

Wrong. You can and should use it.

The reason they were moved from Creature record into CreatureDifficulty record is because blizzard wanted to have enemies in different instance difficulties have different flags. They don't need to be called anything "difficulty".

Then why you need this in 335 or even 4xx? And how to use them and most important why?
There is a huge difference between our flags and their flags
I have a feeling not all flags (unit flags immune for example) are here. And I really doubt it's because that table is from MoP
You even don't need it in master because they stopped to use it

Tbh I don't see much difference between your explanation and my

[xvwyh]

CreatureDifficultyFlags contains also levels and expansion fields, so the name CreatureDifficulty sounds more reasonable

You mean CreatureDifficulty contains levels and expansion. Yes, because that's yet another data that they want to change between expansions.

Then why you need this in 335 or even 4xx?

Because even tho the data is sourced from a later expansion, most of the data for old creatures would still be valid. No all, of course, some surely got changed with later expansions, and especially with dungeon reworks, but it's still a great source of info that we shouldn't ignore. We did adapt to using BroadcastText, after all, even if it took a while, and even tho some of the data in it is too new.

And how to use them

Just like Ovahlord suggested - start loading them and start slowly reading flags from them instead of from creature_template and the like, one by one. For example, not so long ago TC added creature_movement table or whatever to control the flying/swimming/rooted flags, and until then we had InhabitType. They can all be ditched and instead be read from CreatureStaticFlags: Floating, Amphibious/Aquatic, Sessile. And voila, you no longer need to maintain your own table, now that data comes from the official source. Granted, some scripts might break and will need to be fixed, those that expect the creature to start in a state that they're not supposed to start from according to CreatureStaticFlags. But once you fix them - that's it, you won't have to ever bother with that issue again, because the data from CreatureStaticFlags is set in stone, and hopefully will never change, unless someone finds convincing proofs that the data changed in later expansions and needs to be "downgraded".

and most important why?

Because it's a source of blizzlike data, not something that we ourselves guessed/selectively sniffed/changed to suit our needs, and didn't even bother to leave a comment about it to future developers.

I'm not telling you to go and start using them right this instant. But it's something that we all should keep in our minds, we shouldn't ignore this well of knowledge. We should be checking it every time we want to make a change to confirm that it matches retail data. Or maybe even that it doesn't. And either reexamine the change we want to make, or try to find some info confirming that it got changed in a later expansion. And after some time, after we find many correlations, the desire to actually start using the data from that DBC will grow bigger and bigger, and, eventually, someone might start working on integrating that DBC into the server.

I have a feeling not all flags (unit flags immune for example) are here.

Huh? It's there:

https://github.com/TrinityCore/TrinityCore/blob/16b39a448acbe8ace88550a367be8e6bf565b00d/src/server/game/Entities/Creature/CreatureData.h#L41-L42

Or do you mean that not all creatures have it that should? That means they simply don't start in that state, immune flags can be applied or removed by scripts at runtime, after all.

You even don't need it in master because they stopped to use it

They were never using it in the first place, as far as I know. Flags are there for research/knowledge sharing reasons.

[offl]

Then why you need this in 335 or even 4xx?
>

Because even tho the data is sourced from a later expansion, most of the data for old creatures would still be valid. No all, of course, some surely got changed with later expansions, and especially with dungeon reworks, but it's still a great source of info that we shouldn't ignore. We did adapt to using BroadcastText, after all, even if it took a while, and even tho some of the data in it is too new.

BroadcastText were used since beginning or since TBC, my latest discovery: these Children's Week spells uses broadcast texts + we also know for a long time they're used in gameobject_template(again in TBC objects)

Ok, let me summ everything here because I simply can't understand some things:

Because it's a source of blizzlike data, not something that we ourselves guessed/selectively sniffed/changed to suit our needs, and didn't even bother to leave a comment about it to future developers.

They were never using it in the first place, as far as I know. Flags are there for research/knowledge sharing reasons.

The reason they were moved from Creature record into CreatureDifficulty record is because blizzard wanted to have enemies in different instance difficulties have different flags. They don't need to be called anything "difficulty".

"CreatureDifficultyFlags" or, as they should be called, "creature static flags" are THE flags that are and have always been used for everything creature related. They existed since the very beginning, the first 20 of them or so are in alpha's .pdb along with their WowEdit descriptions, all the ones that existed at that moment.

The last quote: do you mean they existed in that CreatureDifficulty or do you mean they simply existed? Or both?
With no doubt those flags existed since beginning.

BUT if they existed only as serverside flags without being added also in CreatureDifficulty, why we should use that system? Why we should pull everything in one table and create a huge mess if it's not even a way they handle those flags?

And how to use them
>

Just like Ovahlord suggested - start loading them and start slowly reading flags from them instead of from creature_template and the like, one by one.

So the same structure (flags1 flags2...)? Currently it's pita to check those flags, I mean I forgot to change Flags1 to Flags2 like 10times.

With all that info all I can say is: clean flags_extra(some of them may be wrong or useless), implement several known-what-does extra flags(unkillable, no corpse after death, prevent melee, 3 flags related to visibility and other simple flags). When you run out of flags, create extra_flags2 because it's exactly what Blizzard does.

[xvwyh]

BroadcastText were used since beginning or since TBC, my latest discovery: these Children's Week spells uses broadcast texts + we also know for a long time they're used in gameobject_template(again in TBC objects)

Since the beginning. All the texts in them appear in chronological order.

They were never using it in the first place, as far as I know. Flags are there for research/knowledge sharing reasons.

I was referring to the developers of master branch of TC here, not blizzard. Just to make it clear, because you bunched this quote up along with other quotes in which I was referring to blizzard.

The last quote: do you mean they existed in that CreatureDifficulty or do you mean they simply existed? Or both?
With no doubt those flags existed since beginning.

I mean they existed in Creature records from the very beginning and up until they created CreatureDifficulty records to avoid having to make a copy of the whole Creature record for every possible instance difficulty. And this was sorely needed, because the number of difficulties exploded at one point, now there 42 of them, as opposed to just 4 during WotLK. When CreatureDifficulty was created - flags were moved from Creature to CreatureDifficulty so that they can change them per-difficulty. But them being moved from one record to another doesn't change their meaning or their name. Hence CreatureStaticFlags. That's how they were most likely called in blizzard's corebase, given that in alpha's .pdb their names are contained in a global variable called g_creatureStaticFlags.

BUT if they existed only as serverside flags without being added also in CreatureDifficulty, why we should use that system?

I don't understand this sentence. Yes, they were serverside flags, but why shouldn't we use them now that we have access to them? We didn't before, and so we had to make do with what the server was giving to us - unit_flags, dynamic_flags, type_flags. But that data was woefully incomplete, and often incorrect (e.g. sniffed in the middle of combat). Flags from CreatureDifficulty dump are always correct, at least as correct as they appeared in retail DB at the moment of bruteforcing that data, but the usual caveats of it being updated to newer expansions apply.

Why we should pull everything in one table and create a huge mess if it's not even a way they handle those flags?

It is the way they handle whose flags. It is the field that the designers were toggling checkboxes for in WowEdit since the very beginning.

So the same structure (flags1 flags2...)?

Yes, of course.

Currently it's pita to check those flags, I mean I forgot to change Flags1 to Flags2 like 10times.

And that's why we need proper tools, like Truice. You're messing with numbers in the database at your own risk. Here's how I'm viewing those flags in my tool:

Also I can't understand why don't we use ENUM and SET types in DB instead of numbers. They're encoded as numbers and bitmasks under the hood anyway. That way you won't even need tools to avoid making mistakes:

Isn't this far better?

And as far as the code is concerned - it's entirely a non-issue, you can make overloads that will work with the proper field based on the parameters you provide:
https://github.com/TrinityCore/TrinityCore/blob/16b39a448acbe8ace88550a367be8e6bf565b00d/src/server/game/Spells/SpellInfo.h#L544-L558

[offl]

We are talking like we have extra_flags3 already and going to create extra_flags4
Meanwhile only 4 difficulty flags were implemented and all in wrong way. Extra_flags are not fully filled after 12+ years and amount of known NYI difficulty flags is 15, and to check all of them you need to spent tens of hours

From what I see you want to create temporary, not even used by them, added in MoP thing that changes a lot in development
What's wrong with extra_flags2 if they already created unit_flags2 and unit_flags3

[xvwyh]

I feel like I'm talking to a wall. Sure, keep reinventing your bicycles when you already have a perfectly fine one, and you just need to learn how to ride it. I'm done with this topic.