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

WebMercatorViewport supports camera meter offset #235

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

Pessimistress
Copy link
Collaborator

Needed for react-map-gl to support Mapbox terrain. Names are aligned with deck.gl's Viewport class.

Change list:

  • Add position to WebMercatorViewport constructor
  • Add meterOffset field to viewport instance
  • Docs
  • Unit test

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.779% when pulling 197dd55 on x/mercator-viewport-position into fd96311 on master.

| `pitch` | `number` | `0` | The pitch (tilt) of the map from the screen, in degrees (0 is straight down) |
| `bearing` | `number` | `0` | The bearing (rotation) of the map from north, in degrees counter-clockwise (0 means north is up) |
| `altitude` | `number` | `1.5` | Altitude of camera in screen units |
| `position` | `number[]` | `null` | Offset of the camera, in meters |
Copy link
Collaborator

Choose a reason for hiding this comment

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

meterOffset not mentioned?

Perhaps a small section further down in the doc describing how meterOffset and position work, for positioning camera and for following elevation of maps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

meterOffset is a member on the viewport instance. position is an optional prop you pass to the constructor. They are essentially the same value in this use case. Only position is user-facing.

Yes it is confusing. I am only doing this to be compatible with the deck.gl Viewport class because react-map-gl needs to support both.

@@ -28,6 +29,7 @@ export default class WebMercatorViewport {
bearing: number;
altitude: number;

meterOffset: number[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not used?

@@ -76,8 +84,9 @@ export default class WebMercatorViewport {
this.bearing = bearing;
this.altitude = altitude;
this.center = center;
this.meterOffset = position || [0, 0, 0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer if meterOffset and position were kept separate and added internally. I have been viewing position as the camera's relative position in meters.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I am only doing this to be compatible with the deck.gl Viewport class because react-map-gl needs to support both.

Understood. As mentioned, I would still prefer if I could set position independently of map terrain elevation - this makes it seem like this field is being taken over for the elevation purpose when on a terrain map.

If that is not possible, perhaps some confusion could be reduced if we store it as position and offer a getter called meterOffset that just returns it?

I was assuming the meterOffset only needs to be a single number, i.e elevation? But perhaps mapbox expect a vector?

@Pessimistress
Copy link
Collaborator Author

Both position and meterOffset are vec3 like this class's deck.gl counter part. You can see how it is used in react-map-gl: visgl/react-map-gl#1483 essentially position is set to [0,0,elevation].

If you are using this class with Mapbox then it doesn't make sense to set the offset to anything else, for that will cause the cameras to go out of sync.

@Pessimistress Pessimistress merged commit 20b5db4 into master Jun 3, 2021
@Pessimistress Pessimistress deleted the x/mercator-viewport-position branch June 3, 2021 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants