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

Add vector and matrix utility functions #15101

Closed

Conversation

axeljaeger
Copy link
Contributor

@axeljaeger axeljaeger commented May 14, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented May 14, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

* @param offset defines the offset in the target array where to start storing values
*/
export function ComposeMatrixToArray<T extends Matrix>(scale: DeepImmutable<Vector3>, rotation: DeepImmutable<Quaternion>, translation: DeepImmutable<Vector3>, array: FloatArray, offset: number): void {
const x = rotation._x,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a code duplication of what we already have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes as mentioned in the forum, this is code duplication of Matrix.ComposeToRef, unfortunately I don't know how to reuse the existing code. Maybe ComposeToRef could make use of this version, but then you would have the + offset with an offset of 0.

@bjsplat
Copy link
Collaborator

bjsplat commented May 14, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented May 14, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15101/merge/testResults/webgpuplaywright/index.html

@bjsplat
Copy link
Collaborator

bjsplat commented May 14, 2024

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

I am not to sure what those new functions bring ?

Is it just a coding style preference ?

@axeljaeger
Copy link
Contributor Author

Hi Sebavan,
the original idea was to add some functions that are not yet in the library, such as Vector2.toVector3, that feel missing because there is already a similar function, such as Vector3.toVector2.

As Deltakosh mentioned in the discussion, for the sake of tree-shaking, we should actually not put more functions in Vector and Matrix, but rather add them to the mentioned utility functions file, so I prepared this MR as a base of discussion.

Actually, personally I got unsure wether it is worth filling this file utility file already with the missing functions while the majority of this functions remains for now in Vector an Matrix classes respectively.

When it is about making the functions pure functions and not part of the classes I can also easily add them somewhere in my project.

What do you think?

@sebavan
Copy link
Member

sebavan commented May 17, 2024

Hi Sebavan, the original idea was to add some functions that are not yet in the library, such as Vector2.toVector3, that feel missing because there is already a similar function, such as Vector3.toVector2.

As Deltakosh mentioned in the discussion, for the sake of tree-shaking, we should actually not put more functions in Vector and Matrix, but rather add them to the mentioned utility functions file, so I prepared this MR as a base of discussion.

Actually, personally I got unsure wether it is worth filling this file utility file already with the missing functions while the majority of this functions remains for now in Vector an Matrix classes respectively.

When it is about making the functions pure functions and not part of the classes I can also easily add them somewhere in my project.

What do you think?

I completely agree that new addition should be in this file and this format, but here I am more questioning the general usefulness of those shortcuts and how frequently we would use them to understand if it justifies the extra maintenance and weight in the UMD scenario :-)

@deltakosh @RaananW any thoughts?

@sebavan
Copy link
Member

sebavan commented May 22, 2024

Lets keep them in your project for now and reopen if we change our mind then ?

@sebavan sebavan closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants