Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@
this.zoomOnBoundingInfo(min, max, focusOnOriginXZ, onAnimationEnd);
}

/**

Check failure on line 307 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo (Format, Lint, and more)

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L307

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(307,5): error jsdoc/require-param: Missing JSDoc @param "mode" declaration.

Check failure on line 307 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo (Format, Lint, and more)

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L307

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(307,5): error jsdoc/require-param: Missing JSDoc @param "radiusScaling" declaration.

Check failure on line 307 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L307

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(307,5): error jsdoc/require-param: Missing JSDoc @param "mode" declaration.

Check failure on line 307 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L307

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(307,5): error jsdoc/require-param: Missing JSDoc @param "radiusScaling" declaration.
* Targets the bounding box info defined by its extends and updates zoom level accordingly.
* @param minimumWorld Determines the smaller position of the bounding box extend
* @param maximumWorld Determines the bigger position of the bounding box extend
Expand All @@ -312,7 +312,14 @@
* @param onAnimationEnd Callback triggered at the end of the framing animation
* @returns true if the zoom was done
*/
public zoomOnBoundingInfo(minimumWorld: Vector3, maximumWorld: Vector3, focusOnOriginXZ: boolean = false, onAnimationEnd: Nullable<() => void> = null): boolean {
public zoomOnBoundingInfo(
minimumWorld: Vector3,
maximumWorld: Vector3,
focusOnOriginXZ: boolean = false,
onAnimationEnd: Nullable<() => void> = null,
mode: string = "sphere",
radiusScaling: number = 1
): boolean {
let zoomTarget: Vector3;

if (!this._attachedCamera) {
Expand Down Expand Up @@ -350,13 +357,13 @@
// Small delta ensures camera is not always at lower zoom limit.
let radius = 0;
if (this._mode === FramingBehavior.FitFrustumSidesMode) {
const position = this._calculateLowerRadiusFromModelBoundingSphere(minimumWorld, maximumWorld);
const position = this._calculateLowerRadiusFromModelBoundingInfo(minimumWorld, maximumWorld, mode, radiusScaling);
Copy link
Contributor

@georginahalpern georginahalpern Oct 15, 2025

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

Copy link
Author

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.

if (this.autoCorrectCameraLimitsAndSensibility) {
this._attachedCamera.lowerRadiusLimit = radiusWorld.length() + this._attachedCamera.minZ;
}
radius = position;
} else if (this._mode === FramingBehavior.IgnoreBoundsSizeMode) {
radius = this._calculateLowerRadiusFromModelBoundingSphere(minimumWorld, maximumWorld);
radius = this._calculateLowerRadiusFromModelBoundingInfo(minimumWorld, maximumWorld, mode, radiusScaling);
if (this.autoCorrectCameraLimitsAndSensibility && this._attachedCamera.lowerRadiusLimit === null) {
this._attachedCamera.lowerRadiusLimit = this._attachedCamera.minZ;
}
Expand Down Expand Up @@ -392,21 +399,21 @@
return true;
}

/**

Check failure on line 402 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo (Format, Lint, and more)

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L402

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(402,5): error jsdoc/require-param: Missing JSDoc @param "mode" declaration.

Check failure on line 402 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo (Format, Lint, and more)

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L402

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(402,5): error jsdoc/require-param: Missing JSDoc @param "radiusScale" declaration.

Check failure on line 402 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L402

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(402,5): error jsdoc/require-param: Missing JSDoc @param "mode" declaration.

Check failure on line 402 in packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts#L402

packages/dev/core/src/Behaviors/Cameras/framingBehavior.ts(402,5): error jsdoc/require-param: Missing JSDoc @param "radiusScale" declaration.
* Calculates the lowest radius for the camera based on the bounding box of the mesh.
* @param minimumWorld
* @param maximumWorld
* @returns The minimum distance from the primary mesh's center point at which the camera must be kept in order
* 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 {
Copy link
Contributor

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

Copy link
Author

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.

const camera = this._attachedCamera;

if (!camera) {
return 0;
}

let distance = camera._calculateLowerRadiusFromModelBoundingSphere(minimumWorld, maximumWorld, this._radiusScale);
let distance = camera._calculateLowerRadiusFromModelBoundingInfo(minimumWorld, maximumWorld, mode, radiusScale);
if (camera.lowerRadiusLimit && this._mode === FramingBehavior.IgnoreBoundsSizeMode) {
// Don't exceed the requested limit
distance = distance < camera.lowerRadiusLimit ? camera.lowerRadiusLimit : distance;
Expand Down
61 changes: 46 additions & 15 deletions packages/dev/core/src/Cameras/arcRotateCamera.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1448,7 +1448,7 @@
meshes = meshes || this.getScene().meshes;

const minMaxVector = Mesh.MinMax(meshes);
let distance = this._calculateLowerRadiusFromModelBoundingSphere(minMaxVector.min, minMaxVector.max);
let distance = this._calculateLowerRadiusFromModelBoundingInfo(minMaxVector.min, minMaxVector.max);

// If there are defined limits, we need to take them into account
distance = Math.max(Math.min(distance, this.upperRadiusLimit || Number.MAX_VALUE), this.lowerRadiusLimit || 0);
Expand Down Expand Up @@ -1561,24 +1561,55 @@
/**
* @internal
*/
public _calculateLowerRadiusFromModelBoundingSphere(minimumWorld: Vector3, maximumWorld: Vector3, radiusScale: number = 1): number {
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 {
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Author

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

const engine = this.getScene().getEngine();
const aspectRatio = engine.getAspectRatio(this);
const frustumSlopeY = Math.tan(this.fov / 2);
const frustumSlopeX = frustumSlopeY * aspectRatio;

// Formula for setting distance
// (Good explanation: http://stackoverflow.com/questions/2866350/move-camera-to-fit-3d-scene)
const radiusWithoutFraming = boxVectorGlobalDiagonal * 0.5;

// Horizon distance
const radius = radiusWithoutFraming * radiusScale;
const distanceForHorizontalFrustum = radius * Math.sqrt(1.0 + 1.0 / (frustumSlopeX * frustumSlopeX));
const distanceForVerticalFrustum = radius * Math.sqrt(1.0 + 1.0 / (frustumSlopeY * frustumSlopeY));
return Math.max(distanceForHorizontalFrustum, distanceForVerticalFrustum);
const oldRadius = this.radius;
let distance = oldRadius;
let distanceForHorizontalFrustum: number = 0;
let distanceForVerticalFrustum: number = 0;
const height = (maximumWorld.y - minimumWorld.y) * 0.5;
const widht = (maximumWorld.x - minimumWorld.x) * 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit, width* :)

Copy link
Author

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!

if (mode === "sphere") {
const boxVectorGlobalDiagonal = Vector3.Distance(minimumWorld, maximumWorld);
// Get aspect ratio in order to calculate frustum slope

// Formula for setting distance
// (Good explanation: http://stackoverflow.com/questions/2866350/move-camera-to-fit-3d-scene)
const radiusWithoutFraming = boxVectorGlobalDiagonal * 0.5;

// Horizon distance
const radius = radiusWithoutFraming * radiusScale;
distanceForHorizontalFrustum = radius * Math.sqrt(1.0 + 1.0 / (frustumSlopeX * frustumSlopeX));
distanceForVerticalFrustum = radius * Math.sqrt(1.0 + 1.0 / (frustumSlopeY * frustumSlopeY));
distance = Math.max(distanceForHorizontalFrustum, distanceForVerticalFrustum);
} else if (mode === "bounding box") {
// Setting the distance according to the bounding box (centring the first face of the bounding box)
const depth = (maximumWorld.z - minimumWorld.z) * 0.5;
distanceForHorizontalFrustum = (radiusScale * widht) / frustumSlopeX + depth;
distanceForVerticalFrustum = (radiusScale * height) / frustumSlopeY + depth;
distance = Math.max(distanceForHorizontalFrustum, distanceForVerticalFrustum);
} else {
return this.radius;
Copy link
Contributor

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

Copy link
Author

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;
Copy link
Contributor

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?

Copy link
Author

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

this.orthoLeft = -this.orthoRight;
this.orthoTop = this.orthoRight / aspectRatio;
this.orthoBottom = this.orthoLeft / aspectRatio;
} else {
this.orthoRight = height * aspectRatio * radiusScale;
this.orthoLeft = -this.orthoRight * radiusScale;
this.orthoTop = height;
this.orthoBottom = -this.orthoTop;
}
console.log("ciao", this.orthoBottom, this.orthoTop, this.orthoLeft, this.orthoRight);

Check failure on line 1610 in packages/dev/core/src/Cameras/arcRotateCamera.ts

View check run for this annotation

Azure Pipelines / CI-Monorepo (Format, Lint, and more)

packages/dev/core/src/Cameras/arcRotateCamera.ts#L1610

packages/dev/core/src/Cameras/arcRotateCamera.ts(1610,13): error no-console: Unexpected console statement.
Copy link
Contributor

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

Copy link
Author

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.

}
return distance;
}

/**
Expand Down