-
Notifications
You must be signed in to change notification settings - Fork 177
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
[script] [combat-trainer] Don't face/engage when using Innocence #5792
base: main
Are you sure you want to change the base?
[script] [combat-trainer] Don't face/engage when using Innocence #5792
Conversation
Adds a new `is_engaging_allowed?` gamestate which is honored by preventing any action that would require a `face` or `engage`. This prevents empaths from breaking Innocence (e.g. when manipulating and/or using GS)
combat-trainer.lic
Outdated
@@ -2507,7 +2508,7 @@ class TrainerProcess | |||
waitrt? | |||
fix_standing | |||
when /^Tactics$/i | |||
bput($tactics_actions.sample, 'roundtime', 'There is nothing else', 'Face what', 'You must be closer', 'You must be standing', 'Strangely, you don\'t feel like fighting right now', 'flying too high for you to attack') unless game_state.npcs.empty? | |||
bput($tactics_actions.sample, 'roundtime', 'There is nothing else', 'Face what', 'You must be closer', 'You must be standing', 'Strangely, you don\'t feel like fighting right now', 'flying too high for you to attack') unless game_state.npcs.empty? || !game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with Innocence (I just read its entry on elanthipedia), so maybe this is a dumb question. Why would someone using Innocence also be training Tactics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fair, there are a couple "should never happen" examples here -- see also training Astrology, since Innocence is an empath signature spell. However, it also felt like is_engaging_allowed?
could potentially be broader than whether or not you have Innocence cast, so I tried to cover pretty much any case that would lead to a face
or engage
command being issued. If that doesn't make sense I could also see this being restricted to only cases that would actually happen for an empath using the spell.
combat-trainer.lic
Outdated
@@ -2230,6 +2230,7 @@ class AbilityProcess | |||
timer = game_state.cooldown_timers['Battle Cry'] | |||
return unless !timer || (Time.now - timer).to_i > @battle_cry_cooldown | |||
return unless game_state.npcs.length > 0 | |||
return unless game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Battle cries are Bardic. Would a Bard ever be using Innocence?
combat-trainer.lic
Outdated
@@ -2762,6 +2763,7 @@ class TrainerProcess | |||
return if game_state.npcs.empty? | |||
return if fail_count > @analyze_retry_count | |||
return if DRSkill.getxp('Tactics') >= @combat_training_abilities_target | |||
return unless game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with Innocence (I just read its entry on elanthipedia), so maybe this is a dumb question. Why would someone using Innocence also be using Analyze?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misconfiguration? It should be safe to return here regardless, as the two are mutually exclusive.
combat-trainer.lic
Outdated
@@ -3407,7 +3411,7 @@ class AttackProcess | |||
end | |||
|
|||
def dance(game_state) | |||
if game_state.npcs.empty? | |||
if game_state.npcs.empty? || !game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do dance actions break Innocence? If not, should this check be moved to line 3420?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the engage
on line 3416 will break Innocence. Any face
or engage
will. (But for whatever reason, support
won't, and implicitly facing the next target with a target
bob
etc won't...)
But dance commands like weave
and circle
require you to be at pole range or closer, which is incompatible with Innocence which keeps enemies entirely disengaged if it works.
combat-trainer.lic
Outdated
@@ -2567,7 +2568,7 @@ class TrainerProcess | |||
when /^Collect$/i | |||
game_state.sheath_whirlwind_offhand | |||
retreat | |||
put('engage') unless game_state.npcs.empty? || game_state.retreating? | |||
put('engage') unless game_state.npcs.empty? || game_state.retreating? || !game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about making a function on game_state (engage
?) that encapsulates this repeated set of conditionals? See lines 2621, 2812, 3358, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that. Also considered making is_engaging_allowed?
defined as "not cast Innocence and not retreating"
There are some cases where we'll completely skip some actions when retreating that we should be able to do while using Innocence, so they're not quite the same thing, but there is a lot of repeat/overlap.
Maybe it would make sense to also include game_state.npcs.empty?
in the check too. In that case, it doesn't hurt anything to issue a face
or engage
command but it's still pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also address a number of the other concerns. "Why would an empath..." "Why would someone using Innocence..."
Generalizes more to "prevent us from issuing a face
or engage
command when it would be either bad or wrong"
How about treating Innocence like we treat Absolution? Currently the |
`is_engaging_allowed?` is now `can_engage?` and covers Innocence, retreating, and whether there are enemies in the room.
That makes a lot of sense. When I'm defining the hunt I should know that I plan to be using Innocence and I don't want to be dancing to break it. |
Instead of checking that the Innocence spell is up, allows the hunt to be defined with an innocence-mode toggle.
`can_face?` strictly covers Innocence and no targets. `can_engage?` also includes retreating state. For battle cries and tactics where we only try to face, not engage, uses the less restrictive.
Any other thoughts here? |
If you can clean/resolve any pending comments, and if it's in a place to be re-reviewed, tag us for review? |
combat-trainer.lic
Outdated
@@ -3407,7 +3409,7 @@ class AttackProcess | |||
end | |||
|
|||
def dance(game_state) | |||
if game_state.npcs.empty? | |||
unless game_state.can_engage? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but I'd prefer to swap the two branches so we can use if
instead of unless
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry following up on this one. What do you mean? I'll fix the code and get this merged as there's interest from a couple others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if game_state.can_engage?
line 3415...
else
pause 1
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpherbig Is it good now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor comments
Co-authored-by: Robert Herbig <[email protected]>
combat-trainer.lic
Outdated
@@ -2762,6 +2763,7 @@ class TrainerProcess | |||
return if game_state.npcs.empty? | |||
return if fail_count > @analyze_retry_count | |||
return if DRSkill.getxp('Tactics') >= @combat_training_abilities_target | |||
return unless game_state.is_engaging_allowed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misconfiguration? It should be safe to return here regardless, as the two are mutually exclusive.
combat-trainer.lic
Outdated
@@ -3407,7 +3409,7 @@ class AttackProcess | |||
end | |||
|
|||
def dance(game_state) | |||
if game_state.npcs.empty? | |||
unless game_state.can_engage? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpherbig Is it good now?
I'll smoke test this today and if no errors, will merge. |
Adds a new
is_engaging_allowed?
gamestate which is honored by preventing any action that would require aface
orengage
. This prevents empaths from breaking Innocence (e.g. when manipulating and/or using GS)