Skip to content
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.

Sign up for GitHub

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

[Don't Merge] Fix ability copying effects #12912

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[Don't Merge] Fix ability copying effects #12912

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
455d801
Create failed.txt
jimga150 Sep 23, 2024
b0d9847
Revert "Create failed.txt"
jimga150 Sep 25, 2024
5c2fbc2
Implement deterministic Ability ID generation in PermanentImpl.addAbi…
jimga150 Sep 25, 2024
f2e807b
Change ID generation to simple XOR and fix LSB/MSB swap
jimga150 Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Mage/src/main/java/mage/abilities/Ability.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,15 @@
public interface Ability extends Controllable, Serializable {

/**
* Assigns a new {@link java.util.UUID}
* Assigns a new random {@link java.util.UUID}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@JayDi85 JayDi85 Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@JayDi85 JayDi85 Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

*/
void newId();

/**
* Assigns a new {@link java.util.UUID}
*/
void newId(UUID newID);

/**
* Assigns a new {@link java.util.UUID}
*/
Expand Down
12 changes: 12 additions & 0 deletions Mage/src/main/java/mage/abilities/AbilityImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,18 @@ public void newId() {
}
}

@Override
public void newId(UUID newID) {
if (!(this instanceof MageSingleton)) {
this.id = newID;
}
getEffects().newId(newID);

for (Ability sub : getSubAbilities()) {
sub.newId();
}
}

@Override
public void newOriginalId() {
this.id = UUID.randomUUID();
Expand Down
2 changes: 2 additions & 0 deletions Mage/src/main/java/mage/abilities/effects/Effect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public interface Effect extends Serializable, Copyable<Effect> {

void newId();

void newId(UUID newID);

/**
* Some general behaviours for rule text handling: Rule text of effects get
* automatically a full stop "." at the end, if not another effect e.g. with
Expand Down
7 changes: 6 additions & 1 deletion Mage/src/main/java/mage/abilities/effects/EffectImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,13 @@ public TargetPointer getTargetPointer() {

@Override
public void newId() {
this.newId(UUID.randomUUID());
}

@Override
public void newId(UUID newID){
if (!(this instanceof MageSingleton)) {
this.id = UUID.randomUUID();
this.id = newID;
}
}

Expand Down
16 changes: 16 additions & 0 deletions Mage/src/main/java/mage/abilities/effects/Effects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
import mage.target.targetpointer.TargetPointer;
import mage.util.CardUtil;

import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.UUID;

/**
* @author BetaSteward_at_googlemail.com
Expand Down Expand Up @@ -185,6 +187,20 @@ public void newId() {
}
}

/**
* Generates new UUIDs for all effects deterministically, using seed ID.
* @param newID ID to use as seed for PRNG
*/
public void newId(UUID newID) {
SecureRandom numberGenerator = new SecureRandom(newID.toString().getBytes());
long msb, lsb;
for (Effect effect : this) {
msb = numberGenerator.nextLong();
lsb = numberGenerator.nextLong();
effect.newId(new UUID(msb, lsb));
}
}

public void setTargetPointer(TargetPointer targetPointer) {
if (targetPointer == null) {
return;
Expand Down
10 changes: 9 additions & 1 deletion Mage/src/main/java/mage/game/permanent/PermanentImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,15 @@ public Ability addAbility(Ability ability, UUID sourceId, Game game, boolean fro
// other abilities -- any amount of instances
if (!abilities.containsKey(ability.getId())) {
Ability copyAbility = ability.copy();
copyAbility.newId(); // needed so that source can get an ability multiple times (e.g. Raging Ravine)

// needed so that source can get an ability multiple times (e.g. Raging Ravine)
// Deterministic to allow repeated ability copies to have same UUID
// (see https://github.com/magefree/mage/issues/10372)
long msb = ability.getId().getMostSignificantBits() + getId().getMostSignificantBits() + 1;
long lsb = ability.getId().getLeastSignificantBits() + getId().getMostSignificantBits() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

copyAbility.newId(new UUID(msb, lsb));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// copyAbility.newId();

copyAbility.setControllerId(controllerId);
copyAbility.setSourceId(objectId);
// triggered abilities must be added to the state().triggers
Expand Down
5 changes: 5 additions & 0 deletions Mage/src/main/java/mage/game/stack/StackAbility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ public void newId() {
}
}

@Override
public void newId(UUID newID) {
this.ability.newId(newID);
}

@Override
public void newOriginalId() {
}
Expand Down
Loading