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

Change altitude of look at point? #3683

Closed
Philip2809 opened this issue Feb 12, 2024 · 12 comments · Fixed by #4851
Closed

Change altitude of look at point? #3683

Philip2809 opened this issue Feb 12, 2024 · 12 comments · Fixed by #4851
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed

Comments

@Philip2809
Copy link
Contributor

User Story

As a developer where I am making 3d buildings with toggleable floors I would benefit from having a way of changing the altitude of camera look at/rotation around point, I think it is called the focal point of the camera.

Rationale

Because having a 3d map with toggleable floors, either tough extrusions or three js, it would be nice to be able to change the altitude of the camera look at point. I know this is possible today because of the terrain and when you have different altitudes in the terrain this works. I would appreciate if someone who is more familiar with the codebase could help me find where this calculations of the altitude happens and maybe I can work to implement something myself from there.

Impact

Would make 3d worlds more enjoyable and easier to navigate for everyone!

@HarelM
Copy link
Collaborator

HarelM commented Feb 12, 2024

What's preventing you from doing it now?

@HarelM HarelM added the need more info Further information is requested label Feb 12, 2024
@Philip2809
Copy link
Contributor Author

I can't find where I would do this today?

@HarelM
Copy link
Collaborator

HarelM commented Feb 12, 2024

This is probably similar to what's here, maybe with a few different parameters:

calculateCameraOptionsFromTo(from: LngLat, altitudeFrom: number, to: LngLat, altitudeTo: number = 0): CameraOptions {

@Philip2809
Copy link
Contributor Author

Does this change the altitude of the focal point?

@HarelM
Copy link
Collaborator

HarelM commented Feb 12, 2024

Not sure I understand the question. Play with it and see if it helps.

@Philip2809
Copy link
Contributor Author

Managed to figure out what I wanted in more detail. In the transform.ts file, If I set the this.elevation and this._elevation to 180. (hard coded to demonstrate) I get exactly what I want:
firefox_aNUR1U5dR7
Rotation around a point that is above the ground, Is this called the pivot point?
May I make a pull request where you can set an additional variable to "add" more elevation like this?
I am not very familiar with the codebase but an option to when displaying "fill-extrusion" could be to automatically change this value when your "camera look at point" (pivot point?) is above a building? But just a feature were I can change this value and the height of your camera automatically would be awesome when making toggleable floors on a 10+ floor building

@HarelM
Copy link
Collaborator

HarelM commented Feb 14, 2024

This demo looks nice!
I'm not sure I'd know how to properly solve this in terms of API.
There are many places where altitude/elevation can be set/added: flyto, easeto, rotate, zoom, pan, etc.
There are some API currently that support some similar things (like rotate no around the center of the map), but I think it requires a lot of digging in order to properly solve all the aspects of this.
Generally speaking, I think a design document/post for this would be optimal before rushing to implementation.

While I'm sure you already know that, changing the elevation in the transform class can lead to a lot of unexpected behaviors, and while it might solve this specific case, it might create problem with other features.

@HarelM HarelM added enhancement New feature or request PR is more than welcomed Extra attention is needed and removed need more info Further information is requested labels Feb 14, 2024
@Philip2809
Copy link
Contributor Author

Yes, I was thinking about that, changing this in the transform class might cause a lot of problems in other places. There is some automated tests today that you can run to test this? For a bigger feature like this, a design post/document would be smart, where do you have that stuff today?

@HarelM
Copy link
Collaborator

HarelM commented Feb 14, 2024

There are a lot of tests, opening a PR would run them all and you can look into the readme scattered in this repo to better understand how to run all kind of tests.
Regarding design, you can either update the initial post in this issue, or create a PR with the design (although a PR is usually after you write some code, but you can create a draft PR I guess).
There are also the docs in the developer guides, I don't know if this is the right place, there are some design documents there...
In any case, the important thing is the design, we can later on decide where to put it.

@Philip2809
Copy link
Contributor Author

Played around with it a bit more and found this so far:
instead of using the ._elevation and .elevation in the _calcMatrices function I make a variable "elevation", add my pivot elevation and then use this in the function in place of the elevation. This seams to work perfectly where the elevation is 0, in a city and stuff, but when trying this on the terrain example it falls apart. If you go to the example:
https://maplibre.org/maplibre-gl-js/docs/examples/3d-terrain/
There is already a little "jump" when moving around, zoom in far around a mountain. Another bug that is there today is when you don't move but hold the right mouse button to pan down and then the map center moves. Both of these bugs are exaggerated when adding more height with this pivotelevation.

Issue on terrain example today:
firefox_wZ21OOj1St
Load in, get center, hold right mouse button, move down, repeat a couple of times this happens:
image

As said before, these or other issues are exaggerated when I add more altitude and I get some different bugs depending on if I set add the elevation variable or only use the pivotelevation in the _calcMatrices function.

@HarelM
Copy link
Collaborator

HarelM commented Feb 15, 2024

Yes, we have some bugs related to zoom and terrain unfortunately which appear to be hard to solve, see:

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2024

I think the following PR will allow this requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR is more than welcomed Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants