PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
9.69k stars 2.26k forks source link

1.13: Unable to modify players swimming state #1328

Open j-rockwell opened 6 years ago

j-rockwell commented 6 years ago

What behavior is expected:

Setting a players swimming state to false should take them out of their swimming state. Cancelling the EntityToggleSwimEvent should cancel the swimming state change for an entity.

What behaviour is observed:

No change in state - Setting the player's swimming state to false on a PlayerMoveEvent will cause them to 'flicker' but it will not remove them from the state.

Steps/models to reproduce:

Cancel the EntityToggleSwimEvent or try setting the players swimming state.

Plugin list:

None

Paper build number:

Paper 129

Anything else:

If cancelled the EntityToggleGlideEvent is successfully stopped, if possible try replicating this functionality with swimming.

BillyGalbreath commented 6 years ago

I took a stab at this and I cant get the client to respond to flag 4 (the swimming flag) at all (event or no event).

However, I could get it to respond to flag 3 (sprinting flag). In the EntityToggleSwimEvent if I cancelled the event and then set sprinting to false then I could successfully cancel getting into swim mode with double tapping W key. It even blocked quick presses of the ctrl key. However, if I held down ctrl then it flickered a bit and then continued into swim mode.

While in swim mode after cancelling the event and setting sprinting to false, I verified both of these flags were set to false. So, there is some kind of desync between server and client when it comes to these flags.

I put a 1 tick repeating task on a test plugin that toggles the sprint flag on then off if the swimming flag is set to false. This test was 100% effective, it kept the server and client in sync with each other. However it's not a cure-all as it's obviously not valid fix. Just wanted to include this as something tried.

Here is the test plugin I have, in case anyone else wants to take a crack at this.

Just wanted to reiterate: I use the sprinting flag (3) because the client does not respond at all to the swimming flag (4).

public class TestPlugin extends JavaPlugin {
    @Override
    public void onEnable() {
        getServer().getPluginManager().registerEvents(new Listener() {
            @EventHandler
            public void onSwimToggled(EntityToggleSwimEvent event) {
                if (event.getEntityType() == EntityType.PLAYER && event.isSwimming()) {
                    Player player = (Player) event.getEntity();
                    player.setSprinting(true);
                    player.setSprinting(false);
                }
            }
        }, this);

        new BukkitRunnable() {
            public void run() {
                for (Player player : Bukkit.getOnlinePlayers()) {
                    if (!player.isSwimming()) {
                        player.setSprinting(true);
                        player.setSprinting(false);
                    }
                }
            }
        }.runTaskTimer(this, 1, 1);
    }
}
Laarryy commented 2 years ago

Is this issue still valid on 1.19.1? I'll write up a test plugin when I get a chance but if anyone's got something to easily test this, would appreciate confirmation.

lynxplay commented 2 years ago

From my initial testing, this still seems to be the case. We would most likely have to dive in deeper here and figure out why the client refuses to accept these.

Lulu13022002 commented 2 years ago

This is not only a clientside issue but also the server that spam the setSwimming method for example each tick the method updateSwimming is called

        if (this.isSwimming()) {
            this.setSwimming(this.isSprinting() && this.isInWater() && !this.isPassenger());
        } else {
            this.setSwimming(this.isSprinting() && this.isUnderWater() && !this.isPassenger() && this.level.getFluidState(this.blockPosition).is(FluidTags.WATER));
        }

This means even if a plugin set the flag or cancel the event the state will be revert the next tick A current workaround for the change: ground->swimming is to just set the sprinting flag to false But this cannot work for the other switch (swimming->ground), also this event spam in this case if it's cancelled

So a proper fix will get ride of this method and set the swimming state only when the condition is met But it's also a clientside issue coz i have played a little bit with that method and even when i remove this method, the swimming animation is played clientside. This method probably exists also clientside. So we should probably wait that mojang (hopefully) delegate all the logic serverside and avoid to run this method each tick.

MWHunter commented 2 years ago

The client does actually respond to swimming metadata, it is just overridden at the start of the client’s tick but swimming set by server allows the client to continue swimming in situations that the player wants to swim but can’t start swimming due to eyes not in water or another similar issue.

MWHunter commented 2 years ago

A desync is quite normal though, it desync’s even in vanilla as client and server run the same logic and hope they get the same result. There’s not much you can do to fix this desync other than spamming packets at the client.

Edit: ironically spamming packets at the client causes one desync of this method, as last sprinting and last swimming change on the client without also changing on the server due to packets and latency. Plus changing the variables server sided cause issues because latency.

Lulu13022002 commented 2 years ago

Yeah we should probably just deprecate theses methods for now (cancel of the event + setSwimming method). Thanks for the client explaination, i don't have a deobfuscated jar.