Add helper method to compute the maximum bounding boxes for an array of meshes #15045
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15045/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/15045/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/15045/merge#BCU1XR#0 |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU (Experimental) |
| * @param animationStep An optional animation step value indicating the amount to iterate while computing bounding boxes | ||
| * @returns An array of bounding boxes corresponding to the input meshes, animation group, and animation step values | ||
| */ | ||
| export function computeMaxBoundingBoxes(meshes: Array<AbstractMesh>, animationGroup?: AnimationGroup, animationStep = 1 / 6): Array<BoundingBox> { |
There was a problem hiding this comment.
Should it not be used in CreateDefaultCamera ?
There was a problem hiding this comment.
I'm not sure. This is an expensive operation.
| * @param animationStep An optional animation step value indicating the amount to iterate while computing bounding boxes | ||
| * @returns An array of bounding boxes corresponding to the input meshes, animation group, and animation step values | ||
| */ | ||
| export function computeMaxBoundingBoxes(meshes: Array<AbstractMesh>, animationGroup?: AnimationGroup, animationStep = 1 / 6): Array<BoundingBox> { |
There was a problem hiding this comment.
The meshes array is not intended to be mutated, yes? Should it a be a DeepImmutable?
There was a problem hiding this comment.
That's a good idea.
| * @param animationStep An optional animation step value indicating the amount to iterate while computing bounding boxes | ||
| * @returns An array of bounding boxes corresponding to the input meshes, animation group, and animation step values | ||
| */ | ||
| export function computeMaxBoundingBoxes(meshes: Array<AbstractMesh>, animationGroup?: AnimationGroup, animationStep = 1 / 6): Array<BoundingBox> { |
There was a problem hiding this comment.
If no animationGroup is provided, then no animationStep should be provided, correct? Would it be worth it to reflect this in the API, either with classic overloads or named tuples?
| export function computeMaxBoundingBoxes(meshes: Array<AbstractMesh>, animationGroup?: AnimationGroup, animationStep = 1 / 6): Array<BoundingBox> { | |
| export function computeMaxBoundingBoxes(...args: [meshes: Array<AbstractMesh>] | [meshes: Array<AbstractMesh>, animationGroup: AnimationGroup] | [meshes: Array<AbstractMesh>, animationGroup: AnimationGroup, animationStep: number]): Array<BoundingBox> { | |
There was a problem hiding this comment.
I personally don't think this is worth the effort. It makes the code more difficult to read.
I considered using a options arguments instead of individual arguments but decided against it also.
No strong preference though.
There was a problem hiding this comment.
Personally I would prefer an options object using discriminated unions to make it super clear that it is not valid to provide an animationStep without providing an animationGroup. I think this kind of API design just helps ppl understand how to use it, but your call.
There was a problem hiding this comment.
I think requiring that animationStep not be passed in when animationGroup is not present is harder to use. Most of the time, the user doesn't know if there is going to be an animationGroup or not.
| if (animationGroup) { | ||
| const step = animationGroup.getLength() * animationStep; | ||
|
|
||
| for (let frame = animationGroup.from; frame < animationGroup.to; frame += step) { |
There was a problem hiding this comment.
Should be frame <= animationGroup.to, as the to frame is inclusive.
|
|
||
| for (let frame = animationGroup.from; frame < animationGroup.to; frame += step) { | ||
| // Must increment the render id such that world matrices are recomputed. | ||
| animationGroup.scene.incrementRenderId(); |
There was a problem hiding this comment.
nit: You could pass true to computeWorldMatrix() instead.
| @@ -0,0 +1,62 @@ | |||
| import { AnimationGroup } from "../Animations/animationGroup"; | |||
There was a problem hiding this comment.
| import { AnimationGroup } from "../Animations/animationGroup"; | |
| import type { AnimationGroup } from "../Animations/animationGroup"; |
No description provided.