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

[script] [combat-trainer] Don't face/engage when using Innocence #5792

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bishoprook
Copy link
Contributor

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)

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)
@bishoprook bishoprook changed the title Combat-trainer: don't face/engage with Innocence [script] [combat-trainer] Don't face/engage when using Innocence May 5, 2022
@MahtraDR MahtraDR requested a review from rpherbig May 6, 2022 00:07
@@ -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?
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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?
Copy link
Contributor

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?

@@ -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?
Copy link
Contributor

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?

Copy link
Collaborator

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.

@@ -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?
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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?
Copy link
Contributor

@rpherbig rpherbig May 6, 2022

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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"

@MahtraDR
Copy link
Collaborator

MahtraDR commented May 6, 2022

How about treating Innocence like we treat Absolution? Currently the undead argument triggers checks for Absolution, and prevents attacks without the spell cast. It seems consistent to keep that workflow? The it's a more deliberate action on the part of the user, setting the argument in their hunting_info section like we would construct or undead and not just "Oh, is Innocence up?"

`is_engaging_allowed?` is now `can_engage?` and covers Innocence, retreating, and whether there are enemies in the room.
@bishoprook
Copy link
Contributor Author

The it's a more deliberate action on the part of the user, setting the argument in their hunting_info section like we would construct or undead and not just "Oh, is Innocence up?"

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.
combat-trainer.lic Outdated Show resolved Hide resolved
combat-trainer.lic Outdated Show resolved Hide resolved
`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.
@bishoprook
Copy link
Contributor Author

Any other thoughts here?

@MahtraDR
Copy link
Collaborator

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?

@MahtraDR MahtraDR requested a review from rpherbig May 10, 2022 08:40
@@ -3407,7 +3409,7 @@ class AttackProcess
end

def dance(game_state)
if game_state.npcs.empty?
unless game_state.can_engage?
Copy link
Contributor

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

Copy link
Collaborator

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.

Copy link
Contributor

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

Copy link
Collaborator

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?

combat-trainer.lic Outdated Show resolved Hide resolved
Copy link
Contributor

@rpherbig rpherbig left a comment

Choose a reason for hiding this comment

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

Only minor comments

combat-trainer.lic Outdated Show resolved Hide resolved
@@ -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?
Copy link
Collaborator

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.

@@ -3407,7 +3409,7 @@ class AttackProcess
end

def dance(game_state)
if game_state.npcs.empty?
unless game_state.can_engage?
Copy link
Collaborator

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?

combat-trainer.lic Outdated Show resolved Hide resolved
@MahtraDR
Copy link
Collaborator

MahtraDR commented Jul 1, 2022

I'll smoke test this today and if no errors, will merge.

@Kiriawen
Copy link
Contributor

@MahtraDR Is this one still brewing or has it gone flat?

@MahtraDR
Copy link
Collaborator

@MahtraDR Is this one still brewing or has it gone flat?

I plan to merge this imminently, now that HE is over, I can actually test it. :D

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.

4 participants