[Don't Merge] Fix ability copying effects

[jimga150]

Addresses #10372

This fix has some test failures that all are related to SetBasePowerToughnessSourceEffect:

case study: SetBasePTSourceTest.testSvogthos (tests the "becomes creature" effect of [[Svogthos, the Restless Tomb]])

Test fails because the creature it becomes dies immediately, since its Toughness is 0. This is because in ContinuousEffects.apply, the layeredEffects set is iterated over in no particular order (since its a set). This shouldn't matter, since effect order need only be implemented between layers, and all effects on the same sublayer should be able to be applied in any order.

However, in the original state, it just happened to be that the SetBasePowerToughnessSourceEffect of SvogthosToken applies AFTER the BecomesCreatureSourceEffect creating that token in this apply loop, so when state-based actions are checked, the toughness is seen as 5 (set by SetBasePowerToughnessSourceEffect). After i made my change, that order is reversed, so the SetBasePowerToughnessSourceEffect is overwritten by a replacement effect setting the toughness to 0.

Setting aside why it is that these orders were reversed in a loop iterating over a set, it looks like the problem is that BecomesCreatureSourceEffect sets the power and toughness on the same sublayer (7b, P/T setting effects), and are vulnerable to this race condition.

I'm not sure what the exact distinction is between layers 7a and 7b, and moving these effects between layers seems like a bad idea, so i'm inclined to say that BecomesCreatureSourceEffect should not be setting power and toughness at all in this scenario. A switch should be implemented that causes it to skip setting these values in anticipation of a different continuous effect (probably an ability of the creature) setting them.

[xenohedron]

I think it's specific to an effect making a source object into a creature and granting it what would normally be considered a characteristic defining ability - if I recall correctly Svogthos is the only example.

I'll look into it but I think the solution is to write a custom effect for Svogthos, so as not to have a step where it sets PT to 0. But setting PT is definitely needed for the general case and almost all other usages of the common class.

Edit: the reason it's not a CDA is (4) here, that's why both layer 7b (though interpreting it as 7a would make it always wrong instead of weirdly indeterminate)

604.3a. A static ability is a characteristic-defining ability if it meets the following criteria: (1) It defines an object's colors, subtypes, power, or toughness; (2) it is printed on the card it affects, it was granted to the token it affects by the effect that created the token, or it was acquired by the object it affects as the result of a copy effect or text-changing effect; (3) it does not directly affect the characteristics of any other objects; (4) it is not an ability that an object grants to itself; and (5) it does not set the values of such characteristics only if certain conditions are met.

And I don't understand why the timestamps are apparently out of order or changed by your code, I think that's the place to investigate.

[JayDi85]

For info: all non mtg code must be extracted to helper function like CardUtil, no needs to msb/lsb/whatever in game engine. There are already existing RandomUtil that use single random source. I recommend to use it for all random values around. That's sid/source can be used later for repeatable test games in LoadTests.

[JayDi85]

Test fails because the creature it becomes dies immediately, since its Toughness is 0. This is because in ContinuousEffects.apply, the layeredEffects set is iterated over in no particular order (since its a set)

Sounds buggy/bad or not fully researched. MTG rules allow PT < 0 in the middle of the effects calculations. Xmage uses same logic -- permanent will die from negative PT in state base actions (e.g. on next game cycle), not in the middle of the calculation.

[JayDi85]

Also xmage uses time based effect's order like paper rules (due effects adding timestamp). Maybe it can be bugged with dependency effects (if some effect depend on another then it must be applied later). Example: effect that checking card type must be applied after effect that change card type. Search for dependencyTypes and dependendToTypes

[jimga150]

The death doesn't happen in the middle of a calculation, it happens after the continuous effects were applied in the "wrong" order (massive quotes on "wrong" here because the order shouldn't matter). The problem is that there are both "set toughness to 0" and "set toughness to 5" effects ok the same sublayer affecting the same creature, resulting in what cosmetically could be compared to a race condition.

[jimga150]

Also xmage uses time based effect's order like paper rules (due effects adding timestamp). Maybe it can be bugged with dependency effects (if some effect depend on another then it must be applied later). Example: effect that checking card type must be applied after effect that change card type. Search for dependencyTypes and dependendToTypes

I'll check this out. Maybe my understanding of effects on the same sublayer is wrong

[JayDi85]

See details in rule 613.8:

613.8. Within a layer or sublayer, determining which order effects are applied in is sometimes done using a dependency system. If a dependency exists, it will override the timestamp system.

XMage support it but it require to manual setup (see DependencyType usage), so only few popular use cases implemented. Rare or new effects/combo can be miss.

[xenohedron]

The "set toughness to 5" effect should systematically always apply after the "set toughness to 0" effect (by timestamp) even with both in layer 7b. If that can't be easily fixed, then there's no good reason for the "set toughness to 0" effect to be here at all in this specific card application.

(Maybe there are underlying issues with dependencies but that's not directly related.)

[jeffwadsworth]

Just a bit of insider information here: the continuous effects code currently doesn't check the timestamps inside the layer 7 sublayers. The other layers are checked with a pretty hacky method that "mostly" works okay. But, if you have 2 or more 7b sublayers involved, they will not be timestamped correctly.

[jimga150]

Just a bit of insider information here: the continuous effects code currently doesn't check the timestamps inside the layer 7 sublayers. The other layers are checked with a pretty hacky method that "mostly" works okay. But, if you have 2 or more 7b sublayers involved, they will not be timestamped correctly.

So it sounds like i was right in terms of the implementation in XMage, but that this is out of sync with MTG rules that do care about ordering within the same sublayer.

So are all layer 7 effects just happening to work correctly due to the order they're added to the layer set?

[xenohedron]

Can the test framework pick up the original issue? If so, can you include a test for it?

Also, does this change eliminate the need for the Whipgrass Entangler workaround?

[xenohedron]

Can you explain how you came up with this algorithm?

Why "+1"?
Why add instead of bitwise XOR?
Why is lsb using msb from original id?

[jimga150]

Can the test framework pick up the original issue? If so, can you include a test for it?

I will add a test that verifies that the propaganda copying is fixed, once i land on a solution that works and survives scrutiny.

Also, does this change eliminate the need for the Whipgrass Entangler workaround?

In theory, it should (or more accurately, it will). I'll try removing that workaround and seeing if it still works as intended once i get the generalized solution complete.

Can you explain how you came up with this algorithm?

Why "+1"? Why add instead of bitwise XOR? Why is lsb using msb from original id?

I might change it to an XOR. The +1 was a change i made on a hunch that didn't work out, and the MSB/LSB mixup is a genuine issue it seems. This is all going to need to be moved to CardUtil anyways, so this is pending some refactoring.

[jimga150]

Found the issue. The reason the order is consistently swapped is that the order property of the effect, something that is quite well tracked, is used to sort the lists containing the layered effects (i was wrong earlier about the effects being tracked in unordered sets).

The way this order is determined is in ContinuousEffects.updateTimestamps, which takes a key and a list of layered effects. It compares the provided list to the layered effects previously stored at that key (or an empty list if this is the first time this key has been used), and adds any /new/ effects to the list at that key with a new, unique order number, tracked through the incrementing order property in ContinuousEffects.

Normally, when a new effect gets added, updateTimestamps sees it wasn't in the last call (since it has a new ID) and gives it a new order. With deterministic effect ID generation, however, this pattern is broken as soon an effect has to be re-checked. The re-generated effect has the same ID as the last time it was checked, so the list under the key checked with updateTimestamps assumes it already has its order set properly, and doesn't give it a new one. this leaves it with the default order number: 0.

The list of effects is sorted by this order, which is then used to apply those effects in.

I'm not sure how to fix this. I think a rewrite of these timestamps is in order?