Camera Roll

[NathanMOlson]

Reopening accidentally closed PR: #4771

This is the first part of #4717: adding camera roll.

In this PR, roll is defined as rotation about the camera boresight. Rotation order is bearing, pitch, roll. easeTo() and flyTo() are implemented using linear interpolation of Euler angles spherical linear interpolation.

You can play with this feature here, using the controls in the top right corner: https://nathanmolson.github.io/camera_roll

There is some weirdness near +/- 90 degrees roll, but I'd like to get #4750 merged before attempting to address it. Near 90 degrees, it appears that tiles are being selected with a much lower zoom level than they should be.

You can also play with camera roll on the globe projection here: https://nathanmolson.github.io/globe_roll/

[HarelM]

Another thing I just thought about is a dedicated control for this.
All other viewport possible changes can be updated using the NavigationControl.
I think there should be some options to allow changing the roll from a dedicated button in the navigation control if the user sets it to true, like the visualize pitch and show compass.
See here:
https://maplibre.org/maplibre-gl-js/docs/API/type-aliases/NavigationControlOptions/
Also, there is a non-interactive map flag that need to support this as well in case there is a control, I think, not sure.

Otherwise, after adding the roll to the spec, updating the spec package here and addressing the above, I think this can be merged.

[NathanMOlson]

Otherwise, after adding the roll to the spec, updating the spec package here and addressing the above, I think this can be merged.

@HarelM Is the NavigationControl update required to merge this? I don't intend to ever use the NavigationControl.

[HarelM]

I believe so, also an example page with this new functionality, preferably with a control the users can interact with.

[NathanMOlson]

OK, Here's a demo of the updated NavigationControl. https://nathanmolson.github.io/camera_roll

1. The compass icon now indicates the roll angle (when visualizeRoll is true).
2. The user can control the roll angle by pressing Ctrl and right-click and dragging.

@HarelM if this is acceptable I will add unit tests and an example page. Or I can extend an existing example (maybe navigation, 3d-terrain, or set-perspective?)

[HarelM]

According to what I tested, I think this needs to be in the opposite direction - i.e. when moving the mouse right I expect the right part to be "lowered".
Not sure if it's exactly here or not.

[NathanMOlson]

I do not think the behavior should change. As currently implemented, at pitch = 0, roll and bearing are the same, and roll control behaves the same as bearing control. Your proposed change would cause identical mouse movements to move bearing and roll in opposite directions, which seems unexpected and confusing.

[HarelM]

I see what you mean about mouse movement for bearing.
If you are looking at the map from the top then dragging above the center of the map behaves "opposite", as if the gesture is expecting to be done below the center of the map to feel intuitive.
This is not the case for mobile as you have two fingers which defines the rotation better.
I believe both roll and bearing mouse gestures needs to be changed to be "better".
Something like taking into consideration the point where the gesture started and calculating the angle from the map center or view center for the rotation.
Here's a movie I made, I was not aware that this is the case, it doesn't make any sense...

Screen.Recording.2024-10-17.at.1.24.36.mov

I don't know if this is out of scope for this PR, but I wouldn't like to pile up on the "bad" behavior.
Since we are still in 5.x pre-release, this can be a breaking change that can be introduced at this point.

[NathanMOlson]

I think that's out of scope for this PR.

[HarelM]

I think the demo looks good!
I've added a few minor comments.
Please make sure to merge main and resolve conflicts.
I think the navigation example should be altered to add pitch and roll visualization and enable roll there as well.

[HarelM]

Please also see if you can update the style-spec package that was just released and remove the as any we talked about.

not ready yet...

[HarelM]

Is it possible to make this a bit more strict somehow?
Maybe by creating an object of type CameraOptions?
Something like:

const coercedOptions: CameraOptions = {
  center: this.style.stylesheet.center,
  ...
}

[NathanMOlson]

That's what I tried at first. I got a ton of unit test failures and I don't understand how they are related, so I copied a working example from camera.ts.

Here's an example of what happens using your approach (there are other failed test suites as well):

                const coercedOptions: CameraOptions = {
                    center: new LngLat(this.style.stylesheet.center[0], this.style.stylesheet.center[1]),
                    zoom: this.style.stylesheet.zoom,
                    bearing: this.style.stylesheet.bearing,
                    pitch: this.style.stylesheet.pitch,
                    roll: this.style.stylesheet.roll
                };

=>

npx jest src/ui/map_tests/map_layer.test.ts --reporters=default
FAIL unit src/ui/map_tests/map_layer.test.ts (75.728 s)
✕ #moveLayer (5003 ms)
✕ #getLayer (5001 ms)
✕ #removeLayer restores Map#loaded() to true (5003 ms)
#getLayersOrder
✕ returns ids of layers in the correct order (5003 ms)
#setLayoutProperty
✕ sets property (5001 ms)
✓ throw before loaded (8 ms)
✕ fires an error if layer not found (5002 ms)
✕ fires a data event (5002 ms)
✕ sets visibility on background layer (5002 ms)
✕ sets visibility on raster layer (5004 ms)
✕ sets visibility on video layer (5004 ms)
✕ sets visibility on image layer (5004 ms)
#getLayoutProperty
✕ fires an error if layer not found (5003 ms)
#setPaintProperty
✕ sets property (5002 ms)
✕ #3373 paint property should be synchronized with an update (5001 ms)
✓ throw before loaded (5 ms)
✕ fires an error if layer not found (5002 ms)

● #moveLayer

thrown: "Exceeded timeout of 5000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  17 | });
  18 |
> 19 | test('#moveLayer', async () => {
     | ^
  20 |     const map = createMap({
  21 |         style: extend(createStyle(), {
  22 |             sources: {

  at Object.<anonymous> (src/ui/map_tests/map_layer.test.ts:19:1)

[NathanMOlson]

@HarelM this works:

                let coercedOptions: CameraOptions = {}
                if (this.style.stylesheet.center != undefined) {
                    coercedOptions.center = new LngLat(this.style.stylesheet.center[0], this.style.stylesheet.center[1]);
                }
                if (this.style.stylesheet.zoom != undefined) {
                    coercedOptions.zoom = this.style.stylesheet.zoom;
                }
                if (this.style.stylesheet.bearing != undefined) {
                    coercedOptions.bearing = this.style.stylesheet.bearing;
                }
                if (this.style.stylesheet.pitch != undefined) {
                    coercedOptions.pitch = this.style.stylesheet.pitch;
                }
                if (this.style.stylesheet.roll != undefined) {
                    coercedOptions.roll = this.style.stylesheet.roll;
                }

Do you like this better than the current pick()? Or do you have any idea how to turn this into something prettier?

[HarelM]

I've looked into it.
I think pick is doing these if in a shorter why and is typesafe enough.
I would like the CameraOptions to be defined before the pick but I see the Stylespec's center is defined as a number[] and the CameraOptions is expecting center to be [number, number].
I think this might be solvable by adjusting the style-spec package exported types.
What do you think?

[HarelM]

In any case, this is not a showstopper for this PR, this is nit picking in the nice-to-have realm.
If you feel like improving this bit feel free to open a PR etc.

[NathanMOlson]

We should probably figure out what to do about center-point elevation before changing the style-spec package exported types. maplibre/maplibre-style-spec#851

[HarelM]

These doesn't have to be tied together, we can improve the style spec typings to supports current implementation and change that when needed in the future.

[HarelM]

Last two comments hopefully before this can get merged.
Thanks a lot for answering my annoying code review comments, I know this can be irritating and I appreciate your openness and patience.