Skip to content

Switch to JOML vectors. #7959

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

Open
wants to merge 4 commits into
base: dev/feature
Choose a base branch
from

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Jun 18, 2025

Problem

In the pursuit of a Bukkit-less Skript, Vector is a rather large thorn in the side. It's used all over the place in many places that have no relation to Bukkit at all. It's also not exactly the more versatile or optimized implementation of vectors. Since Minecraft now provides JOML at no cost to us, it seems prudent to swap to relying on JOML vectors instead.

JOML vectors also open up some new optimization opportunities as they allow for more efficient operations compared to Bukkit's.

Solution

Replaces nearly every instance of Vector with Vector3d. The vector classinfo is now backed by Vector3d.
Vector remains in Bukkit-specific areas where it would be silly to try to force in Vector3d just to swap it back to Vector every time it's needed, but Vector should no longer be passed between syntaxes.

No additional optimizations have been made to existing implementation to help make this PR easier to review. It should be a 1:1 replacement as of now. Despite that, a small performance improvement of around 5-10% has been seen when testing basic vector operations.

I have added converters between Vector3d and Vector to help back-compatibility with addons, though that likely won't help a lot since any use of %vector% will now produce a Vector3d instead of the expected Vector.

Vectors stored in variables will be deserialized as a Vector3d without issue, and vice versa if the user ever goes back to to a version prior to this PR.

It's very likely I've missed a spot or two and I would appreciate anyone willing to test this.

Testing Completed

No new tests added.

Supporting Information

This should target 2.13 at the earliest.

Breaking changes:

  • The 'vector' classinfo is now backed by Vector3d, meaning any addon who uses a %vector% input expecting it to be a Vector will need to re-write their code.
  • The should be no user-facing breaking changes, only addon-facing ones.

Completes: none
Related: none

@sovdeeth sovdeeth requested a review from a team as a code owner June 18, 2025 05:51
@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Jun 18, 2025
@sovdeeth sovdeeth requested review from cheeezburga and erenkarakal and removed request for a team June 18, 2025 05:51
@skriptlang-automation skriptlang-automation bot added the needs reviews A PR that needs additional reviews label Jun 18, 2025
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

looks good

@sovdeeth sovdeeth requested a review from Efnilite June 19, 2025 20:01
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Looks good

@skriptlang-automation skriptlang-automation bot added feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed needs reviews A PR that needs additional reviews labels Jun 25, 2025
@sovdeeth sovdeeth added the don't merge me !! For pull requests that should not be merged due to some outstanding dispute, conflict or dependency. label Jun 29, 2025
@sovdeeth sovdeeth moved this to In Review in 2.13 Release Jul 15, 2025
@sovdeeth sovdeeth removed the don't merge me !! For pull requests that should not be merged due to some outstanding dispute, conflict or dependency. label Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) 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. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls.
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants