cmangos / issues

This repository is used as a centralized point for all issues regarding CMaNGOS.
180 stars 47 forks source link

DEPRECATED fields and functionality #2960

Open killerwife opened 2 years ago

killerwife commented 2 years ago

Feature Details

Currently we have many deprecated features we need to phase out over time. I will go over all of them, explain the alternative and the reasoning. I will be working myself on deprecating the stuff I mention but there is no chance of me getting it all done.

EAI:

Action ACTION_T_SET_UNIT_FIELD - not portable across expansions - use specific unit or npc field alterations instead. If you need any others, need to let me know.
Action ACTION_T_TEXT and ACTION_T_CHANCED_TEXT - superceded in functionality by ACTION_T_TEXT_NEW completely - unification
Action ACTION_T_COMBAT_MOVEMENT Param2 - already done by Action ACTION_T_AUTO_ATTACK by default and properly
Actions ACTION_T_CASTCREATUREGO and ACTION_T_CASTCREATUREGO_ALL - incorrect old paradigm where spell cast was somehow linked to awarding creature/go credit. Either a spell is meant to be used or ACTION_T_KILLED_MONSTER is to be used instead.

Dbscripts:

SCRIPT_COMMAND_FIELD_SET, SCRIPT_COMMAND_FLAG_SET, SCRIPT_COMMAND_FLAG_REMOVE - same as ACTION_T_SET_UNIT_FIELD

Pools and linking:

Superceded by spawn_group - if any functionality is not possible, let me know

script_waypoints:

Superceded by waypoint_path - important effort to minimize all SQL from core repo over time

script_texts:

Superceded by broadcast_text - same as script_waypoints

creature table: DONE!

gameobject table: DONE!

combat spellcasting:

ACTION_T_CAST in EAI, C++ casting without custom condition, creature_template_spells, creature_cooldowns - ACTION_T_CAST only deprecated to an extent(will not be removed) - creature_spell_list has same functionality and also unifies all these systems under umbrella described by official developers - also enables setting lists in EAI and dbscripts - if missing any functionality let me know

@cala @Grz3s @Tobschinski @AnonXS

Benefits and Purpose

Less copy paste and less newcomer confusion

Core Versions

miraco commented 2 years ago

For spawn_group / pooling https://github.com/cmangos/issues/issues/2604

Tobschinski commented 2 years ago

I think we are missing a way to chance spawns in creature- & gameobject_spawn_entry if you want to fully replace pooling.

killerwife commented 2 years ago

Ok then find an example where its actually needed and where spawn_group chancing cant be used.

Tobschinski commented 2 years ago

Well that depends on you guys, I have no problem with forcing every single creature and gameobject to be in a spawngroup, but .. I think I am the only one.

killerwife commented 2 years ago

I meant where you need chancing. 95% random entry does not need chancing

Tobschinski commented 2 years ago

Think of any rare spawn that has an alternative creature. Dark Iron Ambassador in Gnomeregan..

killerwife commented 2 years ago

Which is very few in the grand scheme of things :D

Grz3s commented 2 years ago

Lord Pythas in WC has few spawn points.. and every one is filled by another npc... if hes not there... old pool system couldnt handle that.

Tobschinski commented 2 years ago

Which is very few in the grand scheme of things :D

Don't argue against that but, I cannot guarantee you that any of these countless spider/wolf don't use it. But like I said, I'm down to use spawn-groups for everything, up to you.

AnonXS commented 2 years ago

creature_ai_events:

Action ACTION_T_SET_UNIT_FIELD - edit: no because some cases still need it (health, mana, npcflags, weapon) and these might then be converted to cpp, which i'm not a fan of it its not totally needed due to complexity.

Action ACTION_T_CHANCED_TEXT - some npc do actions and have chance to do a text doing that action, so overall no we should keep it, else we need to script it as spellscript...

2573002 25730 9 0 100 1025 0 5 16000 21000 0 0 11 26339 1 0 44 33 11563 0 0 0 0 0 En'kilah Necrolord - Say and Cast Saurfang's Rage - wotlk

Maybe convert ACTION_T_COMBAT_MOVEMENT Param2 into "Dont Turn" Value, as we've a hard time setting this currently in both EAI and DBScripts. We have 1 case of param2 being used currently so i'l remove that. Actions ACTION_T_CASTCREATUREGO and ACTION_T_CASTCREATUREGO_ALL - dont have much knowledge/usecases for them tbh. - ACTION_T_KILLED_MONSTER

('2089901','20899','9','0','100','1','0','500','0','0','0','0','21','0','1','0','20','0','0','0','0','0','0','0','Void Conduit - Prevent Combat Movement and Prevent Melee'), -- doubles up with 20 0

Then Renaming it ACTION_T_MOVEMENT_TURNING or something because setting it to 0 in param1 also does the same OUT of combat afaik or it was like that at some point.

but yea we should again try to gut as much scripting capabilities as we can that have shown themselves to not be blike so they can not be abused in the future to do nonsniff scripting, especially if they duplicate themselves with other script actions.

ACTION_T_CAST - no. due to what you wrote yourself.


dbscripts:

SCRIPT_COMMAND_FIELD_SET - yes SCRIPT_COMMAND_FLAG_SET - yes SCRIPT_COMMAND_FLAG_REMOVE - yes

ACTION_T_SET_UNIT_FIELD - yes as in EAI


pooling and linking:

I will be using pooling and creature_linking for cases i see spawn_groups inferior to the usecase.


script_waypoints:

Task for SD2 / Core people to offload ENTRY script_waypoints to ENTRY waypoint_path Scripts, that said should happen in form of improvements/rewrites.


script_texts:

Superceded by broadcast_text - scripts need to be revisited to exchange DoBroadcastText(); vs DoScriptText();


creature:

modelid yes (now handled by custom file for the edgecases), equipment_id - no, still too much todo.

As i said befor i'd like equipment_id to stay a little while longer as we've alot of conflicts there and we never got around to break down and remove creature_equip_template_raw yet so that should be first. modelid also poses some leftover issues that should make us reconsider blank out removing it.

curhealth, curmana - moved to creature_spawn_data_template - yes

DeathState - yes*, also be completely removed as not blizzlike - use feign death auras instead

why not nuke currentwaypoint, that is outdated too at this point?

creature_template:

in a first step to remove CLS stuff we should remove damage related columns that hold no real purpose as they are just empty in valid information and wrong all the time.

MinMeleeDmg MaxMeleeDmg MinRangedDmg MaxRangedDmg MeleeAttackPower RangedAttackPower RangedBaseAttackTime

miraco commented 2 years ago

I wanna leave this https://github.com/cmangos/issues/issues/2849 here too.

OnSpawnGroupDeath function. I found another example where we could use something like this.

Escort NPCs that spawn waves, and only continue their waypoints when these spawned npcs are dead. For example wailing caverns We currently do it like this https://github.com/cmangos/mangos-tbc/blob/master/src/game/AI/ScriptDevAI/scripts/kalimdor/wailing_caverns/wailing_cavernsScripts.cpp#L211

Grz3s commented 2 years ago

OnSpawnGroupDeath function. 👍🏻

cala commented 2 years ago

I'm totally pro reducing redundancy in the various system and removing deprecated ones.

Like @Grz3s and @Tobschinski I think that neither pool system nor spawn_group system can cover 100% of the cases. The question is: should they? Maybe for the few unique cases, we can also rely on C++/SD2 like we did in many places already. And I don't think Blizzard always created a whole new system everytime they had to handle specific cases, most of the time they prefered to twist existing ones (AQ40 and Naxx are good examples of twisted spells in Classic that became regular and clean systems in TBC).

Though, I agree that chancing in spawn_group system might be needed. For the example provided by @Tobschinski but also because some regular spawns outdoors are also on chance spawns. Furbolgs in Ashenvale or gnolls or kobolds in Elwynn Redridge are known to have their melee/casters versions sharing the same spawn points but with a higher probability for the melee version, hence the usual "kill the [insert melee name] to make the [insert caster name] spawn".

I agree with @AnonXS regarding ACTION_T_COMBAT_MOVEMENT : turning/facing has always been a source of headache and not only in ACID.

Not much to say regarding the other points.

killerwife commented 2 years ago

Well, I would like to see some statistical proof that shows the higher probability for the melee version lets say. Also spawn_groups can now basically handle all that I know of, and everything else fits into the worldstate variable control framework that I had to add. (and c++ can handle those in any way, shape or form)

ACTION_T_COMBAT_MOVEMENT only disables moving. To disable facing, SetTarget(nullptr) and SetCombatScriptStatus(true) is necessary, I havent introduced this mechanism to EAI yet, but I guess I have been using them so much in core scripts that its probably time, since I use them for in-combat scripts all the time.

miraco commented 4 months ago

Well, I would like to see some statistical proof that shows the higher probability for the melee version lets say. Also spawn_groups can now basically handle all that I know of, and everything else fits into the worldstate variable control framework that I had to add. (and c++ can handle those in any way, shape or form)

I may found something that we currently can not reproduce with using spawn_group (atleast not without using worldstate)

Shadow Labyrinth: WowClassic_MpD9CyyFRC

Group with 3 NPcs If the marked NPC (X) is an Acolyte, the other 2 gonna be both deathsworn. If the marked NPC (X) is an Deathsworn, the other 2 gonna be Acolyte There are atleast 2 groups with this behavior.