update camera's target when the mouse wheel is rolling for zoomtomouselocation #14653

ecojust · 2023-12-27T17:25:08Z

https://forum.babylonjs.com/t/zoomtomouselocation-issue-after-camera-settarget/46716

fixed bug : zoomtomouselocation-issue-after-camera-settarget

bjsplat · 2023-12-27T17:26:45Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat · 2023-12-27T17:44:22Z

Snapshot stored with reference name:
refs/pull/14653/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14653/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14653/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14653/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/14653/merge
https://gui.babylonjs.com/?snapshot=refs/pull/14653/merge
https://nme.babylonjs.com/?snapshot=refs/pull/14653/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/14653/merge#BCU1XR#0

Popov72 · 2023-12-27T18:29:11Z

Thanks for the PR, but please be patient for the review as the team is on Christmas vacation.

sebavan

I do not think this is the right fix but I am terrible with Cameras :-)

Could we not update some of the parameters only once when we actually sets the new target ?

The new change is pretty heavy to run every frame.

to determine whether the user has forced a camera target value

ecojust · 2024-01-03T12:34:38Z

I do not think this is the right fix but I am terrible with Cameras :-)

Could we not update some of the parameters only once when we actually sets the new target ?

The new change is pretty heavy to run every frame.

yeah , u are right
I have used a variable to limit the occurrence of this behavior (_targetSetManually)

sebavan

I am still not sure it is the right approach overall.

First, we know if someone called setTarget and we could use this as our trigger.

Second, I wonder if some parameters could be computed once in setTarget only so that the render loop does not change if setTarget has not been called.

Lets wait on @PolygonalSun inputs.

packages/dev/core/src/Cameras/Inputs/arcRotateCameraMouseWheelInput.ts

PolygonalSun · 2024-01-03T20:37:00Z

packages/dev/core/src/Cameras/Inputs/arcRotateCameraMouseWheelInput.ts

+        const direction = camera.target.subtract(camera.position).normalize();
+        const ray = new Ray(camera.position, direction, Number.MAX_SAFE_INTEGER);
+        const ground = Plane.FromPositionAndNormal(Vector3.Zero(), camera.upVector);
+        const distance = ray.intersectsPlane(ground) ?? 0;
+        const intersectionPoint = ray.origin.add(ray.direction.scale(distance));
+        camera.setTarget(intersectionPoint);


A lot of this code is similar to the code found in _updateHitPlane and _getPosition. It'd be worth looking into combining some of the new code with the _updateHitPlane code so you're not potentially creating two rays and planes on a single frame.

PolygonalSun · 2024-01-03T20:38:37Z

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

@@ -61,6 +61,8 @@ export class ArcRotateCamera extends TargetCamera {
    protected _target: Vector3;
    @serializeAsMeshReference("targetHost")
    protected _targetHost: Nullable<AbstractMesh>;
+    @serialize("targetSetManually")
+    protected _targetSetManually: Boolean;


Is there any reason why this can't just be a public variable instead of a property with a protected variable?

It is considered here that this variable is used in conjunction with other functions and should not be explicitly modified separately
For example, modify this parameter with setTarget

PolygonalSun · 2024-01-03T21:05:32Z

packages/dev/core/src/Cameras/Inputs/arcRotateCameraMouseWheelInput.ts

+        const distance = ray.intersectsPlane(ground) ?? 0;
+        const intersectionPoint = ray.origin.add(ray.direction.scale(distance));
+        camera.setTarget(intersectionPoint);
+        camera.targetSetManually = false;


It looks like this value is set outside of the code (possibly by the user) to allow the user to force the target to be set for a single frame and then is reset to false. What is the use case for this to be set to true? Is it something to be set to true once (as needed) or repeated more frequently?

This value can now be set to true when passed in by setTarget.

The use case that is true is :
to tell the mouse wheel event that the user forcibly changed the camera's target,
and that the camera's reasonable target needs to be updated once to ensure proper use of zoomToMouseLocation

PolygonalSun · 2024-01-03T21:22:40Z

Marking as draft. I think that this needs a bit more work before we can merge it.

ecojust · 2024-01-04T05:20:42Z

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

     */
-    public setTarget(target: AbstractMesh | Vector3, toBoundingCenter = false, allowSamePosition = false, cloneAlphaBetaRadius = false): void {
+    public setTarget(target: AbstractMesh | Vector3, toBoundingCenter = false, allowSamePosition = false, cloneAlphaBetaRadius = false, setManually = false): void {


@sebavan @PolygonalSun
My previous idea was to change the _targetSetManually variable by adding a parameter to setTarget so that the user does not have to explicitly change the parameter
If you agree, I can resume the previous operation

github-actions · 2024-03-14T00:20:17Z

This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale".

github-actions · 2024-03-30T00:19:31Z

This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale".

deltakosh · 2024-04-02T22:02:49Z

Closing with no activity

update camera's target when the mouse wheel is rolling

6b2562e

ecojust changed the title ~~update camera's target when the mouse wheel is rolling~~ update camera's target when the mouse wheel is rolling for zoomtomouselocation Dec 27, 2023

Popov72 requested a review from PolygonalSun December 27, 2023 18:26

sebavan requested changes Jan 2, 2024

View reviewed changes

juzisang added 2 commits January 3, 2024 18:53

Defines a variable(_targetSetManually)

99f438c

to determine whether the user has forced a camera target value

Rollback the setTarget function

06f4fb6

ecojust requested a review from sebavan January 3, 2024 12:35

sebavan requested changes Jan 3, 2024

View reviewed changes

packages/dev/core/src/Cameras/Inputs/arcRotateCameraMouseWheelInput.ts Show resolved Hide resolved

PolygonalSun suggested changes Jan 3, 2024

View reviewed changes

PolygonalSun marked this pull request as draft January 3, 2024 21:21

juzisang added 3 commits January 4, 2024 11:57

update the target first and then update the hit plane,

2528e80

Optimized code

ff43158

use TmpVectors store direction

34c5bb1

ecojust commented Jan 4, 2024

View reviewed changes

juzisang added 2 commits January 5, 2024 00:22

Optimized code:reduce Garbage Collection pressure

c2526bc

Merge remote-tracking branch 'bbl/master'

ffc5d67

github-actions bot added stale and removed stale labels Mar 14, 2024

github-actions bot added the stale label Mar 30, 2024

deltakosh closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update camera's target when the mouse wheel is rolling for zoomtomouselocation #14653

update camera's target when the mouse wheel is rolling for zoomtomouselocation #14653

ecojust commented Dec 27, 2023 •

edited by sebavan

bjsplat commented Dec 27, 2023

bjsplat commented Dec 27, 2023

Popov72 commented Dec 27, 2023

sebavan left a comment

ecojust commented Jan 3, 2024

sebavan left a comment

PolygonalSun Jan 3, 2024

PolygonalSun Jan 3, 2024

ecojust Jan 4, 2024

PolygonalSun Jan 3, 2024

ecojust Jan 4, 2024

PolygonalSun commented Jan 3, 2024

ecojust Jan 4, 2024

github-actions bot commented Mar 14, 2024 •

edited by sebavan

github-actions bot commented Mar 30, 2024

deltakosh commented Apr 2, 2024

update camera's target when the mouse wheel is rolling for zoomtomouselocation #14653

update camera's target when the mouse wheel is rolling for zoomtomouselocation #14653

Conversation

ecojust commented Dec 27, 2023 • edited by sebavan

bjsplat commented Dec 27, 2023

bjsplat commented Dec 27, 2023

Popov72 commented Dec 27, 2023

sebavan left a comment

Choose a reason for hiding this comment

ecojust commented Jan 3, 2024

sebavan left a comment

Choose a reason for hiding this comment

PolygonalSun Jan 3, 2024

Choose a reason for hiding this comment

PolygonalSun Jan 3, 2024

Choose a reason for hiding this comment

ecojust Jan 4, 2024

Choose a reason for hiding this comment

PolygonalSun Jan 3, 2024

Choose a reason for hiding this comment

ecojust Jan 4, 2024

Choose a reason for hiding this comment

PolygonalSun commented Jan 3, 2024

ecojust Jan 4, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2024 • edited by sebavan

github-actions bot commented Mar 30, 2024

deltakosh commented Apr 2, 2024

ecojust commented Dec 27, 2023 •

edited by sebavan

github-actions bot commented Mar 14, 2024 •

edited by sebavan