-
Notifications
You must be signed in to change notification settings - Fork 858
Fix Dargo's sacrifice cost #14162
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
Fix Dargo's sacrifice cost #14162
Conversation
xenohedron
left a comment
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 looks fine, using common classes is good, but I am confused as to what the problem actually was
|
@xenohedron I am still somewhat confused myself on the problem. But happy to provide what I did work out. The main issue is that Dargo was sometimes being discounted to just R when there had not been a sacrifice that turn (this was an AI match, not sure if relevant). I added some temporary debug messages to where the sac map is incremented and noticed that i seemed to get SACRIFICED_PERMANENT events when i was not expecting (e.g. on casting something), causing the value in the sac map to be incorrect. To replicate:
While looking into this I also noticed a secondary issue, using the rollback function does not reset the sacrifices for that turn. So on a turn where Dargo costs 6R, if the cost is reduced via sacrifices to R, then the turn is rolled back, he will still cost R without having to sacrifice anything. Hope that clarifies things somewhat, |
|
Interesting. The AI does simulate the game state, which effectively involves firing various events and rolling back, and those simulations do get caught by print statements. You can wrap in So you confirm that you can no longer reproduce those bugs with the code in this PR? |
|
@xenohedron Thanks for the suggestion. I believe I may have found the culprit, all of the increases were related to simulated events, the problem was that the Yes, i can confirm :) |
|
@xenohedron looks like there are many buggy places with same code style (wrong usage of static fields). Created related issue: #14249 |
|
Oh good catch, that certainly explains it |
Hopefully addresses #12349 + #11443
as well as part of #7750
The previous implementation would suddenly add large discounts on casting a creature/artifact, didn't seem to handle rollbacks, among other issues.
fix #12349, fix #11443