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

Method for cameraoptions from camera and center #1427

Merged
merged 10 commits into from
Aug 27, 2022
Merged

Conversation

BAschl
Copy link
Contributor

@BAschl BAschl commented Aug 3, 2022

As requested in #1414 I created a method to calculate the cameraoptions from a given center and camera, which can then be used in jumpTo, flyTo and so on.

Sadly it's not easy to know if the new center would be visible (or if there's a mountain inbetween), as this requires a redraw of the coordsframebuffer. Also note, that the returned cameraOptions could be beyond the constraints.

Launch Checklist

  • Write tests for all new functionality.
  • Document any changes to public APIs. (You mean doc comments? If so, then yes)
  • Manually test the debug page.
  • Suggest a changelog category: feature
  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

@HarelM
Copy link
Collaborator

HarelM commented Aug 3, 2022

Thanks for taking the time to do this!
Why is API receiving Mercator coordinates and not Lat lng?
Also there's a need to add change log entry in changelig.md.

@BAschl
Copy link
Contributor Author

BAschl commented Aug 3, 2022

Thank you.

It doesn't really matter. If it would take LngLat there would be the need for a 3rd parameter: cameraAltitude.
Should I change it to LngLat?

@HarelM
Copy link
Collaborator

HarelM commented Aug 3, 2022

The general API is around lat lng, so I prefer to keep it this way. I'm not even sure if mercator coordinates is actually exported publicly at this point, so I would feel better if the API will remain with latlng, and latitude.
I know this makes the call signature a bit more dirty, but it avoids the need to use the mercator class just before calling this method. IDK...

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2022

Bundle size report:

Size Change: +258 B
Total Size Before: 204 kB
Total Size After: 205 kB

Output file Before After Change
maplibre-gl.js 195 kB 195 kB +258 B
maplibre-gl.css 9.65 kB 9.65 kB 0 B
ℹ️ View Details No major changes

@BAschl
Copy link
Contributor Author

BAschl commented Aug 3, 2022

Okay, done :)

src/ui/camera.ts Outdated Show resolved Hide resolved
src/ui/camera.test.ts Outdated Show resolved Hide resolved
@prozessor13
Copy link
Collaborator

Thanks for your PR! Very useful :D.

I created a route-flight test-page which uses your method. Basically it works great when using it with jumpTo, but when using flyTo or easeTo the positioning is not correct, which surprised me a lot. Today i do not have time to digg more into this, so for now i send only my test-page. More tests will follow.

Regrads. Max
route.zip
.

@BAschl
Copy link
Contributor Author

BAschl commented Aug 4, 2022

Thank you Max.
I added your debug page to the latest commit and added a click event handler. Alt+click (or alt+ctrl+click) two points to see.

There actually was an error, because I forgot to add the toAltitude after changing to LngLat.
The weird behaviour persists though. When calling easeTo or jumpTo in quick succession it starts the movement and immediatly stops it again when calling it again. Also it's so jumpy because of recalculate zoom.

@prozessor13
Copy link
Collaborator

When comment out:

the flyTo and easeTo works as expected and the route-flight looks much more smooth and natural. Sadly i cant remember why i added the freezeElevation logic into camera.ts, so i have to rethink the freeze logic!

@prozessor13
Copy link
Collaborator

Another interesting usecase of this method would be:

When the camera goes below terrain (f.e. during dragging), rise the camera above terrain, but let it look onto the same center-point.

@HarelM
Copy link
Collaborator

HarelM commented Aug 4, 2022

I think the freeze elevation was due to the fact that the camera was "giggling" when you moved the map around (i.e. the camera would go up when you go up a mountains and go down when you go down to a valley), I'm not sure though...

@prozessor13
Copy link
Collaborator

yes you are right, but the freeze logic is implemented already in handler_manager.ts which handles the user-dragging interactions.
I vaguely remember that the problem had something to do with the swipe feature, especially on mobile devices. Definitely some tests are needed for this!

@BAschl BAschl changed the title Method for bearing and pitch from camera and center Method for cameraoptions from camera and center Aug 4, 2022
@prozessor13
Copy link
Collaborator

created a new pull-request for the freezeElevation logic problem, so from my perspective this pull request is ready for merge.

#1514

test/debug-pages/route.html Outdated Show resolved Hide resolved
@@ -600,6 +600,13 @@ class Map extends Camera {
return this._controls.indexOf(control) > -1;
}

calculateCameraOptionsFromTo(from: LngLat, altitudeFrom: number, to: LngLat, altitudeTo?: number) : CameraOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be tested as well?

@HarelM
Copy link
Collaborator

HarelM commented Aug 22, 2022

Also there's a need to resolve conflicts. Thanks for taking the time to write this!

@BAschl
Copy link
Contributor Author

BAschl commented Aug 22, 2022

I'll take a look at it tomorrow. I've been to a festival the last week and need today to recharge.

@BAschl
Copy link
Contributor Author

BAschl commented Aug 25, 2022

Should be all done now.

I added test cases for calling it on map with a stubbed terrain and additionally I added test cases for the returned zoom. Could you please take a look at it? Should I put constants for expectedZoom?

Furthermore i covered an edge case (From the same as To => division by 0) by throwing an error and wrote a test for it.

src/ui/map.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Aug 25, 2022

Please fix lint.

@BAschl
Copy link
Contributor Author

BAschl commented Aug 26, 2022

What? My local Lint doesn't catch everything? :o

@HarelM HarelM merged commit 6b80a81 into maplibre:main Aug 27, 2022
@birkskyum birkskyum mentioned this pull request Aug 27, 2022
@@ -774,6 +775,41 @@ abstract class Camera extends Evented {
return this.fire(new Event('moveend', eventData));
}

/**
* Calculates pitch, zoom and bearing for looking at @param newCenter with the camera position being @param newCenter
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation needs improvement. The summary refers to params that don't exist. It's confusing that 'from' is referred to as a camera, but 'to' is referred to as a center. If I'm understanding correctly, 'from' is where the camera is located; 'to' is where it's pointing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to open a PR, it's easier to understand it this way :-) Docs can always be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants