jasper-rietrae2 / SAI-Editor

Editor for TrinityCore's SAI (SmartAI / smart_scripts) database scripting system
GNU General Public License v3.0
46 stars 69 forks source link

Wrong SMART_ACTION_SET_EVENT_PHASE, SMART_ACTION_INC_EVENT_PHASE, SMART_ACTION_RANDOM_PHASE & SMART_ACTION_RANDOM_PHASE_RANGE parameters #23

Open kelno opened 9 years ago

kelno commented 9 years ago

The tooltips and parameter names given by the editor for these actions suggest phaseMask are used when core-side those actions set phase indexes. More simply a creature can't be in multiple phases at once so it doesn't make sense to put a phase mask in a phase setter. More technically, SmartScript::mEventPhase is the creature current phase index. The smartAI phase related actions use SmartScript::SetPhase which set mEventPhase. Which is only accessed in IsInPhase, which uses mEventPhase as a phase index. (SmartScript.h::219).

Here is the db fix part I made if you want to use it :

UPDATE action_type_information
SET 
tooltip = "Set the creature's event phase to a new value", 
parameterString1 = "Phase index"
WHERE action_type = 22;

UPDATE action_type_information
SET tooltip = "Increment or decrement the creature's event phase"
WHERE action_type = 23;

UPDATE action_type_information
SET 
tooltip = "Set the creature's event phase to a new value", 
parameterString1 = "Phase index 1",
parameterString2 = "Phase index 2",
parameterString3 = "Phase index 3",
parameterString4 = "Phase index 4",
parameterString5 = "Phase index 5",
parameterString6 = "Phase index 6"
WHERE action_type = 30;

UPDATE action_type_information
SET 
tooltip = "Set the creature's event phase randomly between two indexes values",
parameterString1 = "Phase index 1",
parameterString2 = "Phase index 2"
WHERE action_type = 31;
JasperAppec commented 9 years ago

Hmm. I'm not entirely convinced yet that the event phase is not a mask (not able to check very throhoughly at the moment). https://github.com/TrinityCore/TrinityCore/blob/6.x/src/server/game/AI/SmartScripts/SmartScriptMgr.h#L178

kelno commented 9 years ago

Okay so, a smart event is using a phase mask while a creature using smart ai can be only in one phase at a time. This value is stored in SmartScript::mEventPhase. What SMART_ACTION_SET_EVENT_PHASE and other actions are doing is setting the parameter value in SmartScript::mEventPhase.

If you want to be be sure that mEventPhase is indeed an index and not a phase:

mEventPhase is then accessed by SmartScript in SmartScript::IsInPhase to check if the event should be played in current phase. The function is not documented but you can read it here : https://github.com/TrinityCore/TrinityCore/blob/6.x/src/server/game/AI/SmartScripts/SmartScript.h#L219 This is transforming mEventPhase into a phase mask then compare it to the given argument, so we clearly observe here that mEventPhase is storing a phase index.

I'm not sure how to make this clearer sorry :>

The first reason I found this is because one of my tester had troubles setting phases with SAI-Editor. If first thought there was a bug in the core because of this and made a pull request on TrinityCore before checking it more thoroughly and finding the actual problem. I think this wasn't noted before because up to phase index 2, the phase mask equals the phase index, so this wasn't a problem in almost every case since there are hardly any SmartScripts which use more than 2 phases.

JasperAppec commented 9 years ago

I'm really busy with school and work for the coming time, I will look into it later. Thanks!