Wynntils

Wynntils

611k Downloads

`WorldStateImpl` occasionally has wrong world state

kristofbolyai opened this issue ยท 37 comments

commented

During development I have found that WorldStateImpl#onServer returns false on multiple occasions, while connected to the server. This is likely related to the mixins that call the world state change events.

commented

Potentially related: having Litematica for Fabric installed stops Artemis from detecting world joins at all, even though the lobby and class selection triggers still work

EDIT: this is not actually the case, my bad

commented

Overall the UUID check has been holding up fine, so I'm a bit reluctant to see it go. If we can determine that there is just one additional alternative UUID I'd rather check for both, than to try to make some other heuristic to guess which of all player's name that would represent the world name.

commented

I think we should rewrite it to something like this:

        if (!onServer()) return;

        if (!e.getId().equals(WORLD_UUID) && !e.getId().equals(WORLD_UUID_2)) return;
        
        Component displayName = e.getDisplayName();
        String name = ComponentUtils.getUnformatted(displayName);
        Matcher m = WORLD_NAME.matcher(name);
        if (m.find()) {
            String worldName = m.group(1);
            setState(State.WORLD, worldName);
        } else {
            Reference.LOGGER.error("World name did not match pattern: " + name);
        }

and just try adding the second UUID you found.

commented

Do you remember which server you were logged onto? It might be different (but consistent) between servers? You did not play on beta or anything such?

commented

I switched to multiple servers, reconnected from the lobby. Nothing seemed to fix it, but I think there is a very specific reason this happens. I am working on #135 and had an exception that disconnected me, relogged and then all worlds had the different UUID.

commented

It was 3d524cd9-c75b-3410-b1da-de66593bd4f6 in my case, however I don't know if it is random or not. Until I restarted, it seemed consistent.

commented

Looking at the code, it seems the UUID is necessary to extract the world name.

commented

I don't really remember. I think I just traced packets that were sent and tried to look for suitable patterns to lock onto. What is the UUID in the failing cases? Is it a second, alternative UUID (which might carry some significance, perhaps?), or is it just random, or is it left out entirely?

commented

Can you extract the world name in the same way from that UUID instead? Maybe @HeyZeer0 knows why Wynncraft is using (at least) two different UUIDs to publish the world name?

I don't have the instance anymore, so I can't debug it now :c.

commented

Looking at the code, it seems the UUID is necessary to extract the world name.

How so?

    @SubscribeEvent
    public void update(PlayerDisplayNameChangeEvent e) {
        if (!onServer()) return;

        if (e.getId().equals(WORLD_UUID)) {
            Component displayName = e.getDisplayName();
            String name = ComponentUtils.getUnformatted(displayName);
            Matcher m = WORLD_NAME.matcher(name);
            if (m.find()) {
                String worldName = m.group(1);
                setState(State.WORLD, worldName);
            } else {
                WynntilsMod.error("World name did not match pattern: " + name);
            }
        }
    }

Can't we just remove the check (and use something to make sure we are on Wynn)?

commented

Can you extract the world name in the same way from that UUID instead? Maybe @HeyZeer0 knows why Wynncraft is using (at least) two different UUIDs to publish the world name?

commented

You need to make sure the PlayerDisplayNameChangeEvent is for the fake player representing the world to extract the display name. Otherways you'll get events for all logged in players and you can't say which of them is the world name.

commented

I see. I will try to come up with an alternative later.

commented

Sure, I can add it and see if it resolves this.

commented

Overall the UUID check has been holding up fine, so I'm a bit reluctant to see it go. If we can determine that there is just one additional alternative UUID I'd rather check for both, than to try to make some other heuristic to guess which of all player's name that would represent the world name.

we already have a heuristic though? the world name pattern is unique and could never be spoofed by a player ign or something. to me checking uuid doesn't seem necessary at all

commented

I have finally made progress on this bug! Sometimes, the events that we use to check world state (this case PlayerDisplayNameChangeEvent), does not have the correct UUID, so e.getId().equals(WORLD_UUID) fails.

According to git blame, @magicus wrote the WORLD_UUID code. Can you elaborate where the UUID checking came from and is it neccessary?

commented

Oh, I see now. Legacy has the same issue. Weird. I guess we should not depend on using UUIDs then. I will fix this bug, but I will wait for magicus before starting on it.

commented

The strangest thing is that only a full restart of the game fixes this issue, relogging does not. Weird.

commented

Are we sure this can't be spoofed? Is there not some new fancy way of paying players to set their name on Wynncraft?

commented

@P0keDev do you want to work on this or should I?

commented

you can handle it unless you really don't want to - i think you can just remove the uuid part and be good to go

this isn't the only way that the world state fails, however

commented

I will fix this issue then. How can it fail when UUID is correct?

commented

player names can't have spaces or formatting codes - the formatting is defined by wynn anyway, since they customize all the name packets for the layout of the tab menu

commented

the disconnect event will sometimes trigger when going from hub to world, making the mod think we're not even on wynncraft anymore

commented

Given what HeyZeer0 said about how bungeecord (which I think wynncraft uses) is a proxy, how does the disconnect event fire?

commented

i have no idea, and i don't think it really matters. the client disconnect code gets triggered and we need a client-side solution to handle it - we should start logging every connect event to see if we're getting reconnected to something new somehow

commented

ah

commented

I've run into problems with worldstate when I've been debugging my chat manager. I noticed the same here, the disconnect code seems to be called, why I don't know.

So with the current title of the bug, it seems to be still open. Or if this gets messy, we can close it and open a new. The cause is different but the result is the same.

commented

@kristofbolyai should this be closed now?

commented

@P0keDev mentioned a secondary bug with WorldState?

commented

Can you extract the world name in the same way from that UUID instead? Maybe @HeyZeer0 knows why Wynncraft is using (at least) two different UUIDs to publish the world name?

As far as my concerns go we use only one UUID for the world identifier, so you can definitely use it to keep track of which world the player is in. The reason there is always only one UUID is that it is generated based on the index of the tab list, since that's always the same, by result the UUID never changes as well.

Tho definitely let me know if there is something glitching this behavior, but it is most likely an edge case you guys are ignoring, as an example the tablist is only updated when the player character is successfully loaded (after you pick one)

commented

@HeyZeer0 It seems like we are not always seeing the same UUID. @kristofbolyai saw 3d524cd9-c75b-3410-b1da-de66593bd4f6. Is that something you can check were it came from?

If it's glitchy, it's unfortunately far too glitchy. :( If it glitched once in a million logins, I could argue we should ignore it, but it's every so often.

commented

@HeyZeer0 It seems like we are not always seeing the same UUID. @kristofbolyai saw 3d524cd9-c75b-3410-b1da-de66593bd4f6. Is that something you can check were it came from?

If it's glitchy, it's unfortunately far too glitchy. :( If it glitched once in a million logins, I could argue we should ignore it, but it's every so often.

Thing is it happened on every server and relogin, hubbing, switiching. Maybe it was the client somehow? Is that possible?

commented

100% Sure the UUID is changing? It doesn't make much sense for me to do so. The UUID generation is basically

int id = x * 100 + y;
UUID uuid = UUID.nameUUIDFromBytes(ByteBuffer.allocate(16).putInt(id).array());

Which for 2 as X and 1 as Y (the location of the global tag) always results in 16ff7452-714f-3752-b3cd-c3cb2068f6af

commented

This is the kind of bug that annoys me to no end. I mean, we can probably get around by just using pattern matching on the name, but the UUID should work and we don't understand why it does not. That kind of problem is often signifying an underlying issue that we are just sweeping under the rug, and that can come back and bite us at any time.

Otoh, we are a limited team with limited resources, and should perhaps not spend them on figuring out root causes for stuff that has viable workarounds...

commented

It pretty much looked like it. Obviously this is a rare bug, can't go back easily. Although, IMO it can easily happen that something happens client side, not sure why though. Do you think it's safe enough to assume the server name and format? (it obviously needs patching if it is changed, but otherwise)

commented

I believe this is fixed by #198. I'm actually so confident that I'm going to close this now. Please reopen if you ever see this problem again.