Skip to content
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

updating Camera.changed to account for changes in roll #11844

Merged
merged 4 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- Fixed a bug affecting voxel shader compilation in WebGL1 contexts. [#11798](https://github.com/CesiumGS/cesium/pull/11798)
- Fixed a bug where legacy B3DM files that contained glTF 1.0 data that used a `CONSTANT` technique in the `KHR_material_common` extension and only defined ambient- or emissive textures (but no diffuse textures) showed up without any texture [#11825](https://github.com/CesiumGS/cesium/pull/11825)
- Fixed an error when the `screenSpaceEventHandler` was destroyed before `Viewer` [#10576](https://github.com/CesiumGS/cesium/issues/10576)
- Fixes how `Camera.changed` handles changes in `roll`. [#11844](https://github.com/CesiumGS/cesium/pull/11844)

##### Additions :tada:

Expand Down
39 changes: 35 additions & 4 deletions packages/engine/Source/Scene/Camera.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,25 +380,56 @@ Camera.prototype._updateCameraChanged = function () {

const percentageChanged = camera.percentageChanged;

// check heading
const currentHeading = camera.heading;

if (!defined(camera._changedHeading)) {
camera._changedHeading = currentHeading;
}

let delta =
let headingDelta =
Math.abs(camera._changedHeading - currentHeading) % CesiumMath.TWO_PI;
delta = delta > CesiumMath.PI ? CesiumMath.TWO_PI - delta : delta;
headingDelta =
headingDelta > CesiumMath.PI
? CesiumMath.TWO_PI - headingDelta
: headingDelta;

// Since delta is computed as the shortest distance between two angles
// the percentage is relative to the half circle.
const headingChangedPercentage = delta / Math.PI;
const headingChangedPercentage = headingDelta / Math.PI;

if (headingChangedPercentage > percentageChanged) {
camera._changed.raiseEvent(headingChangedPercentage);
camera._changedHeading = currentHeading;
}

// check roll
const currentRoll = camera.roll;

if (!defined(camera._changedRoll)) {
camera._changedRoll = currentRoll;
}

let rollDelta =
Math.abs(camera._changedRoll - currentRoll) % CesiumMath.TWO_PI;
rollDelta =
rollDelta > CesiumMath.PI ? CesiumMath.TWO_PI - rollDelta : rollDelta;

// Since delta is computed as the shortest distance between two angles
// the percentage is relative to the half circle.
const rollChangedPercentage = rollDelta / Math.PI;

if (rollChangedPercentage > percentageChanged) {
camera._changedRoll = currentRoll;
}
if (
rollChangedPercentage > percentageChanged ||
headingChangedPercentage > percentageChanged
) {
camera._changed.raiseEvent(
Math.max(rollChangedPercentage, headingChangedPercentage)
);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed there is no early return in the heading check previously. I believe this is because the function needs to clone the frustum before exiting, as is done below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that makes sense. I'll remove the early return since previous behavior was okay.

I was trying to avoid the event maybe being fired multiple times, but maybe that is a non-issue -- or at the very least outside the scope of this one.

Thanks for the review!

}
if (camera._mode === SceneMode.SCENE2D) {
if (!defined(camera._changedFrustum)) {
camera._changedPosition = Cartesian3.clone(
Expand Down
24 changes: 23 additions & 1 deletion packages/engine/Specs/Scene/SceneSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1263,8 +1263,10 @@ describe(
expect(args[0][0]).toBeGreaterThan(scene.camera.percentageChanged);
});

it("raises the camera changed event on heading changed", function () {
it("raises the camera changed event on heading changed when looking up", function () {
const spyListener = jasmine.createSpy("listener");

scene.camera.lookUp(0.785398);
scene.camera.changed.addEventListener(spyListener);

scene.initializeFrame();
Expand All @@ -1284,7 +1286,27 @@ describe(
expect(args[0].length).toEqual(1);
expect(args[0][0]).toBeGreaterThan(scene.camera.percentageChanged);
});
it("raises the camera changed event on roll changed", function () {
const spyListener = jasmine.createSpy("listener");
scene.camera.changed.addEventListener(spyListener);

scene.initializeFrame();
scene.render();

scene.camera.twistLeft(
CesiumMath.PI * (scene.camera.percentageChanged + 0.1)
);

scene.initializeFrame();
scene.render();

expect(spyListener.calls.count()).toBe(1);

const args = spyListener.calls.allArgs();
expect(args.length).toEqual(1);
expect(args[0].length).toEqual(1);
expect(args[0][0]).toBeGreaterThan(scene.camera.percentageChanged);
});
it("raises the camera changed event on position changed", function () {
const spyListener = jasmine.createSpy("listener");
scene.camera.changed.addEventListener(spyListener);
Expand Down
Loading