Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Add 3D camera API #16419

Merged
merged 7 commits into from
May 2, 2020
Merged

Add 3D camera API #16419

merged 7 commits into from
May 2, 2020

Conversation

mpulkki-mapbox
Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox commented Apr 21, 2020

This pull request is the first iteration of a "real" 3D camera API that gives the user more precise controls of the camera entity itself by exposing direct properties like position and orientation. The difference to the current viewport API is that instead of applying transformations to the map the user has (a limited) access to the underlying camera object that is orbiting around the map center point at a certain distance. Implementation follows standard conventions and is a typical way of controlling the camera in any 3D engine/software.

Both interfaces, the old and the new, are interchangeable and fully compatible with each other: changes made to the transformation via one API is immediately reflected to the other one.

  • Same constraints apply to both APIs. Pitch is limited to 60 degrees, arbitrary camera positions are not allowed and roll-component of the camera orientation is disabled. In other words the camera state is valid on both APIs.
  • Camera/map transformation properties can be modified in parallel with both APIs. There are certain limitations as some properties might have dependencies with each other. For example modifying the zoom using the old API will also change position of the camera seen from the new API.

Interface

Disclaimer: naming of things is not final!

struct FreeCameraOptions {
    /** Position of the camera in mercator coordinates */
    optional<std::array<double, 3>> mercatorPosition;

    /** Focus point on the map where the camera is looking at. Effectively the same value as
        map center coordinate. */
    optional<LatLng> focusPoint;

    /** Helper function for setting the mercator position using Lat&Lng and altitude */
    void setLocation(const LatLng& location, double altitudeMeters);

    /** Helper function for converting mercator position into Lat&Lng and altitude. */
    std::tuple<LatLng, double> getLocation();
};

Only two parameters are currently exposed to the user: position in mercator coordinates and focus point on the map. Focus point is equivalent of the center point in the viewport API and it's used to compute the orientation of the camera. Zoom value can't be set explicitly set as it's computed from the distance between the camera and the focus point.

This interface could be extended to give the user better control of the camera (raw orientation access as a quaternion, view frustum information, projection properties, etc.)

TODO

  • More input validations
  • Add unit tests

@mpulkki-mapbox mpulkki-mapbox added the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 21, 2020
@mpulkki-mapbox
Copy link
Contributor Author

double x, y, z, w;?

s is the scalar component of the quaternion, but it could be w as well. :)

@mpulkki-mapbox
Copy link
Contributor Author

What if user passes scale 0.0, would worldSize be 0.0?
cameraPosition[0] /= worldSize;

This is a very good point. I've added state validations and unit tests to the TODO-list.

@mpulkki-mapbox
Copy link
Contributor Author

Maybe worth introducing new type that will represent location in 3D space? Lat,Lon,Alt?

Adding a proper type for the mercator coordinate is a good idea. Something like mercator_coordinate.js used already in gl-js.

@1ec5
Copy link
Contributor

1ec5 commented Apr 22, 2020

Can’t wait to see a converter between SceneKit’s SCNCamera class and this new FreeCameraOptions struct. 🚀

@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from c0696ae to de33779 Compare April 28, 2020 08:28
@mpulkki-mapbox mpulkki-mapbox requested a review from a team as a code owner April 28, 2020 08:28
@mpulkki-mapbox
Copy link
Contributor Author

Interface update

I've updated the public interface by replacing the focusPoint field with an orientation unit quaternion. Main motivation behind this change is that representing the camera with just position and a focus point is ambiguous in certain scenarios. Having a direct access to the orientation quaternion also allows users to implement different controlling schemes as opposed to having to use only view vectors. Arbitrary quaternions are not supported though as the orientation must be valid for both APIs.

@mapbox/gl-native I'd like to get feedback especially on the FreeCameraOptions struct and on the naming of things!

@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from 6380b70 to bb486ba Compare April 28, 2020 11:13
include/mbgl/map/camera.hpp Outdated Show resolved Hide resolved
include/mbgl/map/camera.hpp Outdated Show resolved Hide resolved
include/mbgl/map/camera.hpp Outdated Show resolved Hide resolved
include/mbgl/map/camera.hpp Outdated Show resolved Hide resolved
src/mbgl/util/camera.cpp Outdated Show resolved Hide resolved
src/mbgl/util/camera.cpp Outdated Show resolved Hide resolved
@chloekraw
Copy link
Contributor

@mpulkki-mapbox @tmpsantos is it ok if we delay merging this until after 1.6.0 is released on Thursday?

@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from 8ed5b9e to 808487b Compare April 28, 2020 19:22
@mpulkki-mapbox
Copy link
Contributor Author

@mpulkki-mapbox @tmpsantos is it ok if we delay merging this until after 1.6.0 is released on Thursday?

@chloekraw yes this is fine by me!

@chloekraw
Copy link
Contributor

@mpulkki-mapbox 👍 thank you! I'll add the label to help us remember and remove after the release is published, even though currently there isn't an approval.

@chloekraw chloekraw added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 29, 2020
include/mbgl/map/camera.hpp Outdated Show resolved Hide resolved
include/mbgl/map/map.hpp Show resolved Hide resolved
src/mbgl/map/map.cpp Show resolved Hide resolved
@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from abde423 to 9e8a210 Compare April 29, 2020 20:13
@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from 7c1ad3a to a133ebd Compare April 30, 2020 11:19
@mpulkki-mapbox mpulkki-mapbox removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Apr 30, 2020
@mpulkki-mapbox mpulkki-mapbox force-pushed the mpulkki-camera-api branch 2 times, most recently from 3bda614 to 974b11e Compare April 30, 2020 18:05
@chloekraw chloekraw removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label May 1, 2020
@mpulkki-mapbox mpulkki-mapbox merged commit 47fb356 into master May 2, 2020
@mpulkki-mapbox mpulkki-mapbox deleted the mpulkki-camera-api branch May 2, 2020 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants