-
Notifications
You must be signed in to change notification settings - Fork 807
Keep track of perpetual mana cost changes for display #9323
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
base: master
Are you sure you want to change the base?
Keep track of perpetual mana cost changes for display #9323
Conversation
| } | ||
|
|
||
| public void updateManaCostForView() { | ||
| currentState.setManaCost(this.getManaCost()); |
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.
I'm pretty sure that breaks perpetual effects
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.
I haven't found any issues with it but that code is only called by "addChangedManaCost" and "removeChangedManaCost". I added this line for Thought Partition (not sure what other cards directly change the mana cost; maybe there's other cases I missed?)
I did test that "decrease mana cost by 1" and "set mana cost to 5" (applied in either order) resulted in a displayed mana cost of 4 which matches what the actual cast cost ends up being when you cast it.
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.
It messes with Card.getOriginalManaCost and in result, messes with Card.getManaCost
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.
Yes; I'm not sure why "getOriginalManaCost" uses CardState at all. Shouldn't "original" be a static value of the card itself? I haven't found any cases where this code doesn't result in the correct values but I see your point - I put that line in to fix constant-value perpetual effects but I think what you're saying is that the perpetual calculation routine should be checking the changedCardManaCost tree rather than updating the state manaCost value. I'll work on that.
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.
I have updated the code to not set the manaCost value here. Thanks!
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.
Anything else I can do here?
Currently perpetual changes to P/T are reflected in hand but mana cost changes are not. This change tracks perpetual mana cost reductions so they can be displayed in hand (and other zones).
Normally this just means reducing (or increasing) the generic mana cost value, but for spells with X in the cost decreasing the cost contributes to "X" so for those spells I have added the generic reduction as an additional mana symbol after the regular mana costs but with a yellow highlight. For example, a Fireball spell reduced by 2 mana would display as XR2 where the 2 is outlined in yellow. This should be fairly intuitive since the player sees it appear when they reduce the cost. I experimented with displaying it as XR-2 (which is what it says in the card text box) but that takes up another symbol's worth of room and looks funny. I tried adding "-" to the generic cost symbol but that was hard to read.
I have tested decreasing costs and increasing costs with regular spells, spells with both X and regular generic costs like X2B (Ingenious Mastery), spells that set the mana cost to a specific value (thought partition; this already worked but I didn't break it), and spells that have changed zones before and after cost reductions.
