Devs: objects/UUID compare problems (inner rollbacks problem)

[JayDi85]

Continue from #4402

You can't compare UUID or objects by == operand. Need to use only obj.equals().

IntelliJ IDEA have inspections on that problems (use custom inspect settings to filter only needed data):

Current project have 72 warnings with == on objects (exported list to web page here):

BUT some xmage objects do not overrides Equals method and use default one, e.g. compare objects by == (compare refs, not data) .

As example, Player objects do not have Equals. All players data searched from game's state object:

But game state can be changed and reverted. In particular, it's saved to history every step by COPY (e.g. new state's players is not equals to old one). And on rollback step can broke something if you save ref object to somewhere (is that can cause of "buggy rollback" function?) or if you have another thread (is xmage use one thread per game?):

I don't known confirmed bug with that issue like with static filters (#4402). But devs have to be careful about this.

Howto fix that:

• Enable IDE warnings, find and replace == to equals for xmage and UUID objects compare;
• Add to each needed object overrided equals method. Example for PlayerImp.java (players with same ID will be equal):

@Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }

        if (o == null || getClass() != o.getClass()) {
            return false;
        }

        PlayerImpl obj = (PlayerImpl) o;
        if (this.getId() == null || obj.getId() == null) {
            return false;
        }

        return this.getId().equals(obj.getId());
    }

[LevelX2]

Good findings! 👍

[ingmargoudt]

Very good finding!!

A use the project Lombok jar often, which is a library that creates constructors, getters, setters and equal methods automatically for you with just an annotation

[Zzooouhh]

Interesting, does it impact OneShotEffects too (since they're presumably reverted along with the rollback), or is only a problem for continuous effects in practice?

[JayDi85]

Restore/rollback works dangerous. Looked at code. Game save new state every steps (save a copy of current state with all data, triggers, effects, players, cards, etc).

On restore request it search needed state by index and delete all other after that state:

And replace all current state's data with old one. Old triggers copy too -- it's can't get current effects:

If something have ref to old state's data on that point then it will be broken (like another thread or code execute after restore point).

P.S. O_o Found that restore/rollback used in targets process -- if was wrong target then all state restored O_o It's may be very buggy and broke the game even without rollbacks (if that "rollback" doesn't work correct):

And many more places like payOrRollback abilities:

[JayDi85]

@Zzooouhh yes, replacement bugs from restore/rollback function. That's it. It use save state before cast and then restored after cast (e.g. all data checks after restore will be dangerous to use by refs):

[JayDi85]

Another bad thing with restore (I think) -- on wrong ability it's try to restore AND after restore use old objects/ref to old state. But that old state was removed and inaccessible. Something bad may happing here (or may not -- I don't know details of java VM and GC works). That's ok -- UUID are fine between states.

[JayDi85]

As a fast workaround it may be good idea do not remove all old state on restore, but moved it to temp object (e.g. ensure that all old refs will works to the end). Maybe debugger or JVM have feature to find out all refs to that object after it was moved to temp -- that's can help to find wrong code.

[LevelX2]

Normally Players or MageObjects won't be checked directly.
They are compared by their UUID and if equals is used for the ID it works as intended.

So I don't see that bugy anywhere. But indeed we have to replace == checks on the UUIDs with equals.

[LevelX2]

Are all "==" replaced and this can be closed?

[JayDi85]

Well, I've checked and fixed warning from IDE about == problems in c24ba74. It's can be closed. But devs must check static analyser warnings time by time.

[JayDi85]

Found real life proof (#5784) of that problem:

[JayDi85]

Another one example of rollback and game state problem (#5835 with buyback ability -- a few bugs have allowed this ability to work):

• xmage uses copies of spells/abilities to play in many places, not original objects;
  
• some objects can store inner data/statuses;
  
• some objects forget to make copy and uses one object ref in all instances (e.g. same object in all game states -- that's ok for const objects):
  
• some objects can change inner data and that's data can be used outside of that objects:
  
  
• that's wrong -- you must use game state to store dynamic and shared data not inner objects: