decrement encountersUntilSRChoice at end of combat, not start #1675
Conversation
|
This post from Malibu Stacey quotes cannonfire40 (a dev):
I chose to implement the counter using exactly the formula that cannonfire40 said was used by KoL itself. I am surprised that you chose to eliminate that. |
There was a problem hiding this comment.
Consider this:
You eliminated all "gets" of shadowRiftLastNC.
Therefore, you no longer need to set it.
Therefore, you no longer need the property.
Every "get" of shadowRiftTotalTurns is used in a calculation you removed.
Therefore, you no longer need to set it.
Therefore, you no longer need the property.
You have replaced cannonfire40's "KoL's internal" calculation and replaced it with something else.
I guess I am not surprised that the new implementation does not pass the existing tests, which use actual response texts from KoL to track the properties which this PR eliminates.
Since you eliminated both properties, you could remove the lines from the tests that test those properties, and look only at the "encountersUntilSRChoice" property.
I'd like to see a test that illustrates the reported issue and a formerly failed but works, with your fix.
| @@ -535,14 +534,12 @@ public static void handleShadowRiftFight(MonsterData monster) { | |||
| "shadow matrix" -> { | |||
| // Bosses are NCs | |||
| Preferences.setInteger("shadowRiftLastNC", currentTurns); | |||
| lastNC = currentTurns; | |||
| Preferences.resetToDefault("encountersUntilSRChoice"); | |||
| } | |||
| } | |||
|
|
|||
| // All fights - including bosses - advance turns in zone | |||
| currentTurns = Preferences.increment("shadowRiftTotalTurns", 1); | |||
There was a problem hiding this comment.
Since you threw away the calculation that uses currentTurns, you do not need to set currentTurns.
| @@ -525,7 +525,6 @@ private static String shadowLabyrinthChoiceDecision(String responseText) { | |||
|
|
|||
| public static void handleShadowRiftFight(MonsterData monster) { | |||
| int currentTurns = Preferences.getInteger("shadowRiftTotalTurns"); | |||
| int lastNC = Preferences.getInteger("shadowRiftLastNC"); | |||
There was a problem hiding this comment.
This eliminates the first "get" of shadowRiftLastNC.
| @@ -535,14 +534,12 @@ public static void handleShadowRiftFight(MonsterData monster) { | |||
| "shadow matrix" -> { | |||
| // Bosses are NCs | |||
| Preferences.setInteger("shadowRiftLastNC", currentTurns); | |||
There was a problem hiding this comment.
This is the first "set" of shadowRiftLastNC.
| @@ -553,13 +550,12 @@ public static void handleShadowRiftNC(int choice, String responseText) { | |||
| case 1499 -> { | |||
| // The Shadow Labyrinth | |||
| Preferences.setInteger("shadowRiftLastNC", currentTurns); | |||
There was a problem hiding this comment.
This is the second "set" of shadowRiftLastNC.
| } | ||
| case 1500 -> { | ||
| // Like a Loded Stone | ||
| ResultProcessor.removeItem(ItemPool.RUFUS_SHADOW_LODESTONE); | ||
| } | ||
| } | ||
| Preferences.setInteger("encountersUntilSRChoice", 11 - Math.max(currentTurns - lastNC, 0)); |
There was a problem hiding this comment.
Since you no longer use lastNC, you no longer need to get shadowRiftLastNC.
This PR eliminates all "gets" of shadowRiftLastNC.
Therefore, you no longer need either of the "sets".
Therefore, you no longer need the property.
|
I think PR #1676 does this with a much less invasive change. I suppose that DEBUG logs of ending a fight via Using the Force could also verify that the "end of fight" code was not running and the counters changing, but I had no such DEBUG logs available today. Maybe tomorrow. |
|
Looks like your PR is handling this in a much more elegant way--great work! I have no idea why I didn't think to just move the handleRufusCombat function to be end of combat instead of start, that's an extremely good solution. Are we able to use CLEESH or Macrometeor against bosses? Handling the NC reset at end of combat could be troublesome if we can, but I expect that as bosses they're immune to monster replacement. Either way, that's extremely surmountable With respect to this:
I don't currently see a meaningful difference between that and merely decrementing the property; the quote there seems to clarify that you can't "miss" a noncombat by hitting a wanderer or analogous disruption at the right moment. Decrementing the property and resetting it to 11 when you hit an appropriate NC should also replicate that behavior, but without the intermediary properties. But there could very well be something big that I'm missing here that makes these more different than I assumed. |
|
You have a point about "no meaningful difference". And it would be an elegant code change to eliminate the intermediate properties and just do it the way we do "encountersUntilNEPChoice", say. We have tests for "encountersUntilSRChoice". Should be a "simple matter of coding" to remove the lines from the tests that deal the intermediate properties, change to the simpler implementation of "encountersUntilSRChoice", and verify that the tests still pass. That could be a separate PR. |
This is failing tests right now. I need to get the two tests it's failing (and any potential other tests I add) to process the end of combat with all the monsters in question, but I don't immediately see a way to do that, so this is a draft for now.