-
Notifications
You must be signed in to change notification settings - Fork 24
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
Avoid putting the game in a broken state if invalid turbo settings are submitted for fire-assisted attack #2972
Avoid putting the game in a broken state if invalid turbo settings are submitted for fire-assisted attack #2972
Conversation
…e submitted for fire-assisted attack
This looks like a pretty simple fix. I figure we fire up a dev site for it and take a look. |
Looks good. If we can get testers on the dev site to replicate that behaviour, I'm happy to approve this as-is. |
Dev site is up at https://2967-invalid-turbo-swing-fire.cgolubi1.dev.buttonweavers.com/ui/ --- please take a look! |
Still looking for testers to give this a go. |
@blackshadowshade @cgolubi1 |
I've tested this as well -- including the case of fire overshooting on a power attack (with turbo). It looks good. |
Thank you for testing, both of you! |
@blackshadowshade what else does this need before you'd be good to merge it? |
Thank you for the reminder, merging now. |
This is the simplest possible fix for the root cause.
Background:
submitTurn
, and then get them out of the cache and apply them when we adjust fire dice.The reason we get games into a broken state, is simply that the fire-adjusted path has its own implementation of applying turbo values, and it doesn't check the return value from
set_turbo_sizes()
, such that ifset_turbo_sizes()
fails, we just plow on ahead, and importantly, we plow on ahead tosave_game()
, committing the broken state in the DB.This is the simplest possible fix --- it checks the return value, and tells the player if it failed. And it does work (i'm attaching some screencaps below), because if the player abandons the fire turndown, that abandons the cached turbo values, and the player can start over.
It's not the most user-friendly --- it would be marginally more friendly if we rejected the turbo values on the initial screen, rather than making the player back their attack out. However, i think we should stick with this, because:
BMAttackTrip::attacker_apply_turbo()
workflow where we optimistically apply the turbo changes on cloned dice just to test them. That's more complexity and more opportunity to introduce errors; since the player can rescue themselves with this fix, it doesn't seem worth it.[dev site: https://2967-invalid-turbo-swing-fire.cgolubi1.dev.buttonweavers.com/ui/ ]