Rework DynamicValue implementations #12596

jimga150 · 2024-07-23T00:58:06Z

This PR is to get the new pattern established with specific examples.

~~This will certainly not pass build, but it does demonstrate the solution I've landed on, and I'm presenting this for review before i continue.~~

I found it will generally be easier to go with the solution @xenohedron proposed here, since there are some values that need to be phrased as if they were an amount, rather than a count (power or toughness, versus creatures on the battlefield, for example).

~~The big implication of this is that in order to own a multiplier, DynamicValue had to become an abstract class, rather than an interface.~~

I plan to use this PR to continue implementing this, which will certainly hit every single subclass of DynamicValue as well as many effects that use this type, not to mention cleaning up the following:

~~1. Any use of toString() on a DynamicValue~~
~~2. All uses of .instance on a DynamicValue (since subclasses wont be a singleton anymore)~~

Many effects will need to be reworked to use this phrasing consistently, after which i don't think there will be that much need for effect text overriding just for the purpose of fixing a dynamic value's text.

Open to feedback.

This reverts commit 585dd63.

…multiplier behavior and maximize flexibilty with number of/for each phrasing

jimga150 · 2024-07-23T18:46:29Z

Skipped the writeup:

This new strategy just focuses on making sure all DynamicValue (now remains an interface) implementations own the phrasing and take that as an argument to getMessage()

xenohedron

some interesting ideas here. solving the boost text is a solid step, since that has the substantial complication of two dynamic/static values together.
I'd like to see an example for a more straightforward effect as well, like DrawCardSourceControllerEffect

Mage/src/main/java/mage/abilities/dynamicvalue/MultipliedValue.java

…ge based on whether the phrasing calls for it

…hoDrawCard argument, it's always just "you")

jimga150 · 2024-07-24T16:17:25Z

DrawCardSourceControllerEffect now makes use of an enum to switch between the following phrasings:

"draw cards equal to <value>"
"draw X cards, where X is equal to <value>
"draw <multiplier> card/cards for each <value>"

I made the "for each" phrasing the default, since scryfall indicated that that's the most popular phrasing.

xenohedron

make sure to look at the other decorator classes too, not just MultipliedValue

xenohedron · 2024-07-30T05:34:46Z

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

@@ -16,24 +15,38 @@
 */
 public class DrawCardSourceControllerEffect extends OneShotEffect {

+    public enum DrawCardsPhrasing {


I don't like this being nested here - seems like there should be one public enum for all usages

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

xenohedron · 2024-07-30T05:36:52Z

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

+        StringBuilder sb = new StringBuilder(youDraw ? "you " : "");
+        sb.append("draw");
+        String value = " a";
+        if (amount instanceof StaticValue) {


hm, I wonder if STATIC should be part of the enum, since it'll generally need its own text gen case and that way could just be part of the switch

Right now, detecting instances of StaticValue is already necessary in Effects when they need to call getValue, so this wouldnt significantly improve the text generation pattern. That is, unless there is a pattern that doesnt involve needing to call instanceof StaticValue?

It's ok to use instanceOf for popular and basic classes. Just make sure it do not break cards with extended class versions.

Mage/src/main/java/mage/game/command/emblems/ObNixilisOfTheBlackOathEmblem.java

xenohedron · 2024-07-30T05:41:32Z

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

+            value = " X";
+        } else if (drawCardsPhrasing == DrawCardsPhrasing.EQUAL_TO) {
+            value = "";
+        } else if (amount instanceof MultipliedValue) {


generally not a fan of these instanceof checks, but open to admitting that it's the least bad approach

This approach is probably going to lead to making a DecoratedValue abstract class (or interface?) for instance checking like this, since all the decorated values will need their qualification put into the text in this manner.

Another possible approach is to make decorator classes and StaticValue inherit a common class or interface (maybe still named DecoratedValue, though having StaticValue inherit that seems wrong to me) that has a getValue() function (or something to descriptively replace toString()) that goes in place here:
Draw <getValue()/""> cards <for each/where X is/equal to> getMessage()

Thoughts?

I think it should be possible to have the method in DynamicValue itself

On second thought, i'm not sure how to avoid the instanceof checks. Technically, some unique behavior could be implemented that identifies instances of MultipliedValue and StaticValue such that they can be handled in places like this, but i'm realizing two things that make this kinda pointless:

StaticValue, MultipliedValue, and the other DynamicValue implementations must all be handled differently (3 broad cases)

I can't think of any other cases than these three. the other decorator classes haven't necessitated property handling like 'MultipliedValue' has. AdditiveDynamicValue and IntPlusDynamicValue only need to modify their getMessage calls, and LimitedDynamicValue and LimitedDynamicValue are used in one place each, so trying to generalize text for them doesn't seem worth it.

So trying to avoid using instanceof will still land us handling these classes specially, at which point why not use instanceof

…cts need to reference when building oracle text

…enerator for SetBasePowerToughnessSourceEffect for use in AbominationOfLlanowar

…r DamageTargetEffect and static text for SetBasePowerToughnessPlusOneSourceEffect

…itedDynamicValue (used in one place each, so not worth tweaking text generators for)

JayDi85

Also need report with full rules stats in master and in your PR -- how many rules fixed in that PR.

VerifyCardDataTest.test_verifyCards with FULL_ABILITIES_CHECK_SET_CODES = "*"

JayDi85 · 2024-07-31T00:11:29Z

Mage/src/main/java/mage/abilities/dynamicvalue/AdditiveDynamicValue.java

-    public String getMessage() {
-        return this.dynamicValues.stream().map(DynamicValue::getMessage).collect(Collectors.joining(" "));
+    public String getMessage(EffectPhrasing phrasing) {
+        if (phrasing == EffectPhrasing.X_HIDDEN){


Use case for phrasing and throw exception on default (e.g. on unsupported)

Mage/src/main/java/mage/abilities/dynamicvalue/MultipliedValue.java

JayDi85 · 2024-07-31T00:16:49Z

Mage/src/main/java/mage/abilities/dynamicvalue/MultipliedValue.java

+//            return Integer.toString(multiplier);
+//        }
+//        StringBuilder sb = new StringBuilder();
+//        if (multiplier == 2) {


Do not keep commented code or add explanation about it in the first line like "old version" or "TODO: need check all used cards"

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java

JayDi85 · 2024-07-31T00:22:21Z

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

+        StringBuilder sb = new StringBuilder(youDraw ? "you " : "");
+        sb.append("draw");
+        String value = " a";
+        if (amount instanceof StaticValue) {


It's ok to use instanceOf for popular and basic classes. Just make sure it do not break cards with extended class versions.

JayDi85 · 2024-07-31T00:31:18Z

Mage/src/main/java/mage/util/CardUtil.java

+        } else if (toughness.getSign() < 0){
+            t = "-" + t;
+        }
+
        // sign fix for zero values


1
Looks like duplicated code for sign values above (new) and below (old). Must use one place to set sign text.

2
Also I don't like "else if" parts -- looks like it will be used for dynamic value only (but your comments about manual sign). Must move to static part instead?

//Non-static values will need their minus sign manually inserted

Looks like duplicated code for sign values above (new) and below (old). Must use one place to set sign text.

Not sure what the duplicate code youre referring to is. do you mean that within this function, there is more than one statement prepending a minus sign?

Also I don't like "else if" parts -- looks like it will be used for dynamic value only (but your comments about manual sign). Must move to static part instead?

That would be the opposite of the goal for this statement... It's an else if so that only non-StaticValue classes make it there, since they would already have their minus sign from the Integer.toString call. Is the ask to separate any handling of static and dynamic values in this function so they're not interleaved?

~~@JayDi85 I'm holding up moving forward with the bulk of implementations for this until we resolve this~~

Mage.Tests/src/test/java/org/mage/test/utils/BoostCountTest.java

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java

xenohedron · 2024-09-02T21:59:08Z

Mage/src/main/java/mage/abilities/effects/common/continuous/BoostAllEffect.java

@@ -98,6 +99,11 @@ public BoostAllEffect copy() {
        return new BoostAllEffect(this);
    }

+    public BoostAllEffect usingPhrasing(ValuePhrasing textPhrasing) {


withTextPhrasing for consistency

sorry to be nitpicky, but I think the standard is "with" not "using"

JayDi85 · 2024-09-03T00:59:44Z

Mage/src/main/java/mage/abilities/dynamicvalue/IntPlusDynamicValue.java

@@ -34,7 +44,6 @@ public IntPlusDynamicValue copy() {
        return new IntPlusDynamicValue(this);
    }

-    @Override


Looks like a typo, do not remove override from toString methods:

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java

JayDi85 · 2024-09-03T01:56:00Z

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java

+            sb.append("X");
+        } else if (textPhrasing == ValuePhrasing.EQUAL_TO) {
+            // do nothing
+        } else if (amount instanceof MultipliedValue) {


I don’t like that style of code:

it’s full of potential bugs and miss combinations (you combine phrasing param and class type in one switch instead split it);

it will be miss on new ValuePhrasing changes (no throw exception on unknown value);

it’s overdeveloped - instead using on two lines of code it required 10+ lines.

Old code:

New code from PR:

Must use switch with throw error all the time, not “if else”. Current code smells bad. I don’t look at other effects. But if it use/require same logic then whole thing must be rethink/rework.

P.S. Also look at legacy part - it miss empty check in new code. Is that fine?

The intricacies of templating require a lot of parameters to generate text correctly. It's not necessarily "overdeveloped".

I agree that a switch would be cleaner, but it is not obvious to me that would in fact be a workable solution.

Also:

the old code doesn't correctly generate text in many common scenarios

if getText method doesn't correctly generate text, it doesn't impact game logic or stability

any discrepancies are easily caught by the verify check

For possible ideas: I like a text template style for complex use cases instead multiple if/else (text replacing is slower, but much more easy to edit and support).

// prepare template
template = “some [VALUE] in the middle and [OTHER PARAM]…”;

// prepare params
realValue = xxx;
realParam = yyy;

// gen text
template.replace(“[VALUE]”, realValue);
template.replace(“[OTHER PARAM]”, realParam);

I think i see what you're saying here:

this needs to throw an exception for unsupported phrasings

if-else if chains are inherently less readable and less flexible

I took a shot at changing DamageTargetEffect and DrawCardSourceControllerEffect to use switch statements here. Right now those are the two effects that are being used as a test bed for this pattern.

What remains are the instanceof checks for MultipliedValue and StaticValue complicating such fragments. The base DynamicValue class /could/ be made to own states like being static or having a multiplier, but that would require making it into a class, which we've previously decided against.

If this is still too spaghetti, i'm open to ideas for how this should work instead.

For possible ideas: I like a text template style for complex use cases instead multiple if/else (text replacing is slower, but much more easy to edit and support).

// prepare template template = “some [VALUE] in the middle and [OTHER PARAM]…”;

// prepare params realValue = xxx; realParam = yyy;

// gen text template.replace(“[VALUE]”, realValue); template.replace(“[OTHER PARAM]”, realParam);

This and xenohedron's comments just showed up for me for some reason--I can get behind this too, but im curious to see if you think the switch statment works first.

but im curious to see if you think the switch statment works first

Yes, new switch is better:

Agree that the new switch is better. (Ideally you'd have one switch instead of two switches on that enum, but I'm not sure whether or not trying to combine them would be overall cleaner.)

That's a solid case for the templating idea from JayDi, now that i think about it.

JayDi85 · 2024-09-03T02:27:35Z

Mage/src/main/java/mage/abilities/effects/common/DrawCardSourceControllerEffect.java

@@ -28,10 +30,18 @@ public DrawCardSourceControllerEffect(DynamicValue amount) {
        this(amount, false);
    }

+    public DrawCardSourceControllerEffect(DynamicValue amount, ValuePhrasing textPhrasing) {


For info: I don’t know why devs ignore rules samples in text gen code but it much more easy to read and research with examples like that and/or real card refs:

jimga150 · 2024-09-03T02:44:56Z

Do not trust total stats. It can be false positive. If you fixed a text in one popular use case (1000 cards) but broke dozen other not popular use cases (100 cards) then total stats will be +900. And you will never to know about it by stats.

Possible solution: track text stats history after each rule/effect gen change.

I'm already running a complete oracle text check with VerifyCardDataTest with each change, diffing the errors found there with a run from the most recent ancestor commit on master. My understanding is that this runs checks on all cards implemented in Xmage right now in all sets, and catches any changes that resulted in new errors (or resolved existing ones).

Is this not the case? Or is the ask to add some verification in the branch so everyone can review the same check?

JayDi85 · 2024-09-03T02:54:30Z

Is this not the case? Or is the ask to add some verification in the branch so everyone can review the same check?

Yes. If you checked global stats after each your changes and no degrade found then all fine. No needs in branches and other. Just make sure your changes/replaces really work.

…ation

jimga150 · 2024-09-07T18:33:43Z

The templating is definitely better. Forces me to search for the variations on phrasings that dont necessarily have to do with the DynamicValue at all, like how both DrawCardSourceControllerEffect and DamageTargetEffect both can rearrange parts of their sentences sometimes.

jimga150 · 2024-09-19T06:18:21Z

@JayDi85 lemme know what you think of the most recent changes to DrawCardSourceControllerEffect and DamageTargetEffect. If the approach looks OK i will move on to implementing the new 'getMessage' for DynamicValues in stages.

JayDi85 · 2024-09-19T06:50:26Z

How many classes will require changes such as DrawCardSourceControllerEffect?

jimga150 · 2024-09-19T13:41:17Z

By the end of this whole rework, all effects that use DynamicValue (My search turns up 108, but there are probably more) will have their text generation adapted to the new phrasing scheme.

Right now however. no effects require changing for this to work. This change maintains backwards compatibility until all the work is done, at which point all effects will have the new phrasing scheme, where the default instance of each effect should generate the same text as its legacy phrasing (albeit using the new templating). Then, i can remove all the legacy text generation (this will be all uses of the ValuePhrasing.LEGACY enum) and remove the old getMessage() calls.

jimga150 · 2024-09-27T02:23:24Z

@JayDi85 Lemme know if that sounds acceptable.

Rework CreaturesYouControlCount wrt AngelicExaltation

585dd63

github-actions bot added cards engine labels Jul 23, 2024

jimga150 marked this pull request as draft July 23, 2024 00:58

xenohedron self-assigned this Jul 23, 2024

jimga150 added 2 commits July 22, 2024 23:04

Revert "Rework CreaturesYouControlCount wrt AngelicExaltation"

5295090

This reverts commit 585dd63.

Demo: Just shift phrase responsibility to DynamicValue while keeping …

2cedd4b

…multiplier behavior and maximize flexibilty with number of/for each phrasing

xenohedron reviewed Jul 24, 2024

View reviewed changes

Mage/src/main/java/mage/abilities/dynamicvalue/MultipliedValue.java Outdated Show resolved Hide resolved

jimga150 added 5 commits July 24, 2024 00:45

Use CardUtil for number to text functionality

d61d391

MultipliedValue now conditionally includes the multiplier in getMessa…

efff032

…ge based on whether the phrasing calls for it

Exclude StaticValues from being passed into MultipliedValue

46f081a

Update DrawCardSourceControllerEffect to use new DynamicValue pattern

16bfc76

Rework DrawCardSourceControllerEffect with new pattern (and fix the w…

b744f5f

…hoDrawCard argument, it's always just "you")

jimga150 changed the title ~~Rework CreaturesYouControlCount wrt AngelicExaltation~~ Rework DynamicValue implementations Jul 24, 2024

jimga150 mentioned this pull request Jul 24, 2024

Fixes to DynamicValue getMessage and toString patterns wrt effect strings #12595

Open

add getHint() to DynamicValue

d5cf40f

xenohedron reviewed Jul 30, 2024

View reviewed changes

jimga150 added 2 commits July 30, 2024 12:19

Add ValueHint() to MultipliedValue and StaticValue

03a77b4

Change phrasing enum scheme: DynamicValue now owns the enum that effe…

6dc02be

…cts need to reference when building oracle text

jimga150 mentioned this pull request Jul 30, 2024

Simplify DrawCardSourceControllerEffect text generation #12621

Merged

jimga150 added 5 commits July 30, 2024 14:29

Add X_HIDDEN enum

7eb2146

reorder SetBasePowerToughnessSourceEffect constructors

b5407c9

Add getMessage() for AdditiveDynamicValue and implement static text g…

76d31fd

…enerator for SetBasePowerToughnessSourceEffect for use in AbominationOfLlanowar

Implement getMessage() for IntPlusDynamicValue and adapt getText() fo…

0e30d7c

…r DamageTargetEffect and static text for SetBasePowerToughnessPlusOneSourceEffect

Implement getMessage() and getHint() for LockedInDynamicValue and Lim…

47d97d1

…itedDynamicValue (used in one place each, so not worth tweaking text generators for)

JayDi85 reviewed Jul 31, 2024

View reviewed changes

jimga150 added 2 commits July 30, 2024 21:19

use case statement for multiply handled getMessage

a9fe473

Fix exception message

acefe23

jimga150 added 6 commits September 2, 2024 14:06

clarify comment

f42c0d8

"and {this}'s toughness" -> "and its toughness"

0cfa56c

setStaticText can be private

de35246

BoostSourceEffect description -> sourceDescription

ca348b6

add @OverRide to legacy getMessaage() overrides

a85dfd9

update getValueHint of AdditiveDynamicValue

fd342c2

github-actions bot added the client label Sep 2, 2024

jimga150 added 2 commits September 2, 2024 14:47

put VerifyCardDataTest back the way it was

a07e8e7

fix some overzealous find/replace

d80c436

JayDi85 reviewed Sep 2, 2024

View reviewed changes

Mage.Tests/src/test/java/org/mage/test/utils/BoostCountTest.java Show resolved Hide resolved

JayDi85 reviewed Sep 2, 2024

View reviewed changes

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java Outdated Show resolved Hide resolved

Mage/src/main/java/mage/abilities/effects/common/DamageTargetEffect.java Show resolved Hide resolved

jimga150 added 2 commits September 2, 2024 15:51

add new getBoostCountAsStr tests

75703c3

phrasing -> textPhrasing

2608ac9

xenohedron reviewed Sep 2, 2024

View reviewed changes

usingPhrasing -> usingTextPhrasing

d70dc91

JayDi85 reviewed Sep 3, 2024

View reviewed changes

add back @OverRide to toString() calls

3150c14

jimga150 added 5 commits September 2, 2024 23:30

add FOR_EACH phrasing specs for cards that use them in updated effects

7372fe8

change new effect text generation to use switch statements

8c23606

usingTextPhrasing -> withTextPhrasing

a8b3406

Convert DamageTargetEffect to use templates for text generation

8bd6a8c

update DrawCardSourceControllerEffect to use templating in text gener…

05f43c7

…ation

Rework DynamicValue implementations #12596

Are you sure you want to change the base?

Rework DynamicValue implementations #12596

Conversation

jimga150 commented Jul 23, 2024 • edited Loading

jimga150 commented Jul 23, 2024

xenohedron left a comment

Choose a reason for hiding this comment

jimga150 commented Jul 24, 2024 • edited Loading

xenohedron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimga150 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayDi85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimga150 Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimga150 commented Sep 3, 2024

JayDi85 commented Sep 3, 2024

jimga150 commented Sep 7, 2024

jimga150 commented Sep 19, 2024 • edited Loading

JayDi85 commented Sep 19, 2024

jimga150 commented Sep 19, 2024

jimga150 commented Sep 27, 2024

jimga150 commented Jul 23, 2024 •

edited

Loading

jimga150 commented Jul 24, 2024 •

edited

Loading

jimga150 Jul 30, 2024 •

edited

Loading

jimga150 Aug 15, 2024 •

edited

Loading

jimga150 commented Sep 19, 2024 •

edited

Loading