-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New feature: Enhanced zoomOnBoundingsInfo with Bounding Box Option, Orthographic Camera Support, and Offset #17292
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
base: master
Are you sure you want to change the base?
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://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17292/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/17292/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/17292/merge#BCU1XR#0 |
Devhost visualization test reporter: |
Visualization tests for WebGPU |
WebGL2 visualization test reporter: |
const boxVectorGlobalDiagonal = Vector3.Distance(minimumWorld, maximumWorld); | ||
|
||
// Get aspect ratio in order to calculate frustum slope | ||
public _calculateLowerRadiusFromModelBoundingInfo(minimumWorld: Vector3, maximumWorld: Vector3, mode: string = "sphere", radiusScale: number = 1): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for backwards compatability can you move the new mode parameter to be an optional last parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i see you renamed the function which breaks back-compat. instead, i recommend introducing a new function (with the name you have here), and have the existing _calculateLowerRadiusFromModelBoundingSphere call into your new function
and for our internal implementation (like in the framingbehavior class) you're free to call your new function directly. we just need to make sure we don't break anyone who is already using the sphere fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. I thought about this solution, thinking that just changing the function name in the other methods that use it would be enough. In my ignorance, I didn’t consider that someone might be using this method directly. I can write a new function and call it in the 'box' mode inside the zoomOnBoundingInfo method
let distanceForHorizontalFrustum: number = 0; | ||
let distanceForVerticalFrustum: number = 0; | ||
const height = (maximumWorld.y - minimumWorld.y) * 0.5; | ||
const widht = (maximumWorld.x - minimumWorld.x) * 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit, width* :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always have this problem with this specific word. I might need to get a tattoo of it so I can check it every time I write it down—happens very often, sorry!
this.orthoTop = height; | ||
this.orthoBottom = -this.orthoTop; | ||
} | ||
console.log("ciao", this.orthoBottom, this.orthoTop, this.orthoLeft, this.orthoRight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ciao! :) but let's remove this console log before checkin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... I added some console logs to make sure everything was working in every scenario before pushing, but I should have removed this one.
* to fully enclose the mesh in the viewing frustum. | ||
*/ | ||
protected _calculateLowerRadiusFromModelBoundingSphere(minimumWorld: Vector3, maximumWorld: Vector3): number { | ||
protected _calculateLowerRadiusFromModelBoundingInfo(minimumWorld: Vector3, maximumWorld: Vector3, mode: string = "sphere", radiusScale: number = 1): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add new params to the docstring, for this and the other functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the push failed due to lint control. I'm quite new to TypeScript, so I'm very happy to learn new standard procedures.
let radius = 0; | ||
if (this._mode === FramingBehavior.FitFrustumSidesMode) { | ||
const position = this._calculateLowerRadiusFromModelBoundingSphere(minimumWorld, maximumWorld); | ||
const position = this._calculateLowerRadiusFromModelBoundingInfo(minimumWorld, maximumWorld, mode, radiusScaling); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if radiusscale is not sent, we are defaulting to 1, which then gets sent to camera below
let distance = camera._calculateLowerRadiusFromModelBoundingInfo(minimumWorld, maximumWorld, mode, radiusScale);
however before this change, we would send this._radiusScale in the above code, so the behavior is slightly different
i would recommend making radiusScaling truly optional here, and if it is not passed we default to this._radiusScale still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed there are two _calculateLowerRadiusFromModelBoundingSphere methods—one as a method of FramingBehavior and the other as a method of ArcRotateCamera. One of them takes the radius scale as a parameter, and the other doesn't. I didn't fully understand this choice, but I'll try to figure it out and fix my implementation accordingly.
distanceForVerticalFrustum = (radiusScale * height) / frustumSlopeY + depth; | ||
distance = Math.max(distanceForHorizontalFrustum, distanceForVerticalFrustum); | ||
} else { | ||
return this.radius; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a valid case? or just a fallback if an unknown mode is sent?
i would maybe recommend restricting the mode param to just take in modes that are valid (sphere / box)
you can do that by updating the mode param to by of type 'sphere'|'box', or create a new type BoundingInfoMode= 'sphere' | 'box' and use that for the mode param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a fallback, as you said. I will choose the second option for my fix. It seems cleaner and less hardcoded to me.
Thanks for all your great suggestions and fixes. I'm glad to learn something new.
//Adding a check for orthographic Camera | ||
if (this.mode === Camera.ORTHOGRAPHIC_CAMERA) { | ||
if (aspectRatio < widht / height) { | ||
this.orthoRight = widht * radiusScale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my own learning, where can i read more about how these calculations are determined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking only about rectangles (the canvas and the forward face of the bounding box), the only condition for choosing which dimension to use for x or y (orthoTop or orthoRight) is based on the ratio of the canvas and the ratio of the bounding box face.
If the ratio of the bounding box face is greater than the canvas ratio, we want to set the width (this time I wrote it correctly) as the limit for the ortho camera; otherwise, we want to set the height. In any case, the other range must be proportional to the aspect ratio, or else we will have a deformation of the mesh.
As i said in the previous comment on Monday I'll work on everything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you so much for adding this new feature! added some minor comments :)
Thanks a lot for your review. |
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://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17292/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/17292/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/17292/merge#BCU1XR#0 |
Visualization tests for WebGPU |
Devhost visualization test reporter: |
WebGL2 visualization test reporter: |
hey @PietroFurlan i have proposed a refactor of this PR here I wanted to make sure the behavior stayed exactly the same before/after the change for anyone using the functions, and also make use of a base function when possible so that logic is shared and not duplicated. I was not able to push to your branch directly but feel free to either close this PR / un-draft mine, or to merge the suggestions into your own branch and close mine. Either works! |
Devhost visualization test reporter: |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
hi @PietroFurlan i am still seeing differences between our branches -- namely the updates i made will ensure backwards compatability / same behavior when using the old functions (currently in this branch there is slightly diff behavior in the old funcitons, for example adding ortho framing), plus some duplicate code. please take a look at my PR and cross reference to this one, or we can merge mine directly. also-- for the new ortho formula commit, is that a flaw in my logic? i could have misrepresented it :) please verify the correct math in both sphere/boundingbox. if easier to discuss outside github we can discuss on forum (my username is @Georgie) |
My team and I often need to frame an object at the center of the scene. The zoomOnBoundingsInfo function is very useful for this purpose. However, we noticed that it relies on the bounding sphere to calculate the camera distance, which can cause issues for particularly deep meshes.
To address this, I implemented the ability to choose the method used for distance calculation (bounding sphere or bounding box). Another requirement I had was to work with the orthographic camera, so I implemented a similar system within the same modification for convenience.
This is a tool we use internally, and I thought it would be useful to share it with the community. Additionally, I added the ability to set an offset on the radius so that the mesh does not align exactly with the window edges when displayed.