-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Remove spectator players' cameras when teleporting #13478
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.
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
base: main
Are you sure you want to change the base?
Conversation
paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java
Outdated
Show resolved
Hide resolved
| // Attempt to set the camera first | ||
| Entity camera = this.getHandle().getCamera(); | ||
| this.getHandle().setCamera(null); | ||
| if (camera != this.getHandle() && camera == this.getHandle().getCamera()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an event changes the camera to a different new entity the teleport will still be broken, not sure but I think for any spectated entity you should cancel the teleport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this can just be replaced with the this.getHandle().setCamera(this.getHandle());
i test for example the cancel teleport event and is not broken if you update the camera.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll change that to check for any entity.
i think this can just be replaced with the this.getHandle().setCamera(this.getHandle()); i test for example the cancel teleport event and is not broken if you update the camera.
Would that not still change the camera even if the teleport is cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, how can the event change the camera? PlayerStopSpectatingEntityEvent is immutable, except for being cancellable. I'll update the check regardless but I don't think it will meaningfully change the behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll change that to check for any entity.
i think this can just be replaced with the this.getHandle().setCamera(this.getHandle()); i test for example the cancel teleport event and is not broken if you update the camera.
Would that not still change the camera even if the teleport is cancelled?
In teory but maybe the cancel state already manage that, like say you can test that behaviour but i dont find issue in cancel with the setCamera called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: Doc94@0661918
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: Doc94@0661918
This breaks for me:
- Cancelling PlayerStopSpectatingEntityEvent lets the broken teleport happen
- Cancelling PlayerTeleportEvent kicks me off the entity I'm spectating without teleporting me anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: Doc94@0661918
This breaks for me:
- Cancelling PlayerStopSpectatingEntityEvent lets the broken teleport happen
- Cancelling PlayerTeleportEvent kicks me off the entity I'm spectating without teleporting me anywhere
that two then also happen using teleport command and mess with events too no?
for the PlayerTeleportEvent make sense because first dismount/cancel-spect and then teleport then i feel its expected
for PlayerStopSpectatingEntityEvent then can be good check if this and then teleport (like if the camera really change later or make the camera return a bool)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a very good point actually, I hadn't considered that. I don't think that behaviour makes much sense regardless of where it's coming from - potentially another paper bug? ServerPlayer#teleportTo has a setCamera param which does the same as this PR but doesn't check for event cancellation, would it be worth me patching that too while I'm there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm im calling the expert in teleportations thing here... @Owen1212055
but i think just fix the api method with the camera can be ok?
This fixes #13473 by mirroring vanilla behaviour.