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

VecN/Color array conversion support #7312

Merged
merged 16 commits into from
Jan 29, 2025
Merged

VecN/Color array conversion support #7312

merged 16 commits into from
Jan 29, 2025

Conversation

kpal81xd
Copy link
Contributor

@kpal81xd kpal81xd commented Jan 27, 2025

Motivation

When using Vectors/Colors it is very common to want to serialize and deserialize data to either be used for UI (in conjunction with observers) or to synchronise over a network

Overview

  • Adds fromArray(offset: number = 0) methods on VecN and Color classes to allow setting from arrays
  • Adds toArray(out: any[], offset: number = 0) methods to VecN and Color classes to for explicit conversion to array (avoids potential class fields being added from using Object.values

Testing

  • Add unit tests

@kpal81xd kpal81xd self-assigned this Jan 27, 2025
@Maksims
Copy link
Collaborator

Maksims commented Jan 27, 2025

I wander if it should be on set method or copy method. As copy - feels a bit more appropriate, as it copies from another thing. While set using array - feels weird, as Color or Vec - is not an array-like structure.

@marklundin
Copy link
Member

This would be super helpful for the React lib

@kpal81xd
Copy link
Contributor Author

I wander if it should be on set method or copy method. As copy - feels a bit more appropriate, as it copies from another thing. While set using array - feels weird, as Color or Vec - is not an array-like structure.

Copy implies that the target you are copying from is of the same type.

@kungfooman
Copy link
Collaborator

kungfooman commented Jan 27, 2025

I understand the wish for #toArray, but not the update for #set, since that's just .set(...arr)

Edit: same goes for #fromArray, we have always argued against bloating the API when it is not necessary

@kpal81xd
Copy link
Contributor Author

I understand the wish for #toArray, but not the update for #set, since that's just .set(...arr)

Edit: same goes for #fromArray, we have always argued against bloating the API when it is not necessary

It was mainly from typechecking standpoint as we can convert from type number[] whereas set(...arr) requires arr to be a tuple [number, number, ...]. Yes you can cast but the majority of the time you will be dealing with number[] over tuples. This can also get super annoying converting from an array multiple times and usually ends up just being added anyway as a helper.

@kungfooman
Copy link
Collaborator

kungfooman commented Jan 27, 2025

So #fromArray accepts number[] as an argument, which implies that even vec3.fromArray([1]) is "valid" now (so we are ending up with {x: 1, y: undefined, z: undefined}). You just watered down type-checking.

@kpal81xd
Copy link
Contributor Author

So #fromArray accepts number[] as an argument, which implies that even vec3.set([1]) is "valid" now (so we are ending up with {x: 1, y: undefined, z: undefined}). You just watered down type-checking.

That can easily be fixed by just added undefined defaults

@LeXXik
Copy link
Contributor

LeXXik commented Jan 28, 2025

Can we add an optional index to the to/from methods? I have similar methods, but I use it with large arrays and need to point to the values location.

@kpal81xd
Copy link
Contributor Author

Can we add an optional index to the to/from methods? I have similar methods, but I use it with large arrays and need to point to the values location.

I can do it for fromArray but toArray just generates a new array

@mvaligursky
Copy link
Contributor

Can we add an optional index to the to/from methods? I have similar methods, but I use it with large arrays and need to point to the values location.

Agreed, and for toArray I would also provide optional array = [] parameter, allowing writing to a user allocated array (using provided optional offset)

@kpal81xd kpal81xd merged commit 268c838 into main Jan 29, 2025
7 checks passed
@kpal81xd kpal81xd deleted the serialize branch January 29, 2025 12:56
kpal81xd added a commit that referenced this pull request Jan 29, 2025
* Adds better support for converting vectors/color back and from arrays

* Added null check in set method

* Moved array set to separate method

* Removed extra space

* Removed change to vec2 ctor

* Added tests

* Shortened methods

* Added fallback values as the original properties

* Updated to and from array to include optional offsets + toArray to include optional target array

* Added tests for new arguments

* Switched names of array to arr for consistency

* Updated order of alpha argument

* Updated order of arguments in test
@marklundin
Copy link
Member

marklundin commented Jan 29, 2025

@kpal81xd Is there something like ArrayLike<number> we can use here? @type number[] isn't happy with TypedArrays.

@mvaligursky
Copy link
Contributor

@kpal81xd Is there something like ArrayLike<number> we can use here? @type number[] isn't happy with TypedArrays.

I used {number[]|ArrayBufferView} in few places.

* array. Default is 0.
* @returns {number[]} The vector as an array.
* @example
* const v = new pc.Vec3(20, 10, 5, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The +1 version of Vec3 😅 Great work anyway, good job

@Maksims
Copy link
Collaborator

Maksims commented Jan 29, 2025

If array is provided into toArray, should it check if it can fit values in, to avoid array resizing within that function?

* array. Default is 0.
* @returns {number[]} The vector as an array.
* @example
* const v = new pc.Vec3(20, 10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

And this copypaste had to become Vec2

@mvaligursky
Copy link
Contributor

If array is provided into toArray, should it check if it can fit values in, to avoid array resizing within that function?

I would not expect so, that's not worth the cost overhead. It's up to the user to make sure array is in the right size if they don't want resize.

@kungfooman
Copy link
Collaborator

kungfooman commented Jan 30, 2025

If array is provided into toArray, should it check if it can fit values in, to avoid array resizing within that function?

Aren't we just reinventing an opinionated/non-configurable Array#splice?

What if we just did:

pc.Vec3.prototype[Symbol.iterator] = function *() {
  yield this.x;
  yield this.y;
  yield this.z;
}
a = new pc.Vec3(1, 2, 3);
b = new pc.Vec3(11, 22, 33);
c = new pc.Vec3(111, 222, 333);
[...a, ...b, ...c];
arr = [10, 20, 30, 40, 50, 60, 70, 80]
console.log(...arr);
// At pos 5, delete three add three from a at same place:
arr.splice(5, 3, ...a);
console.log(...arr);
// At pos 2, delete nothing, splice c into arr
arr.splice(2, 0, ...c);
console.log(...arr);

Outputs:

image

We also still can't do:

Array.from(a); // Should output: [1, 2, 3]

(we pretty much successfully added another PC specific idiosyncrasy again)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants