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

feat(scripting/v8): support unpacking Lua vectors #3019

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thelindat
Copy link
Contributor

@thelindat thelindat commented Dec 19, 2024

Goal of this PR

Allows the JS runtime to unpack vectors received from Lua as arrays, rather than requiring manual handling of buffers.

However, I would like to add default Vector classes instead of using arrays if acceptable.

How is this PR achieving the goal

Adds a msgpackr extension for the vector types (20, 21, 22), dependant on #3018.

This PR applies to the following area(s)

ScRT: JS

Successfully tested on

Game builds: N/A

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

Supercedes #2946

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Dec 19, 2024
@Korioz
Copy link
Contributor

Korioz commented Dec 19, 2024

It will break my scripts and i don't think of any way to fix the breaking, my use case is : JS script receive data in any form or types from Lua (db documents) and use the current extension type 20, 21, 22 in the variable to check if the variable was a vector in Lua.

@thelindat
Copy link
Contributor Author

Refusing to support vectors because of backwards compatibility is stupid.

i don't think of any way to fix the breaking

Not having support for arrays or objects with xyz properties, strictly expecting vectors only, seems like a bad idea.
Check the object type, if it's an array or doesn't have the buffer and type properties then fallback to old behaviour.

@p1u3o
Copy link

p1u3o commented Dec 19, 2024

I would find this very useful, could it be put behind a newer manifest version so existing scripts don't break?

@Korioz
Copy link
Contributor

Korioz commented Dec 19, 2024

Refusing to support vectors because of backwards compatibility is stupid.

I approve that point and i am open to changes but these changes will break my use cases, simply because there will be no way to identify a Lua vector from a 2, 3, 4 numbers array.

@Lucas7yoshi
Copy link
Contributor

I'm being nosey but i'd tend to agree that sounds like the kind of thing that would be best put behind a new manifest version beyond cerulean

@thelindat
Copy link
Contributor Author

thelindat commented Dec 20, 2024

As if we'll ever get a manifest version again.

Edit since you reacted with a confused emoji. It's been close to 5 years since cerulean became available, and there have been dozens of things that should have been added but weren't because "breaking" but somehow versioning was never an option. With the new team having pushed a number of far more breaking changes without any sort of backwards-compatibility, why would it happen here?

This update is also dependant on another somewhat breaking change to msgpack where unknown types will outright throw errors (yet to be addressed if that's a concern).

I'd like to say that depending on undefined behaviour (which this absolutely is) in your code means that you are taking a risk by using it and not accounting for future changes to that behaviour. There's this many people who knew how to handle vectors but never considered submitting a PR to properly support for them to FiveM for however many years Lua has had a vector type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants