-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Add option to use boundingBox (instead of boundingSphere) when calculating arcRotateCam lowerRadius. Add orthographic framing #17329
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
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/17329/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/17329/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/17329/merge#BCU1XR#0 |
|
Devhost visualization test reporter: |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
I work a lot with BabylonJS and I love getting my hands dirty with the code. For example, I’ve made a “light” version of the OBJExporter that can shrink mesh size by up to 60%, by skipping details like normals, materials, etc. Let me know if you’re interested! |
| const depth = (maximumWorld.z - minimumWorld.z) * 0.5; | ||
| const aspectRatio = this.getScene().getEngine().getAspectRatio(this); | ||
|
|
||
| if (this.mode === Camera.ORTHOGRAPHIC_CAMERA) { |
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.
@PietroFurlan in your PR the logic here is slightly different - can you confirm which math is correct? do we want the same ortho formula regardless of sphere/box, or should they diffeR?
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.
@PietroFurlan closign PR for now as it has been open a while, lets discuss the correct approach via these comments and once aligned i can re-open the PR
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 I understood correctly: with an orthographic camera there’s no concept of radius or FOV or else, so the sphere/box mode shouldn’t affect the frustum math. Behavior should be identical in ortho mode. In your PR the ortho option existed only for the box mode; by changing the math only there, I introduced the inconsistency—my bad.
In ortho mode, the math is the same for box and sphere, and should be:
this.orthoTop = height * radiusScale;
this.orthoBottom = -this.orthoTop;
this.orthoRight = this.orthoTop * aspectRatio;
this.orthoLeft = -this.orthoRight;| this.orthoRight = height * aspectRatio * radiusScale; | ||
| this.orthoLeft = -this.orthoRight * radiusScale; | ||
| this.orthoTop = height; | ||
| this.orthoBottom = -this.orthoTop; |
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.
there is an error on this formula when passing a radiusScale != 1.
In my last commit you find the correct one:
| this.orthoRight = height * aspectRatio * radiusScale; | |
| this.orthoLeft = -this.orthoRight * radiusScale; | |
| this.orthoTop = height; | |
| this.orthoBottom = -this.orthoTop; | |
| this.orthoTop = height * radiusScale; | |
| this.orthoBottom = -this.orthoTop; | |
| this.orthoRight = this.orthoTop * aspectRatio; | |
| this.orthoLeft = -this.orthoRight; | |
| const depth = (maximumWorld.z - minimumWorld.z) * 0.5; | ||
| const aspectRatio = this.getScene().getEngine().getAspectRatio(this); | ||
|
|
||
| if (this.mode === Camera.ORTHOGRAPHIC_CAMERA) { |
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 I understood correctly: with an orthographic camera there’s no concept of radius or FOV or else, so the sphere/box mode shouldn’t affect the frustum math. Behavior should be identical in ortho mode. In your PR the ortho option existed only for the box mode; by changing the math only there, I introduced the inconsistency—my bad.
In ortho mode, the math is the same for box and sphere, and should be:
this.orthoTop = height * radiusScale;
this.orthoBottom = -this.orthoTop;
this.orthoRight = this.orthoTop * aspectRatio;
this.orthoLeft = -this.orthoRight;
This is a refactor of the PR proposed here: #17292
Adds new optional param 'mode' to the framingBehavior zoomOnBoundingInfo. If specified, calls into the camera's new _calculateLowerRadiusFromModelBoundingInfo function which respects the passed in mode and adds orthographic framing
If no mode is sent, default to old behavior (using cameras _calculateLowerRadiusFromModelBoundingSphere fn, and not adding orthographic framing)
This maintains backwards compatibility of old behavior while still offering option to use new behavior