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

Re-enable the vector from location syntax #6180

Merged
merged 10 commits into from
Jan 1, 2024

Conversation

Moderocky
Copy link
Member

Description

Re-enables the vector from location syntax. This makes several changes.

The previous pattern was slow and bad because it put two inputs directly next to each other and had no literal "keywords" in the pattern.

Note: the original %vector% [to location] [in] %world% is an example of a nightmare pattern.
Not only can it have two inputs (%vector% and %world%) next to each other, but there is also a possibility for it to have no other content at all.
This is a worst-case scenario since it technically allows any two things that might be a vector or a world to go next to each other, but more importantly, it means the parser has to attempt this for everything.

Changes:

  1. The new patterns all have keywords and text separating the inputs.
  2. The toString now contains the whole pattern.
  3. There is a little test for the syntax.
  4. A yaw/pitch will be added to the location if either is provided.
    Previously, both a yaw and a pitch value had to be provided for them to be added, meaning you could specify with yaw 10 but it would not be set if the pitch input happened to be null.
    This has been changed so if either are specified it will set the yaw and pitch (since that is what the user intended to happen) but a zero-value will be substituted for null.
    If neither yaw nor pitch is provided it will return to the old behaviour.

Target Minecraft Versions: any
Requirements: none
Related Issues: none

@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Nov 11, 2023
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.

Minor syntax suggestions, looks great!

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Implementation looks good!, but I am not sure I see the point of this syntax...
Is this not what converters are for? I do see the benefit from being able to specify the yaw/pitch for the created location though. Not sure if that is enough justification

@sovdeeth sovdeeth added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.8 Targeting a 2.8.X version release labels Dec 30, 2023
@sovdeeth sovdeeth merged commit 27d1a30 into SkriptLang:dev/feature Jan 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release 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.

4 participants