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

Some fixes for sai_display_rework #4

Merged
merged 5 commits into from
Dec 28, 2020
Merged

Some fixes for sai_display_rework #4

merged 5 commits into from
Dec 28, 2020

Conversation

funjoker
Copy link
Collaborator

@funjoker funjoker commented Feb 9, 2020

68d4dfa broke parameter loading instead of fixing it

9a5c8c3 Changing OutputType to Exe instead of WinExe caused the console to stay appeared all time, thinking about this, it only makes sense in DEBUG mode.
a62dff1 Makes the console disappear right on initialize if compiled in Release

There is no condition type SMART_ACTION in TC, so we can just hide this part in editParameter

{
"type": "BoolParameter",
"name": "UseTalkTarget",
"description": "Talker will target {target}"
}
],
"description": "{target}: Talk {pram1}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SAI is great, but it has a lot of edge cases and my goal was to support all of them as many as possible. For example, in case of SMART_ACTION_TALK (as far as I can see):

target == TARGET_NONE:
   Self: Talk {pram1} to invoker
target is player:
   Self: Talk {pram1} to {target}
useTalkTarget == true:
   Self: Talk {pram1} to {target}
useTalkTarget == false:
   {target}: Talk {pram1} to invoker

This is huge mess and in my option, it is a big mistake in TrinityCore, imagine writing it by hand in sql, pretty impossible to get desired result easily. What is more, this cannot be entirely moved to editor. Second condition target is player is dynamic, so in case of some targets, we don't know by the time of writing the script if the target will be player or not.

But we can implement description in other cases:

{targetid:choose(0):Self: Talk {pram1} to invoker|{pram3value:choose(0):{target}: Talk {pram1} to invoker|Self: Talk {pram1} to {target}}}```

}
],
"description": "{target}: Add quest {pram1}",
"description": "{target}: Offer/Add quest {pram1}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{target}: {pram2value:choose(0):Add quest {pram1} to quest log|Offer quest {pram1}}

},
{
"type": "Parameter",
"name": "TargetsLimit"
}
],
"description": "Self: Cast spell {pram1} on {target}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self: Cast spell {pram1} on{pram4value:choose(0):| random {pram4}} {target}

}
],
"description": "{target}: Set phase {pram1}",
"description": "{target}: Set/Remove phasegroup {pram1}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{target}: {pram2value:choose(0):Remove|Add} phasegroup {pram1}

{
"type": "Parameter",
"name": "Charges",
"description": "If 0 all charges are removed"
}
],
"description": "{target}: {pram1value:choose(0):Remove all auras|Remove aura due to spell {pram1}}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{target}: {pram1value:choose(0):Remove all auras|Remove{pram2value:choose(0):| {pram2} charges of} aura due to spell {pram1}}

"name_readable": "Invoker Cast",
"uses_target": true,
"target_is_source": true,
"implicit_source": false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, source is always invoker

@@ -78,6 +83,10 @@
"type": "BoolParameter",
"name": "Only to self",
"description": "True - only target will hear the sound, false - everyone in visibility range will hear the sound"
},
{
"type": "Paramter",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoolParameter

},
{
"type": "Paramter",
"name": "Distance"
}
],
"description": "{target}: Play sound {pram1} to {pram2value:choose(0):every player in visibility range of {target}|{target}}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{target}: Play {pram2value:choose(0):direct|distance} sound {pram1} to {pram2value:choose(0):every player in visibility range of {target}|{target}

},
{
"type": "Parameter",
"name": "ForeRespawnTimer"
}
],
"description": "{target}: Despawn {pram1value:choose(0):instantly|in {pram1} ms}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{target}: Despawn {pram1value:choose(0):instantly|in {pram1} ms}{pram2value:choose(0):| respawn in {pram2} seconds}

}
],
"description": "Plays randomly one of: {pram1}, {pram2}, {pram3}, {pram4}, {pram5} {pram6value:choose(0):of all|only self}",
"description": "Plays randomly one of: {pram1}, {pram2}, {pram3}, {pram4} {pram6value:choose(0):of all|only self}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They started using target, so

        "description": "{target}: Play random {pram6value:choose(0):direct|distance} sound one of: {pram1}, {pram2}, {pram3}, {pram4} {pram6value:choose(0):of all|only self}",
        "name_readable": "Play random sound",
        "uses_target": true,
        "target_is_source": true,
        "implicit_source": false

@funjoker
Copy link
Collaborator Author

actually i just realized...
This SmartActions are only valid for 335 and 434, because master is missing dynamic spawn system

How should we handle this ?

@pelekon
Copy link
Collaborator

pelekon commented Feb 11, 2020

actually i just realized...
This SmartActions are only valid for 335 and 434, because master is missing dynamic spawn system

How should we handle this ?

Tbh I have no idea for now, I didn't touch cata+ cores so I know nothing about dynamic spawn system and it's possible impact on SAI for TC(especially TC as I worked on authored, rewritten SAI). Maybe @BAndysc will want to think about it, I am too busy with work and diploma on uni.

}
],
"description": "{target}: Add quest {pram1}",
"description": "{target}: {pram2value:choose(0):Add quest {pram1} to quest log|Offer quest {pram1}}",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry, yesterday I made a mistake here, it is the opposite: choose(0) - offer quest, otherwise - add to questlog

{target}: {pram2value:choose(0):Offer quest {pram1}|Add quest {pram1} to quest log}

@BAndysc
Copy link
Owner

BAndysc commented Feb 12, 2020

actually i just realized...
This SmartActions are only valid for 335 and 434, because master is missing dynamic spawn system

How should we handle this ?

Actually, there are few more actions with different parameters for 335, 434 and 825. We might add tags field to the JSON to mark if action/event/target/whatever is strictly for some version. Then, in settings user will be able to choose what version he wants to work on. But this is a change to another PR.

I know nothing about dynamic spawn system and it's possible impact on SAI for TC

the impact is that those actions are not present on bfa TC :)

@BAndysc BAndysc force-pushed the sai_display_rework branch 2 times, most recently from 94f19ab to f0cc700 Compare February 12, 2020 21:48
@BAndysc BAndysc merged commit c1c8c5f into BAndysc:sai_display_rework Dec 28, 2020
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

Successfully merging this pull request may close these issues.

None yet

3 participants