Fixes to DynamicValue getMessage and toString patterns wrt effect strings

[jimga150]

@xenohedron @JayDi85 @Susucre @theelk801

Proposing that we establish a consistent pattern for how DynamicValue subclasses use getMessage(). Looking at some previous issues, and digging through the history, it looks like this isn't an issue of a few implementations missing an otherwise consistent pattern, rather the issue is that no consistent pattern seems to exist.

There's effects that dynamically generate text assuming that they will have to add the "for each" or "number of" connectors themselves, and some that don't--and there are DynamicValue classes that do the same. This leads to discrepancies like in #12587.

Right now, it seems like each DynamicValue has a toString() that returns (usually) either "X" or "1", where "X" indicates that the value should use "number of" phrasing (i.e. "+X/+X, where X is equal to the number of ..."), and "1" indicates that the value should use "for each" phrasing ("draws 1 card for each ....")

Some DynamicValue subclasses use a multiplier, which can be used or foregone, wherein if the multiplier is used, it implies "for each" usage, otherwise "number of" phrasing is implied.

My proposal:

• Effects should still control the exact phrasing, but that means that sometimes the same DynamicValue subclass can be used in a "for each" context AND a "number of" context (see CardTypesInGraveyardCount used in both [[Consuming Blob]] and [[Emrakul, the Promised End]]), which means that there needs to be a way for DynamicValue subclasses to cater to both.
• DynamicValue subclasses should just provide the message in a singular or plural context ("creatures you control" vs "creature you control"), giving maximum flexibility to the owning effect to craft the message.
  • I'm open to debate about this, since i'm also tempted to say the DynamicValue subclasses should actually own the "number of" and "for each" clauses, but there might be some swath of cards I'm missing that break that pattern such that it will be easier to give control to the effects. Thoughts?

Smaller things, assuming the above is implemented:

• I think that toString() should be removed from DynamicValue. It looks like its two uses, determining the phrasing and providing the existence of a multiplier, can be wrapped into a single multiplier owned by the DynamicValue class, that is defaulted to 1 (implying no multiplier)
• In the same vein, i also think that getSign() should be changed to just return if the multiplier is negative or not--then no subclass will need to re-implement it.

[github-actions[bot]]

Consuming Blob - (Gatherer) (Scryfall) (EDHREC)

{3}{G}{G} Creature — Ooze /+1 Consuming Blob's power is equal to the number of card types among cards in your graveyard and its toughness is equal to that number plus 1. At the beginning of your end step, create a green Ooze creature token with "This creature's power is equal to the number of card types among cards in your graveyard and its toughness is equal to that number plus 1."

Emrakul, the Promised End - (Gatherer) (Scryfall) (EDHREC)

{13} Legendary Creature — Eldrazi 13/13 This spell costs {1} less to cast for each card type among cards in your graveyard. When you cast this spell, you gain control of target opponent during that player's next turn. After that turn, that player takes an extra turn. Flying, trample, protection from instants

[JayDi85]

toString() methods for debug only. Real code must not depends on it. I know there are many places with same pattern (e.g. Ability->toString() in test framework) -- and it's not good.

As I remember -- it's impossible to use a shared dynamic value text (e.g. getMessage or toString) in rules generation due too many different patterns.

[xenohedron]

Thanks for writing this out! I'll respond later.

[ssk97]

I noticed in #12593 is that StaticValue's getMessage() returns "", so effects use toString() to get the actual value. Changing this behavior would break a lot of existing text generation.

[jimga150]

toString() methods for debug only. Real code must not depends on it. I know there are many places with same pattern (e.g. Ability->toString() in test framework) -- and it's not good.

Agree

As I remember -- it's impossible to use a shared dynamic value text (e.g. getMessage or toString) in rules generation due too many different patterns.

It sounds like you're thinking that 'DynamicValue' subclasses should just return the plurality and not the "number of"/"for each" clause?

[JayDi85]

It should like you're thinking that 'DynamicValue' subclasses should just return the plurality and not the "number of"/"for each" clause?

Yes. I think the parent code must decide about DynamicValue’s text usage. Like filters do.

Example:

• This spell costs {1} less to cast for each card type among cards in your graveyard
• Consuming Blob's power is equal to the number of card types among cards in your graveyard

Effect must decide about plural form. I don’t know the size of the “plural problem”. So just an idea… Looks like effect has full control of the plural form, so it can decide about form usage like dynamicValue.getSingleMessage or dynamicValue.getPluralMessage. If dynamic value uses filters inside then it can have constructor with two filters or text messages like new DynamicValue(singleFilter, pluralFilter) or new DynamicValue(filter, singleMessage, pluralMessage).

[jimga150]

thats what im thinking too. I'll wait on xenohedron to respond and then i might take a shot at this.

[xenohedron]

I think there are three main categories of dynamic value text - here are three examples using the same ArtifactYouControlCount.instance:

1. "for each" e.g. [[Cranial Plating]]
2. "equal to the number of" e.g. [[Cephalopod Sentry]]
3. ", where X is the number of" e.g [[Cranial Ram]]

(Is there another phrasing style that I'm missing? There are a few cards with "less than the number of" or "greater than the number of", but I'm pretty sure those all correspond to filters, not values.)

Somehow, the text generation chain needs to support these three options. They should be standardized so that any effect class can choose its preferred default, supported correctly by any arbitrary dynamic value, but can still override to another phrasing if needed without manually setting text.

Maybe have these three options be an enum in Mage.Constants like ValuePhrasing.EACH, ValuePhrasing.EQUAL, ValuePhrasing.X? Perhaps the DynamicValue could have a method getText(ValuePhrasing v), or another approach could be for the DynamicValue itself to have the multiple options (so instead of a single instance, three enum values, or a param in the constructor).

As far as I know, the text after "the number of" is going to be the same in cases 2 and 3. So I could be convinced that just two options is better, such as getText(boolean plural). But you'll still need to provide a method for distinguishing them both somewhere in the chain (see e.g. [[Corrupt]] vs [[Consuming Corruption]]) - and selecting the correct phrasing is easier when you have the context of the words preceding it - so that's why I think approaching it as three options could make sense.

I'll also add - the reason that the DynamicValue uses toString() returning "X" or "1" is that's how it can be used most frequently in the same place that you would concatenate the string representation of an int when generating effect text (as ssk97 mentioned, this is where StaticValue returns its value). That is - the existing toString() is definitely useful for a separate purpose from getMessage() and not just the phrasing style toggle; admittedly it doesn't help handle values that are used with more than one phrasing style. If you can ensure all necessary functionality is handled by dedicated methods, then yes, it would be ideal for toString() to be debug usage only. And any effect that handles DynamicValue text gen needs to get StaticValue text gen right (ideally this could be accomplished without requiring a cast or instanceof).

I'm happy for anyone to work on this but I will want to review to avoid introducing too many new text discrepancies (assuming the overall paradigm is improved, it doesn't have to be perfect of course).

If anyone has more examples of various dynamic value text that would be important to keep in mind for this rework, please share! Best to plan out as much as possible in advance, so that the effort spent on implementing the plan doesn't end up wasted.

[github-actions[bot]]

Cranial Plating - (Gatherer) (Scryfall) (EDHREC)

{2} Artifact — Equipment Equipped creature gets +1/+0 for each artifact you control. {B}{B}: Attach Cranial Plating to target creature you control. Equip {1}

Cephalopod Sentry - (Gatherer) (Scryfall) (EDHREC)

{2}{W}{U} Artifact Creature — Phyrexian Squid */5 Flying Cephalopod Sentry's power is equal to the number of artifacts you control.

Cranial Ram - (Gatherer) (Scryfall) (EDHREC)

{B}{R} Artifact — Equipment Living weapon (When this Equipment enters the battlefield, create a 0/0 black Phyrexian Germ creature token, then attach this to it.) Equipped creature gets +X/+1, where X is the number of artifacts you control. Equip {2}

Corrupt - (Gatherer) (Scryfall) (EDHREC)

{5}{B} Sorcery Corrupt deals damage to any target equal to the number of Swamps you control. You gain life equal to the damage dealt this way.

Consuming Corruption - (Gatherer) (Scryfall) (EDHREC)

{B}{B} Instant Consuming Corruption deals X damage to target creature or planeswalker and you gain X life, where X is the number of Swamps you control.

[jimga150]

I think that any way we slice it, the effect is going to need to get information from the DynamicValue in more places than just appending a single call to getMessage(), even if it has parameters. With that in mind, and considering that there might be more ways than those three to phrase things, i think having the "number of"/"equal to"/"X is" phrasing handled by individual effects would result in the least number of effects needing text overrides.

The tradeoff is a lot of duplicate code looking for a possible multiplier and phrasing it accordingly, but i think with your solution, that code will still exist, just without having to add those string literals.

Another way to handle this is to try and maximally encapsulate phrasing in generic DynamicValue phrasing generators, where actions and direct objects are passed into a function that then phrases the effect text one of a number of ways:

1. [action] [multiplier] [object] for each [value]
2. [action] [object] equal to the number of [value]
3. [action] X [object], where X is [multiplier] the number of [value]

We could add more to this as new cases are discovered. This could belong to DynamicValue--no rewriting for each subclass, because each subclass would provide [value] test, and each call to getMessage would provide the phrasing option, and the [action] and [object] strings, as well as possibly using a variable letter other than "X".

Cards like [[Rootwire Amalgam]] would require the effect to pass in its own usage of the "X" variable, with a conditional replacement of that letter in case it needs changing.

---

I also noticed [[Wernog, Rider's Chaplain]], which uses a "one plus X" kind of DynamicValue. If we wanted DynamicValue subclasses to support adding the "X is" phrase, we will want them to support stuff like this too.

I think this kind of exception means that most of the phrases are better left to the effect text generation rather than the DynamicValue subclasses all having to handle any degree of assumption about what order the sentence will be in, or what qualifiers the effect will need in the middle of the phrase.

[github-actions[bot]]

Rootwire Amalgam - (Gatherer) (Scryfall) (EDHREC)

{5} Artifact Creature — Golem 5/5 Prototype {1}{G} — 2/3 (You may cast this spell with different mana cost, color, and size. It keeps its abilities and types.) {3}{G}{G}, Sacrifice Rootwire Amalgam: Create an X/X colorless Golem artifact creature token, where X is three times Rootwire Amalgam's power. It gains haste until end of turn. Activate only as a sorcery.

Wernog, Rider's Chaplain - (Gatherer) (Scryfall) (EDHREC)

{W}{B} Legendary Creature — Human 1/2 When Wernog, Rider's Chaplain enters or leaves the battlefield, each opponent may investigate. Each opponent who doesn't loses 1 life. You investigate X times, where X is one plus the number of opponents who investigated this way. Friends forever (You can have two commanders if both have friends forever.)

[jimga150]

I'm happy for anyone to work on this but I will want to review to avoid introducing too many new text discrepancies (assuming the overall paradigm is improved, it doesn't have to be perfect of course).

Oh absolutely, this will hit so many effects. I'm thinking of trying to write tests to compare the text a card provides to its oracle test in scryfall as a part of this--possibly on a per-ability basis, just saying "is the ability text a substring of the scryfall oracle text"

[ssk97]

I'm happy for anyone to work on this but I will want to review to avoid introducing too many new text discrepancies (assuming the overall paradigm is improved, it doesn't have to be perfect of course).

Oh absolutely, this will hit so many effects. I'm thinking of trying to write tests to compare the text a card provides to its oracle test in scryfall as a part of this--possibly on a per-ability basis, just saying "is the ability text a substring of the scryfall oracle text"

Those tests already exist, under Mage.Verify.

[jimga150]

Interesting--i see what you're talking about (checkWrongAbilitiesText() in VerifyCardDataTest.java?) but i'm wondering why that test doesnt fail when encountering cards like in #12587.

[ssk97]

Interesting--i see what you're talking about (checkWrongAbilitiesText() in VerifyCardDataTest.java?) but i'm wondering why that test doesnt fail when encountering cards like in #12587.

It should, if you set the sets checked to "*". There's just so many text discrepencies that that check failing is not a fatal error the way most tests are.

[jimga150]

what lever is pulled to demote these tests to nonfatal? Maybe with the above changes we can start being more stringent about card test?

[ssk97]

what lever is pulled to demote these tests to nonfatal? Maybe with the above changes we can start being more stringent about card test?

When it fails it does warn instead of using assert calls. There's a lot more text discrepencies though. Maybe some sort of a "don't increase the count" check would work, not sure how to handle that, and periodic Oracle Text updates can make things worse. Trying to actually eliminate all of them would be a rather extreme amount of work at this stage. @xenohedron has been steadily going through them but there's so many more.

[JayDi85]

checkWrongAbilitiesText used for few things:

1. Fast check new sets for typo errors -- you can see wrong filters, controller, values and other typos in card's code by wrong card's text. That's why text generation must have more priority over setText usage;
2. Check fixes in rules generation -- if you change some rules generation then checkWrongAbilitiesText can be useful to check total stats (is your text fix improve or do worse).

P.S. Verify tests contains many disabled/warn tests cause it can show too much problems. You can look at @Ignored or warn / warningsList.add to find it (example: problems with implemented but missing cards in old sets, missing reprints, missing sets, wrong images settings, wrong token classes, etc).

[jimga150]

Yeah, i get the sense that trying to maintain 100% scryfall oracle text compliance would just not be worth the herculean effort. I might do a sweep and see if i can't squash what non-joke set warnings i see as a part of this effort, though.

Outside of that, I think i'll start a pass on implementing a fix to this with DynamicValue handling only plurality, I'll start with just fixing one DynamicValue subclass and all uses of it to see how it handles.

[jimga150]

Asking as i go - Would it be the end of the world if DynamicValue became an abstract class, forcing all specific implementations to extend it instead and be concrete classes?

[xenohedron]

I can see the benefit in changing DynamicValue from an interface to an abstract class, if adding string fields can reduce the boilerplate method overrides needed. Just need to be careful about mutability and copy constructors.

edit: but enum instances are very useful, and that would be a real loss here, so not sure I like it overall

[jimga150]

Just made a draft PR implementing this as a demo.

[xenohedron]

Regarding the verify test for text discrepancies:

• A year ago, there were ~2700 discrepancies. Today there are ~900. I've dealt with most of the easier fixes, so what remains tends to be fairly challenging to fix properly.
• Upcoming errata is going to introduce a lot more discrepancies.
• Someday xmage might be in a state where failing on text discrepancies is helpful behavior, but we are a long way from that. Most discrepancies that remain are cosmetic.
• What I do is I run the verify test for ability text only on all sets, and save it. Then periodically I run it again and check the local diff to catch regressions. I also check the diff every time I'm modifying text generation to guard against unintended consequences. You can do this yourself as well.
• A good chunk of the remaining discrepancies are dynamic value related. However, a lot of dynamic value related text is set manually, which masks the scope of the problem.
• I have some ideas for improving text generation overall and reducing the need to setText manually in lots of scenarios, but haven't had the time to dedicate to it since it's not really the highest priority.

[xenohedron]

I don't like the approach of requiring a multiplier for every dynamic value. We have the wrapper class MultipliedValue which I think is more appropriate.

Also similar decorator classes to keep in mind for text gen: IntPlusDynamicValue, AdditiveDynamicValue, etc.

[jimga150]

Yeah, i saw those, but those seem like examples of cases that should be wrapped in and handled by a common class rather than classes wrapping enums that modify the value--how that modification is done in language can vary depending on the value and the effect context.

In case we want to keep that pattern though, i could just go for implementing the paramaterizable getMessage() and see what comes up.

[xenohedron]

Sure, the language used can vary, but I expect the phrasing enum would allow for that to be handled even with a wrapper class (as something the effect can own and pass through to the wrapped value).

Do you have specific examples to support why you think your approach is strongest? None of the wrapper classes have that many usages, so I don't know about cluttering too much general infrastructure to cater to them.

[jimga150]

For the multiplier case, i thought there were cards that throw the multiplying clause earlier in the phrase, but i was wrong. For constant addition, there's cards like [[An-Havva Inn]] and [[Hellfire]], which interleave information about how the value is calculated with information about what the value is used for, versus [[Artillery Blast]], which is the more usual phrasing.

As i comb through, i'm thinking yeah maybe we just let the wrapper classes do what they've been doing and shift the phrasing load to the DynamicValue implementations for now. All of these cases can just be handled by custom effects and string overrides, and the point is to minimize the need for that, not eliminate it entirely.

[github-actions[bot]]

An-Havva Inn - (Gatherer) (Scryfall) (EDHREC)

{1}{G}{G} Sorcery You gain X plus 1 life, where X is the number of green creatures on the battlefield.

Hellfire - (Gatherer) (Scryfall) (EDHREC)

{2}{B}{B}{B} Sorcery Destroy all nonblack creatures. Hellfire deals X plus 3 damage to you, where X is the number of creatures that died this way.

Artillery Blast - (Gatherer) (Scryfall) (EDHREC)

{1}{W} Instant Domain — Artillery Blast deals X damage to target tapped creature, where X is 1 plus the number of basic land types among lands you control.

[xenohedron]

Interesting find, though I think the "X plus" wording is rare enough to just be manual text setting: https://scryfall.com/search?q=o%3A%22X+plus%22+-mana%3A{X}

[jimga150]

Updated PR with new strategy

[xenohedron]

Example of three different phrasings to support for the same effect: [[Flow of Ideas]], [[Cosmic Epiphany]], [[Lucid Dreams]]

(edit: Grim Flowering is more similar)

[github-actions[bot]]

Flow of Ideas - (Gatherer) (Scryfall) (EDHREC)

{5}{U} Sorcery Draw a card for each Island you control.

Cosmic Epiphany - (Gatherer) (Scryfall) (EDHREC)

{4}{U}{U} Sorcery Draw cards equal to the number of instant and sorcery cards in your graveyard.

Lucid Dreams - (Gatherer) (Scryfall) (EDHREC)

{3}{U}{U} Sorcery Draw X cards, where X is the number of card types among cards in your graveyard.

[xenohedron]

Another thing that's related to DynamicValues, it would be nice if there were a straightforward default getHint() method to return an appropriate ValueHint. The hint text would generally be the plural phrasing message.

[JayDi85]

The hint text would generally be the plural phrasing message.

Text in card hint and text in rules are different. Rules can have long description with all related conditions, but card hint must be short and easy to read. Example:

• rules: the number of card types among cards in your graveyard
• hint: card types in your graveyard: XXX