Slay the heart #16
Slay the heart #16
Conversation
(cherry picked from commit 3031093e62eb87399c940c136adeed7fe3b696b2)
(cherry picked from commit 68ca384e6097706b408553c0184492075253aebe)
(note: relies on a newer version of comm mod than what is currently in steam workshop)
(note: relies on a newer version of comm mod than what is currently in steam workshop)
… might be useful when hunting infinites later
…e heart and don't have it yet
…t is currently bugged ingame
…g within the strategy
xaved88
left a comment
xaved88
left a comment
There was a problem hiding this comment.
Very nice work! Really solid changes, and great test coverage!
I made a few comments - some are just small nit things to improve. There are two larger structural changes I propose, but I'm open to them not being handled in this PR.
| if amount: | ||
| self.player.block += amount | ||
| self.trigger_block_effects() | ||
| self.player.block = min(self.player.block, 990) # lower than 999 to avoid flailing against Beat of Death |
There was a problem hiding this comment.
for efficiencies sake we could put this at like 500?
There was a problem hiding this comment.
I like the thought but think I'd like to err on the side of it being as close to technically correct as possible and rely on other systems for not wasting our time too much instead of building in (more) intentional errors for efficiency
| return False | ||
|
|
||
| def handle(self, state: GameState) -> HandlerAction: | ||
| def handle(self, state: GameState, slay_heart: bool) -> HandlerAction: |
There was a problem hiding this comment.
So, this works for now, but I think we may want to adjust the approach here.
-
We're only telling it when it handles whether it should consider slaying the heart, but not on can_handle. So that means that we couldn't ever have a handler distinguish between whether or not it should be considered whether it's going for the heart or not. Ultimately, this means less reusable handlers.
-
It will be kind of ugly having this exist in both methods can_handle and handle of the handlers. Perhaps injection via constructor when we're making the handlers would be nicer. (Something to consider, not saying we have to make that change).
There was a problem hiding this comment.
Agreed on not making further changes for now - there are bigger changes we'd want to make here.
Might make a suggestion for people to use latest release to avoid too many issues with us changing stuff all the time.
rs/machine/state.py
Outdated
| # just the first letter of the key is enough | ||
| keys = [] | ||
| if "keys" not in self.game_state(): | ||
| return ['Communication Mod out of date'] |
There was a problem hiding this comment.
Will this cause a nice readable error to show up in the STS Mod or logs? We should probably be logging it ourselves as well in the bottled_ai logs for more visibility.
There was a problem hiding this comment.
It will not, and OK sounds good!
There was a problem hiding this comment.
After further discussion: I've removed the message from state.py and isolated that handling to logger.py
|
Review comments sufficiently addressed, now waiting for Comm Mod changes to be merged and released before merging on our end. |
2 participants
Everything the bot needs to go after the Corrupt Heart
Also:
Notes: