fix: Camera rotation target from new camera position

[hugolafis]

Type of change

Description 📝

This PR fixes a bug when both camera position and rotation are set simultaneously. Currently, when the new target is calculated from the provided quaternion, the offset it uses to position itself in world space references the old camera position, instead of the new vector that the camera position will be set to, and results in an incorrect target position.

This fix addresses that by passing the new vector through into the utility function CameraManagerHelper.calculateNewTargetFromRotation and utilises that for the target offset.

How has this been tested? 🔍

I have manually tested this by utilising a locally build copy of Reveal as a dependency in another application where I first noticed the issue, and the fix appears to have solved the issue.

Test instructions ℹ️

If you call setCameraState with a provided vector and quaternion, both these values should now match the resulting camera.quaternion and camera.position. Previously, if the provided vector was different to the current position of the camera, the resulting camera quaternion would not match the input value.

Checklist ☑️

• I am proud of this feature.
• I have performed a self-review of my own code.
• I have added PR type (Feat, Bug, Chore, Test, Docs, Style, Refactor) to the PR title.
• I have manually tested this for different scenarios (different models, formats, devices, browsers).
• I have commented my code in hard-to-understand areas.
• I have made corresponding changes to the public documentation.
• I have added unit and visuals tests to prove that my feature works to the best of my ability.
• I have refactored the code for readability to the best of my ability.
• I have checked that my changes do not introduce regressions in the public documentation.
• I have outlined any known defects / lacking capabilities in the contents of this PR.
• I have listed any remaining work that should follow this PR in the description or jira/miro/etc.
• I have added TSDoc to any public facing changes.
• I have added "developer documentation" in any package README.md that is related / applicable to this PR.
• I have noted down and am currently tracking any technical debt introduced in this PR.
• I have thoroughly thought about the architecture of this implementation.
• I have asked peers to test this feature.

[haakonflatval-cognite]

Hi!, thanks for the contribution!

I have some trouble reproducing the problem you are stating. I'm testing now by calling

    viewer.cameraManager.setCameraState({
      position: new THREE.Vector3(50, 0, 51),
      rotation: new THREE.Quaternion(0.5, 0.5, 0.5, 0.5).normalize()
    });

    console.log(
      'New camera position: ',
      this.viewer.cameraManager.getCamera().position,
      'rotation: ',
      new THREE.Quaternion().setFromEuler(this.viewer.cameraManager.getCamera().rotation)
    );

, which prints out

New camera position:  
Object { x: 49.99999999999997, y: -7.105427357601002e-15, z: 51 }

 rotation:  
Object { isQuaternion: true, _x: 0.5, _y: 0.5, _z: 0.4999999999999999, _w: 0.5000000000000001 }

The rotation seems to be as expected. Am I misunderstanding the problem? Do you have another specific input that produces the wrong result?

[hugolafis]

Hello :)

I've just reproduced it on the master branch, using the example viewer provided in the repo. Here's the code I used:

Viewer.tsx

      viewer.domElement.addEventListener('keypress', (ev: KeyboardEvent) => {
        switch (ev.key) {
          case 'g':
            const vec3 = new THREE.Vector3(5, 5, 5);
            const quaternion = new THREE.Quaternion(0, 0, 0, 1);

            viewer.cameraManager.setCameraState({
              position: vec3,
              rotation: quaternion
            });

            console.log(quaternion);
            console.log(viewer.cameraManager.getCamera().quaternion);
            break;
          default:
            break;
        }
      });

When first pressing G, the quaternion and camera quaternion values do not match, however if you press it a second time you should see the values match (due to the camera not moving).

Interestingly I do get a matching quaternion value if I use the same value as you tried, however I'm not sure why!

[haakonflatval-cognite]

@hugolafis I see - I've tried again with your value and can reproduce the error now 👍 I'm approving the PR!

I see there are some CI steps failing. The first one can be fixed by running yarn update-api in the viewer folder.
Let me know if you need assistance with resolving the rest

[haakonflatval-cognite]

LGTM 👍

[codecov]

Codecov Report

Merging #3409 (9ab8989) into master (65c4001) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3409      +/-   ##
==========================================
- Coverage   71.53%   71.53%   -0.01%     
==========================================
  Files         344      344              
  Lines       33232    33235       +3     
  Branches     2582     2582              
==========================================
  Hits        23773    23773              
- Misses       9355     9358       +3     
  Partials      104      104              

Impacted Files	Coverage Δ
...ackages/camera-manager/src/DefaultCameraManager.ts	67.62% <0.00%> (-0.11%)	⬇️
...packages/camera-manager/src/CameraManagerHelper.ts	89.53% <100.00%> (+0.12%)	⬆️

... and 1 file with indirect coverage changes