Exopandora / ShoulderSurfing

Shoulder Surfing Reloaded is a highly configurable third person camera mod for minecraft.
MIT License
48 stars 9 forks source link

Ignoring rotation from vanilla packet `ClientboundPlayerPositionPacket` #203

Closed ZsoltMolnarrr closed 4 weeks ago

ZsoltMolnarrr commented 4 weeks ago

Description

Context

I am developing a new mod, which has a skill named "Shadowstep". It teleports the player behind its target, and also rotates the player towards the target (so the player can immediately perform an attack from behind).

Shoulder Surfing:

Problem

When the server teleports the player, using a vanilla ClientboundPlayerPositionPacket, the packet may contain rotation adjustment too. Rotation adjustment is always ignored by Shoulder Surfing camera.

Shadowstep with Vanilla 3rd person camera: https://github.com/Exopandora/ShoulderSurfing/assets/4989469/6120161b-153a-41e3-bfd6-e1b63287f2b2

Shadowstep with Shoulder Surfing 3rd person camera: https://github.com/Exopandora/ShoulderSurfing/assets/4989469/fe99dc64-8a3c-428f-a387-3c42791a1a89

Recommended fix

Add mixin into ClientPacketListener.handleMovePlayer to apply rotation to Shoulder Surfing camera. (Unfortunately I am unfamiliar with Shoulder Surfing internals)

Steps to Reproduce

I'm not sure if there is a vanilla game mechanic that can reproduce this, besides traveling through dimensions.

Minecraft version

1.20.1

Game Logs

https://mclo.gs/eT3aLU7

Additional Information

No response

Exopandora commented 4 weeks ago

Thanks for reporting. While working on some major refactorings lately (see 7128310), I also came across this issue. I wanted to correctly set the camera rotations after a teleport command. Unfortunately, this the problem is a bit more nuanced, as ClientPacketListener.handleMovePlayer is invoked in the following scenarios:

When in Shoulder Surfing perspective, the camera rotations should not reset when teleporting (or using an ender pearl) or (dis-)mounting, as it can be very disorienting. It is also not always possible to distinguish those cases.

As a possible fix, you could use the new Shoulder Surfing API and set the camera rotations manually, if thats possible in your case:

ShoulderSurfing.getInstance().getCamera().setXRot(xRot);
ShoulderSurfing.getInstance().getCamera().setYRot(yRot);

Note that this API has not been released yet. It will be available for Shoulder Surfing 4.0.0+. For now, you would need to access the implemention directly:

ShoulderRenderer.getInstance().setCameraXRot(xRot);
ShoulderRenderer.getInstance().setCameraYRot(yRot);
ZsoltMolnarrr commented 4 weeks ago

Thank you for the feedback.

As far as I can see after some investigation, I don't think using the API would feasible in my case. I sent a vanilla packet (by invoking vanilla teleport function) handled by vanilla client code. Sending a custom packet would be possible to adjust the camera angle additionally, however the position and custom angle adjusting packets would not be fully synchronized, likely resulting multiple camera adjustments under a short time.

I understand your concerns your regarding disorienting camera adjustments, and absolutely commend that 🫡, still I think it would be most optimal if Shoulder Surfing would handle the rotation inside the packet. So I have the following recommendations:

  1. If rotation data in the packet is only present at teleport cases (not at dismounting), adjusting the camera unconditionally will not cause districation (imo), since the position of the camera is changed by the teleport any way.
  2. If rotation is always passed by the server (so we cannot ever distinguish), how about adjusting camera rotation if the difference between current and received angles is larger than a certain (configurable) value (for example 20 degrees).

What do you think?

Exopandora commented 4 weeks ago

I implemented it now by checking for rotation changes. I also did this in initial experiments last week, but there I injected into some other teleport function which caused many issues. The only caveat now is that it also resets the camera rotations when changing dimensions, but I'm fine with that. Let me know if it works for your use case!

ZsoltMolnarrr commented 4 weeks ago

Hello! I tested the topmost commit of 1.20.1 branch. The few scenarios I was able to test it worked perfectly, including shadow step. :)

caveat now is that it also resets the camera rotations when changing

I believe instead of a caveat this is actually the correct behavior.

Thank you for the cooperation!