Keep track of perpetual mana cost changes for display#9323
Keep track of perpetual mana cost changes for display#9323TrevorHayes wants to merge 4 commits intoCard-Forge:masterfrom
Conversation
| } | ||
|
|
||
| public void updateManaCostForView() { | ||
| currentState.setManaCost(this.getManaCost()); |
There was a problem hiding this comment.
I'm pretty sure that breaks perpetual effects
There was a problem hiding this comment.
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.
It messes with Card.getOriginalManaCost and in result, messes with Card.getManaCost
There was a problem hiding this comment.
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.
I have updated the code to not set the manaCost value here. Thanks!
There was a problem hiding this comment.
Anything else I can do here?
|
I Don't have any major problems with this, but i think it should still be an option for people to disable if they don't want it: 
|
| while (parser.hasNext()) { | ||
| final ManaCostShard shard = parser.next(); | ||
| if (shard != null && shard != ManaCostShard.GENERIC) { | ||
| if (shard.toString().equals("{X}")) { |
There was a problem hiding this comment.
this change shouldn't be needed imo
There was a problem hiding this comment.
Not sure what you mean; this is there so that X spells that have perpetual cost reductions can be displayed with them (as fireball example). Is there a better way to detect that a spell has an X in the cost?
| for (final StaticAbility stAb : getCard().getGame().getCardsInGame().get(getCard()).getStaticAbilities()) { | ||
| // for (final StaticAbility stAb : getStaticAbilities()) { // For some reason the current state sometimes doesn't have static abilities | ||
| // Only collect perpetual cost changes | ||
| if ("Card.Self".equals(stAb.getParam("ValidCard")) && "DBCleanup".equals(stAb.getParam("SubAbility"))) { |
There was a problem hiding this comment.
statics don't even have subabilities
There was a problem hiding this comment.
Not sure what you mean; both of these checks were required to avoid crashing on calls on some cards. Yes, some don't have the "SubAbility" parameter but the way the check is written, returning null is fine and evaluates to false.
There was a problem hiding this comment.
ok, let me phrase it this way - your whole PR works for exactly one card:
https://github.com/search?q=repo%3ACard-Forge%2Fforge+%2Fmode.*cost.*card.self.*dbcleanup%2F&type=code
and the only reason it does is because that's a scripting error
| // I don't know why refetching the card data like in this next line is necessary but in some cases (when the cost reduction | ||
| // wasn't added when the card was in the current zone, I think) the static abilities are missing. There's probably | ||
| // a better way to do this. | ||
| for (final StaticAbility stAb : getCard().getGame().getCardsInGame().get(getCard()).getStaticAbilities()) { |
There was a problem hiding this comment.
there is Game.getCardState
though it might not be a bug if it's called from LKI or something
Ah, I like that as a place to put it rather than the general settings. I'll have to track down the other perpetual changes that are already displayed and change that code as well, I suppose. |
4 participants
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.
