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

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 this issue Nov 10, 2014 · 3 comments

Comments

@kelno
Copy link

kelno commented Nov 10, 2014

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
Copy link
Collaborator

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
Copy link
Author

kelno commented Nov 16, 2014

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants