Skip to content
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

Orientation Change Support for Move Event #5961

Merged

Conversation

NotSoDelayed
Copy link
Contributor

@NotSoDelayed NotSoDelayed commented Sep 1, 2023

Description

This PR adds support for orientation related to Skript’s current move event!
Example syntaxes:

on player rotate:
on sheep rotate:

Target Minecraft Versions: any
Requirements: 1.16 (non-player living entities)
Related Issues: -

@veeonthewall
Copy link

LGTM

@TheLimeGlass
Copy link
Collaborator

Event rotate is misleading, it's on any movement.

@sovdeeth
Copy link
Member

sovdeeth commented Sep 1, 2023

Rotate does not get called when the player rotates while moving, which it should be called for.
It does get called while stationary rotating, which is great.
Additionally, the bug where the first rotation after stopping movement also calling Move should probably be addressed in this PR, if possible.

@NotSoDelayed
Copy link
Contributor Author

I’ll sort out the issues when available.
IMO, there should be a pattern for when the player is moving and changing orientations, just as what PlayerMoveEvent is called for, and on player move: seems a perfect fit for this, but it is for sure a breaking change.

@sovdeeth
Copy link
Member

sovdeeth commented Sep 2, 2023

I’ll sort out the issues when available. IMO, there should be a pattern for when the player is moving and changing orientations, just as what PlayerMoveEvent is called for, and on player move: seems a perfect fit for this, but it is for sure a breaking change.

You could do:

on player move:
on player rotate:
on player move or rotate:

Maybe not the cleanest, but the simplest change for users imo

@NotSoDelayed
Copy link
Contributor Author

I guess that’s the best option to prevent a breaking change.

@NotSoDelayed NotSoDelayed marked this pull request as draft September 2, 2023 01:10
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 2, 2023
@NotSoDelayed NotSoDelayed changed the title New Event: Entity Rotation Head Rotation Support for Move Event Sep 2, 2023
@NotSoDelayed NotSoDelayed changed the title Head Rotation Support for Move Event Orientation Change Support for Move Event Sep 2, 2023
@NotSoDelayed NotSoDelayed marked this pull request as ready for review September 3, 2023 00:37
src/main/java/ch/njol/skript/events/EvtMove.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtMove.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/events/EvtMove.java Outdated Show resolved Hide resolved
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@Moderocky Moderocky changed the base branch from master to dev/feature September 22, 2023 11:19
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

src/main/java/ch/njol/skript/events/EvtMove.java Outdated Show resolved Hide resolved
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Nov 2, 2023
@Moderocky Moderocky merged commit 5189e34 into SkriptLang:dev/feature Nov 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants