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

Design Proposal: Add Center Altitude #851

Open
NathanMOlson opened this issue Oct 10, 2024 · 27 comments
Open

Design Proposal: Add Center Altitude #851

NathanMOlson opened this issue Oct 10, 2024 · 27 comments
Labels
enhancement New feature or request

Comments

@NathanMOlson
Copy link
Contributor

NathanMOlson commented Oct 10, 2024

Design Proposal: Add Center Altitude

Motivation

I would like to be able to pitch the camera above 90 degrees.
maplibre/maplibre-gl-js#4717
NathanMOlson/maplibre-gl-js#1

To keep the camera above the terrain, the center point elevation must be above terrain when pitch is > 90. 2D center point is no longer sufficient to place the camera.

Proposed Change

I would like to add a field centerAltitude to the root style spec object. centerAltitude sets the center point elevation in meters above sea level, and joins its friend center.

API Modifications

Root style spec gets a new centerAltitude field. jumpTo(), easeTo(), and flyTo() get elevation added to their options.

Migration Plan and Compatibility

This is a pure extension. To achieve existing functionality, just leave centerAltitude undefined (default 0).

Rejected Alternatives

I considered adding centerAltitude to the API but not to the style spec. Rejected because:

  1. elevation is needed in style spec for render tests.
  2. Similar field center already exists in style spec
@NathanMOlson NathanMOlson changed the title Add Elevation Design Proposal: Add Elevation Oct 10, 2024
@HarelM
Copy link
Collaborator

HarelM commented Oct 10, 2024

Render tests can work with methods defined in the render harness and in the map API (check out wait and other "special" operations).
It is not clear what elevation stands for as currently zoom basically defines the elevation.
It's not perfect, but adding a parameter that may confuse the user is not ideal...

@NathanMOlson
Copy link
Contributor Author

Understood about the render tests.

The elevation proposed here corresponds to map.transform.elevation, which is the elevation of the center point (map.transform.center).

For pitch angles >= 90 degrees, if the center point is at the same elevation as the map, the camera will be below the map. To allow the camera to be above the map, it must look at a center point which hovers above the map (map.transform.elevation must be greater than the elevation of the map, which is zero in the absence of terrain).

@HarelM
Copy link
Collaborator

HarelM commented Oct 10, 2024

Is it possible to add a third number to center?

@HarelM HarelM added the enhancement New feature or request label Oct 21, 2024
@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

Is center the location of the camera or is it the location of where the camera looks at?

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

I know center and zoom are already part of the style spec so we will not remove them, but in my opinion they should never have been added. I think the style sheet should encode how the map looks like, and the MapLibe GL JS / Native API should take care of what you do with the map, i.e., where to look at, zoom level, field of view etc...

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2024

Center is where the camera is looking at.
I respectfully disagree about the fact that center shouldn't be part of the spec.
Consider the case where you want to present a map of a location (for example a web page with a business showing where it resides).
Setting zoom, center, pitch, bearing allows a designer to indicate exactly how the map should look without needing to change a single line of code.

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

OK, so

  • center is the location of the point where the camera is looking at
  • elevation is the elevation of the point the camera is looking at

More question are:

  • is the elevation referenced to sea level or to the height of the 3d terrain under the center point?
  • What is the relation between center, elevation, and zoom?

I have a feeling that you could already emulate what elevation should do by moving the center point and adjusting the zoom level...

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

And would it not make more sense to introduce properties for the camera location and view direction?

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2024

I've asked the same questions basically. Some have been answered here:
maplibre/maplibre-gl-js#4851
For pitch > 90 elevation is a must.
Relevant PR about pitch support is here:
#860
Let me know if this answers your questions.

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

Ok thanks for the links. I think I understand the math part now. But isn't this all very backward? I mean if I remember correctly the use case that you would like to support @NathanMOlson is first-person-view of a pilot flying through a valley. I think it would be much more intuitive as a user interface to provide camera location and view direction. The plain has a gps position and a heading, that's what you should give to maplibre...

@HarelM
Copy link
Collaborator

HarelM commented Oct 21, 2024

Since this is not the main use case for maplibre, we agreed these APIs can be added as a plugin code (external code) as these are basically converters from camera location to camera target.

@ibesora
Copy link
Collaborator

ibesora commented Oct 21, 2024

@wipfli trying to answer some of your questions:

is the elevation referenced to sea level or to the height of the 3d terrain under the center point?

It should be elevation from the sea level IMO, otherwise camera pitch would need to change when moving the map.

What is the relation between center, elevation, and zoom?

elevation is the missing coordinate from center. Changing zoom should only change the distance between the camera position and center, not change the center position.

I think it would be much more intuitive as a user interface to provide camera location and view direction

View direction will not be enough for use cases like this where you want to orbit around a fixed point not set on the terrain unless Maplibre implicitly computes the intersection between view direction and objects in the scene. Even then, there might be use cases where you want the camera to orbit around a point in space that doesn't sit on any object.

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

Zoom influences the distance between camera and center point, and it sets the level of detail of the map. I think this will cause confusion for first person view scenarios. For the same camera location and view direction you can

  • use high zoom and a center point close to the camera
  • use a low zoom and a center point far from the camera
    Right?

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

Have you considered setting the map center point to behind the camera when pitch is larger than 90 degrees?

@wipfli
Copy link
Contributor

wipfli commented Oct 21, 2024

...I guess that would not work to control elevation at pitch = 90 deg.

Anyway, I think both an elevation root property or three-value center are fine to solve this issue.

Regarding showing a locator map, I think that is a perfect example of where you would use 1 style.json and deploy it to all locator maps you have. The style defines how the map looks like. Then an api call puts a marker at a store location and another api call sets zoom and map center.

@NathanMOlson
Copy link
Contributor Author

Zoom influences the distance between camera and center point, and it sets the level of detail of the map. I think this will cause confusion for first person view scenarios. For the same camera location and view direction you can

  • use high zoom and a center point close to the camera
  • use a low zoom and a center point far from the camera
    Right?

Correct. To avoid this confusion, I plan to create a plugin that will convert camera location and view direction to center point.

In my use case, zoom will never be interacted with directly. Instead, only _fov (the vertical field of view of the camera) will be directly controlled, and zoom will be computed from the other variables.

@NathanMOlson
Copy link
Contributor Author

Have you considered setting the map center point to behind the camera when pitch is larger than 90 degrees?

Yes, I considered that. I rejected it because it would require the introduction of "negative distance" (camera to center distance is currently always positive, and closely coupled to zoom). Also, it would introduce ambiguity when pitch is exactly 90 degrees.

@AbelVM
Copy link

AbelVM commented Oct 22, 2024

Center is where the camera is looking at.

Not only that. It also defines the rotation axis origin, so when the user navigates and changes pitch or bearing through mouse or touch interactions, all of those rotations use the center as the reference point for rotations. And perspective is tricky then.

That's why using 3DTiles is a pain in the ass, as the UX turns messy. When looking at 3D point a, the rotations are made around 2D c point, that, depending on pitch, might be kilometers away. And the results are crazy. I'm not sure if this is fixed for terrain elevation

image

The original discussion as reference : https://osmus.slack.com/archives/C03TFH5NE83/p1720105222054479

@ssokol
Copy link

ssokol commented Oct 22, 2024

It's a bit pedantic, but elevation refers to the height of terrain and surface features above a datum - typically the median sea level (MSL). What we're describing is actually 'altitude' referenced to that datum. I don't know that it matters much, but it might save a bit of confusion for someone at some point.

@HarelM posted on Slack a question about changes to the 'center' value - should we add a 3rd parameter to center that represents altitude or should we add a new and independent altitude parameter.

Center, as discussed above, determines what we're looking at. Adding an altitude parameter to that doesn't really make sense - all it would do is change the focal point to focus on empty space above the map or solid rock within it.

The altitude parameter applies primarily to the virtual camera in a "camera-centric" mode. In the current implementation, center determines camera placement. In a camera-centric mode, camera placement and angles determine the center. Use of both 'center' and 'zoom' in a camera-centric mode should probably be disabled, replaced by positioning calls (jumpTo, flyTo, easeTo) or simply raw camera object parameters (altitude, location, azimuth, pitch, roll).

I would tend to agree that the API and any user controls / keyboard mappings for the camera centric mode should be done as a plug-in as it is not part of the primary MapLibre use case.

So to answer @HarelM's question, I tend to think altitude should be its own field rather than an extension to center.

For background, take a look at both the enhancement request and also the detailed set of use cases for the associated bounty. There's also a working demo based off of @NathanMOlson's work-in-progress efforts that showcases what I'm trying to accomplish fairly well.

@HarelM
Copy link
Collaborator

HarelM commented Oct 22, 2024

From the implementation of the support for pitch > 90 it does change whete the camera is looking at, which is the center.
It also helps solving the case where you would like to rotate the view/camera around the top of a building.
maplibre/maplibre-gl-js#4851
So for me it does sound like it is part of the definition of the center...

@ssokol
Copy link

ssokol commented Oct 22, 2024

Hmm... I think we might need more than just center + altitude to meet the "orbit a building" use case using the current 3rd party PoV mechanics.

You could use the 3 parameter center setting (lon, lat, alt) to place the focal point at the center of a building 30 stories above the ground (~30m + base elevation). To look at the building straight-on, you would set the pitch to 90°. You could then cause the camera to orbit the building by adjusting the bearing value.

But... The current implementation basically casts its "gaze" all the way to the horizon. To perform an orbit of the building with the building in focus wouldn't you also need to be able to specify the focal distance - the distance from the building center point to position the camera? And if the building was big enough, you might need a way to differentiate between the distance from the center point (the Y axis around which the camera orbits) and the building face (what you want the camera to show in focus).

Is that feasible, and if so, what would that parameter be? Does it fit in this particular design prop, or should there be a separate one for this use case?

For whatever it's worth, I think you could solve the "orbit a building" case using the 1st person PoV (camera-centric) approach, especially if you add in the motion control work that @samarth is focused on. In that case you would position the camera at a distance from the building at the desired altitude and set the linear, lateral, and azimuth motion parameters to track the center point.

@HarelM
Copy link
Collaborator

HarelM commented Oct 22, 2024

@ssokol
Copy link

ssokol commented Oct 22, 2024

Ah, got it. Apologies...

I have to keep reminding myself that most devs and end users need / prefer 3rd person PoV.

I guess it's the difference between a movie (where the director and cinematographer determine what will be photographed and from there decide how to set up the camera) and real life (where your camera / eyes are where they are and what you see is a result of that).

@HarelM
Copy link
Collaborator

HarelM commented Oct 27, 2024

We need ro conclude this discussion in order for the pitch PR to be merged.
@louwers , @ibesora do you have a preference for this?

  1. Update the center definitions
  2. Add a new property called elevation
  3. Don't add anything, only allow modification though API calls

@louwers
Copy link
Collaborator

louwers commented Oct 29, 2024

I like adding another root property slightly better because it has better backwards compatibility.

A bit pedantic, but I think it should actually be altitude?

image

Edit: Just read back and saw @ssokol made exactly the same point.

@ibesora
Copy link
Collaborator

ibesora commented Oct 29, 2024

+1 to calling it altitude if it's the correct term

Don't add anything, only allow modification though API calls

I would prefer NOT to do this and allow the user to define a style where they can already set altitude.

Update the center definitions
Add a new property called elevation

Between those two, I think the easiest would be adding a new property. Although having center being 3 values is more concise, it's also kind of weird having two of them being coordinates and the third one meters from the sea level.
My vote goes for adding a new property.

@HarelM
Copy link
Collaborator

HarelM commented Oct 29, 2024

@NathanMOlson I think this settles this.
You can proceed with a PR to resolve this. Don't forget to add an issue in native if one doesn't exist.

@NathanMOlson NathanMOlson changed the title Design Proposal: Add Elevation Design Proposal: Add Center Altitude Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants