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): Add unpackers for Vector2, Vector3, and Vector4 u… #2946

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JustGodWork
Copy link

@JustGodWork JustGodWork commented Nov 19, 2024

Goal of this PR

This PR aims to add unpackers for Vector2, Vector3 and Vector4 using msgpack-lite to handle the conversion of Uint8Array data to Buffer and read float values for each vector type.

How is this PR achieving the goal

  • Defined constants for EXT_VECTOR2, EXT_VECTOR3, and EXT_VECTOR4.
  • Implemented unpacker for Vector2 that reads 2 float values (x, y) from Uint8Array.
  • Implemented unpacker for Vector3 that reads 3 float values (x, y, z) from Uint8Array.
  • Implemented unpacker for Vector4 that reads 4 float values (x, y, z, w) from Uint8Array.
  • Converted Uint8Array to Buffer for reading float values.

This PR applies to the following area(s)

  • ScRT: JS

Successfully tested on

Game builds: 3258

Platforms: Windows, Linux

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.

Code example:

on('myExampleEvent', (aVector3) => {
    console.log(`aVector3: x: ${aVector3[0]}, y:${aVector3[1]}, z:${aVector3[2]}` );
    // Output: aVector3: x: 1135.7406, y:443.1956, z:82.6768
});
-- Triggering the event with a vector3 value
TriggerEvent('myExampleEvent', vector3(1135.7406, 443.1956, 82.6768))

Explanation

Without these unpackers, when receiving data encoded with msgpack, we would get a Buffer containing the msgpack type and other data. This Buffer would need to be manually converted and parsed to extract the float values for x, y, z, and w. The unpackers simplify this process by automatically converting the Uint8Array to a Buffer and reading the float values, returning them as an array. This makes it easier to work with vector data in JavaScript when using msgpack for serialization and deserialization.

…sing msgpack

- Define constants for EXT_VECTOR2, EXT_VECTOR3, and EXT_VECTOR4
- Implement unpacker for Vector2 that reads 2 float values (x, y) from Uint8Array
- Implement unpacker for Vector3 that reads 3 float values (x, y, z) from Uint8Array
- Implement unpacker for Vector4 that reads 4 float values (x, y, z, w) from Uint8Array
- Convert Uint8Array to Buffer for reading float values
@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Nov 19, 2024
Comment on lines 53 to 85
// Vector2 unpacker
codec.addExtUnpacker(EXT_VECTOR2, (data) => {
// Converting Uint8Array to Buffer
const buffer = Buffer.from(data);
// Reading 3 float values from the buffer
const x = buffer.readFloatLE(0);
const y = buffer.readFloatLE(4);
return [x, y];
});

// Vector3 unpacker
codec.addExtUnpacker(EXT_VECTOR3, (data) => {
// Converting Uint8Array to Buffer
const buffer = Buffer.from(data);
// Reading 3 float values from the buffer
const x = buffer.readFloatLE(0);
const y = buffer.readFloatLE(4);
const z = buffer.readFloatLE(8);
return [x, y, z];
});

// Vector4 unpacker
codec.addExtUnpacker(EXT_VECTOR4, (data) => {
// Converting Uint8Array to Buffer
const buffer = Buffer.from(data);
// Reading 3 float values from the buffer
const x = buffer.readFloatLE(0);
const y = buffer.readFloatLE(4);
const z = buffer.readFloatLE(8);
const w = buffer.readFloatLE(12);
return [x, y, z, w];
});

Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent.

const vectorUnpacker = (data => [...new Float32Array(data.buffer)]);

codec.addExtUnpacker(EXT_VECTOR2, vectorUnpacker);
codec.addExtUnpacker(EXT_VECTOR3, vectorUnpacker);
codec.addExtUnpacker(EXT_VECTOR4, vectorUnpacker);

And this one should improve number precision.

const vectorUnpacker = (data => Array.from(new Float32Array(data.buffer), (v) => Number(v.toPrecision(7))));

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, @thelindat! That’s a great idea, and I really like the improvement for precision. I’ll go ahead and push this change.

@github-actions github-actions bot added invalid Requires changes before it's considered valid and can be (re)triaged and removed triage Needs a preliminary assessment to determine the urgency and required action labels Nov 20, 2024
@github-actions github-actions bot added triage Needs a preliminary assessment to determine the urgency and required action and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Nov 20, 2024
Copy link
Contributor

@iridium-cfx iridium-cfx left a comment

Choose a reason for hiding this comment

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

This seems like a good idea, but also sounds like it will break any existing code which uses the current behavior.

I can't imagine much/any code would use the current behavior though, so it might be fine?

@JustGodWork
Copy link
Author

This seems like a good idea, but also sounds like it will break any existing code which uses the current behavior.

I can't imagine much/any code would use the current behavior though, so it might be fine?

Actually, I knew this question would come up when I first developed this idea, but I thought this code would add a logical behavior for vector processing. I don't know how many projects use the old way, but I find this code makes more sense.

@thelindat
Copy link
Contributor

it will break any existing code which uses the current behavior.
I can't imagine much/any code would use the current behavior though

That's what I was told a while back so I ended up manually handling the serialisation - but the team hasn't been very shy about breaking changes, and getting serialisation for all JS features is fairly important (getting bigint, date, maps, and sets would be nice).

Though if this is added it would be nice to have a vector class added by cfx, rather than just getting an array.

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.

3 participants