wasm: transform accepts mat4 #329
Conversation
Codecov ReportBase: 91.51% // Head: 91.48% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #329 +/- ##
==========================================
- Coverage 91.51% 91.48% -0.03%
==========================================
Files 32 32
Lines 3723 3724 +1
==========================================
Hits 3407 3407
- Misses 316 317 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -92,7 +92,7 @@ declare class Manifold { | |||
| * | |||
| * @param m The affine transform matrix to apply to all the vertices. | |||
| */ | |||
| transform(m: Matrix3x4): Manifold; | |||
| transform(m: Matrix3x4 | number[16]): Manifold; | |||
There was a problem hiding this comment.
How would you feel about converting all our matrix & vector types to SealedFloat32Array? It looks like they'll be more efficient in glMatrix, and I like the idea of only supporting one format.
There was a problem hiding this comment.
This requires a more verbose constructor, not sure if users will want this. For example, when you do translation you have to do translate(new Float32Array([1, 2, 3])). I don't think performance matter so much in this case, users can decide on their own if they are doing computationally intensive things with js.
I don't see the problem in supporting multiple types here considering we are just treating them the same as plain arrays.
There was a problem hiding this comment.
Yeah, that's a good point. Actually I think I really just want to replace Matrix3x4 with number[12] to match the output of Mesh.transform. I can do that next.
| } | ||
|
|
||
| Manifold IntersectionN(const std::vector<Manifold>& manifolds) { | ||
| return Manifold::BatchBoolean(manifolds, Manifold::OpType::Intersect); | ||
| ; |
There was a problem hiding this comment.
Yeah I think maybe we should just add the deprecated type definitions to the editor for now, the official typescript file is missing a lot of things and not very useful in our case. Hope that their API did not evolve too much.
|
Yeah we can do so
在 2023年2月20日週一 上午1:23，Emmett Lalish ***@***.***> 寫道：
… ***@***.**** approved this pull request.
Should we go ahead and merge this and work on the rest in follow-on PRs?
—
Reply to this email directly, view it on GitHub
<#329 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC5CGAK54TLUQCVN4CNF4YDWYJJIDANCNFSM6AAAAAAVALBENM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This is for #324, it seems that we only need to change
transformto accept glMatrix mat4 type. This also fixes a silly bug in the existing transform code.I tried adding the type definition for glMatrix to the editor. However, the typescript file they provide only contains type declaration but no function declaration, and typescript engine doesn't evaluate the minified js file so it cannot get the definitions.