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

Transform cleanup #13452

Merged
merged 3 commits into from
Nov 27, 2018
Merged

Transform cleanup #13452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3630a54
[core] Remove error-prone setlatLng overload
brunoabinader Nov 23, 2018
1b46099
[core] Cleanup Transform, use {jump,ease}To() instead
brunoabinader Nov 23, 2018
34e72c8
[glfw] Use jumpTo() in the route demo
brunoabinader Nov 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions benchmark/util/tilecover.benchmark.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ static void TileCoverPitchedViewport(benchmark::State& state) {
Transform transform;
transform.resize({ 512, 512 });
// slightly offset center so that tile order is better defined
transform.setLatLng({ 0.1, -0.1 });
transform.setZoom(8);
transform.setAngle(5.0);
transform.setPitch(40.0 * M_PI / 180.0);

transform.jumpTo(CameraOptions().withCenter(LatLng { 0.1, -0.1 }).withZoom(8.0).withAngle(5.0).withPitch(40.0));
Copy link
Contributor

@1ec5 1ec5 Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In the past, we avoided designated initializer lists because certain compilers we targeted didn’t support them. Is that still the case, or could we use designated initializer lists instead of the builder pattern?

transform.jumpTo(CameraOptions {
    .center = LatLng { 0.1, -0.1 },
    .zoom = 8.0,
    .angle = 5.0,
    .pitch = 40.0,
});

Both syntaxes provide more clarity than unlabeled constructor parameters, but I find designated initializer lists to be more elegant. We already use designated initializer lists liberally in Objective-C++ code in platform/{darwin,ios,macos}/, since Clang supports them as an extension to C++. For example:

mapbox-gl-native/platform/darwin/src/MGLGeometry.mm

Lines 114 to 119 in 0addd51

MGLMatrix4 mat4 = {
.m00 = array[0], .m01 = array[1], .m02 = array[2], .m03 = array[3],
.m10 = array[4], .m11 = array[5], .m12 = array[6], .m13 = array[7],
.m20 = array[8], .m21 = array[9], .m22 = array[10], .m23 = array[11],
.m30 = array[12], .m31 = array[13], .m32 = array[14], .m33 = array[15]
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Though I'm quite fond of designated initializer lists too, this technique is a bit different as it links chained setters via a reference to the CameraOptions object, but the constructor still remains empty. In C++, support for designated initializers comes in C++20, so it'll take a while until all our supported platforms can get availability on this.

Copy link
Contributor

@1ec5 1ec5 Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For future reference, Clang has supported designated initializers in C++ for years as an extension to the C++ standard. GCC at some point introduced support for “trivial designated initializers”, which means the fields have to come in the same order as they’re declared, like in Swift.

/ref #26 #4722


std::size_t length = 0;
while (state.KeepRunning()) {
auto tiles = util::tileCover(transform.getState(), 8);
Expand Down
7 changes: 7 additions & 0 deletions include/mbgl/map/camera.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ namespace mbgl {
center point when both are set.
*/
struct CameraOptions {
CameraOptions& withCenter(const optional<LatLng>& o) { center = o; return *this; }
CameraOptions& withPadding(const EdgeInsets& p) { padding = p; return *this; }
CameraOptions& withAnchor(const optional<ScreenCoordinate>& o) { anchor = o; return *this; }
CameraOptions& withZoom(const optional<double>& o) { zoom = o; return *this; }
CameraOptions& withAngle(const optional<double>& o) { angle = o; return *this; }
CameraOptions& withPitch(const optional<double>& o) { pitch = o; return *this; }

/** Coordinate at the center of the map. */
optional<LatLng> center;

Expand Down
1 change: 0 additions & 1 deletion include/mbgl/map/map.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ class Map : private util::noncopyable {

// Position
void moveBy(const ScreenCoordinate&, const AnimationOptions& = {});
void setLatLng(const LatLng&, optional<ScreenCoordinate>, const AnimationOptions& = {});
void setLatLng(const LatLng&, const EdgeInsets&, const AnimationOptions& = {});
void setLatLng(const LatLng&, const AnimationOptions& = {});
LatLng getLatLng(const EdgeInsets& = {}) const;
Expand Down
14 changes: 7 additions & 7 deletions platform/glfw/glfw_view.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,18 +261,18 @@ void GLFWView::onKey(GLFWwindow *window, int key, int /*scancode*/, int action,
static double routeDistance = ruler.lineDistance(lineString);
static double routeProgress = 0;
routeProgress += 0.0005;
if (routeProgress > 1.0) routeProgress = 0;
if (routeProgress > 1.0) {
routeProgress = 0.0;
}

double distance = routeProgress * routeDistance;
auto point = ruler.along(lineString, distance);
auto point = ruler.along(lineString, routeProgress * routeDistance);
const mbgl::LatLng center { point.y, point.x };
auto latLng = routeMap->getLatLng();
routeMap->setLatLng({ point.y, point.x });
double bearing = ruler.bearing({ latLng.longitude(), latLng.latitude() }, point);
double easing = bearing - routeMap->getBearing();
easing += easing > 180.0 ? -360.0 : easing < -180 ? 360.0 : 0;
routeMap->setBearing(routeMap->getBearing() + (easing / 20));
routeMap->setPitch(60.0);
routeMap->setZoom(18.0);
bearing = routeMap->getBearing() + (easing / 20);
routeMap->jumpTo(mbgl::CameraOptions().withCenter(center).withZoom(18.0).withAngle(bearing).withPitch(60.0));
};
view->animateRouteCallback(view->map);
} break;
Expand Down
2 changes: 1 addition & 1 deletion platform/ios/src/MGLMapView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1579,7 +1579,7 @@ - (void)handlePinchGesture:(UIPinchGestureRecognizer *)pinch
{
CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate;
self.mbglMap.setLatLng(MGLLatLngFromLocationCoordinate2D(centerCoordinate),
mbgl::ScreenCoordinate { centerPoint.x, centerPoint.y });
mbgl::EdgeInsets { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x });
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this line doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s still adjusting the frame of reference for the center coordinate, but now it does so by padding all four sides so that the padding covers the entire view, instead of setting an anchor point explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - as @1ec5 mentioned, this was previously done inside an overload for setLatLng in Transform that also received an anchor point as a parameter - this overload has been removed in this PR to avoid confusion.

}
}
[self cameraIsChanging];
Expand Down
96 changes: 26 additions & 70 deletions src/mbgl/map/map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,20 +303,11 @@ void Map::moveBy(const ScreenCoordinate& point, const AnimationOptions& animatio
}

void Map::setLatLng(const LatLng& latLng, const AnimationOptions& animation) {
impl->cameraMutated = true;
setLatLng(latLng, optional<ScreenCoordinate> {}, animation);
easeTo(CameraOptions().withCenter(latLng), animation);
}

void Map::setLatLng(const LatLng& latLng, const EdgeInsets& padding, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setLatLng(latLng, padding, animation);
impl->onUpdate();
}

void Map::setLatLng(const LatLng& latLng, optional<ScreenCoordinate> anchor, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setLatLng(latLng, anchor, animation);
impl->onUpdate();
easeTo(CameraOptions().withCenter(latLng).withPadding(padding), animation);
}

LatLng Map::getLatLng(const EdgeInsets& padding) const {
Expand All @@ -325,49 +316,34 @@ LatLng Map::getLatLng(const EdgeInsets& padding) const {

void Map::resetPosition(const EdgeInsets& padding) {
impl->cameraMutated = true;
CameraOptions camera;
camera.angle = 0;
camera.pitch = 0;
camera.center = LatLng(0, 0);
camera.padding = padding;
camera.zoom = 0;
impl->transform.jumpTo(camera);
impl->transform.jumpTo(CameraOptions().withCenter(LatLng()).withPadding(padding).withZoom(0.0).withAngle(0.0).withPitch(0.0));
impl->onUpdate();
}


#pragma mark - Zoom

void Map::setZoom(double zoom, const AnimationOptions& animation) {
impl->cameraMutated = true;
setZoom(zoom, EdgeInsets(), animation);
easeTo(CameraOptions().withZoom(zoom), animation);
}

void Map::setZoom(double zoom, optional<ScreenCoordinate> anchor, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setZoom(zoom, anchor, animation);
impl->onUpdate();
easeTo(CameraOptions().withZoom(zoom).withAnchor(anchor), animation);
}

void Map::setZoom(double zoom, const EdgeInsets& padding, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setZoom(zoom, padding, animation);
impl->onUpdate();
easeTo(CameraOptions().withZoom(zoom).withPadding(padding), animation);
}

double Map::getZoom() const {
return impl->transform.getZoom();
}

void Map::setLatLngZoom(const LatLng& latLng, double zoom, const AnimationOptions& animation) {
impl->cameraMutated = true;
setLatLngZoom(latLng, zoom, {}, animation);
easeTo(CameraOptions().withCenter(latLng).withZoom(zoom), animation);
}

void Map::setLatLngZoom(const LatLng& latLng, double zoom, const EdgeInsets& padding, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setLatLngZoom(latLng, zoom, padding, animation);
impl->onUpdate();
easeTo(CameraOptions().withCenter(latLng).withZoom(zoom).withPadding(padding), animation);
}

CameraOptions Map::cameraForLatLngBounds(const LatLngBounds& bounds, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {
Expand All @@ -380,9 +356,8 @@ CameraOptions Map::cameraForLatLngBounds(const LatLngBounds& bounds, const EdgeI
}

CameraOptions cameraForLatLngs(const std::vector<LatLng>& latLngs, const Transform& transform, const EdgeInsets& padding) {
CameraOptions options;
if (latLngs.empty()) {
return options;
return CameraOptions();
}
Size size = transform.getState().getSize();
// Calculate the bounds of the possibly rotated shape with respect to the viewport.
Expand Down Expand Up @@ -427,33 +402,24 @@ CameraOptions cameraForLatLngs(const std::vector<LatLng>& latLngs, const Transfo
// CameraOptions origin is at the top-left corner.
centerPixel.y = viewportHeight - centerPixel.y;

options.center = transform.screenCoordinateToLatLng(centerPixel);
options.zoom = zoom;
return options;
return CameraOptions().withCenter(transform.screenCoordinateToLatLng(centerPixel)).withZoom(zoom);
}

CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {

if (!bearing && !pitch) {
return mbgl::cameraForLatLngs(latLngs, impl->transform, padding);
}

Transform transform(impl->transform.getState());

if (bearing) {
double angle = -*bearing * util::DEG2RAD; // Convert to radians
transform.setAngle(angle);
}
if (pitch) {
double pitchAsRadian = *pitch * util::DEG2RAD; // Convert to radians
transform.setPitch(pitchAsRadian);

if (bearing || pitch) {
transform.jumpTo(CameraOptions().withAngle(bearing).withPitch(pitch));
}

CameraOptions options = mbgl::cameraForLatLngs(latLngs, transform, padding);
options.angle = -transform.getAngle() * util::RAD2DEG;
options.pitch = transform.getPitch() * util::RAD2DEG;

return options;

return mbgl::cameraForLatLngs(latLngs, transform, padding)
.withAngle(-transform.getAngle() * util::RAD2DEG)
.withPitch(transform.getPitch() * util::RAD2DEG);
}

CameraOptions Map::cameraForGeometry(const Geometry<double>& geometry, const EdgeInsets& padding, optional<double> bearing, optional<double> pitch) const {
Expand Down Expand Up @@ -557,43 +523,33 @@ void Map::rotateBy(const ScreenCoordinate& first, const ScreenCoordinate& second
}

void Map::setBearing(double degrees, const AnimationOptions& animation) {
impl->cameraMutated = true;
setBearing(degrees, EdgeInsets(), animation);
easeTo(CameraOptions().withAngle(degrees), animation);
}

void Map::setBearing(double degrees, optional<ScreenCoordinate> anchor, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setAngle(-degrees * util::DEG2RAD, anchor, animation);
impl->onUpdate();
return easeTo(CameraOptions().withAngle(degrees).withAnchor(anchor), animation);
}

void Map::setBearing(double degrees, const EdgeInsets& padding, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setAngle(-degrees * util::DEG2RAD, padding, animation);
impl->onUpdate();
easeTo(CameraOptions().withAngle(degrees).withPadding(padding), animation);
}

double Map::getBearing() const {
return -impl->transform.getAngle() * util::RAD2DEG;
}

void Map::resetNorth(const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setAngle(0, animation);
impl->onUpdate();
easeTo(CameraOptions().withAngle(0.0), animation);
}

#pragma mark - Pitch

void Map::setPitch(double pitch, const AnimationOptions& animation) {
impl->cameraMutated = true;
setPitch(pitch, {}, animation);
easeTo(CameraOptions().withPitch(pitch), animation);
}

void Map::setPitch(double pitch, optional<ScreenCoordinate> anchor, const AnimationOptions& animation) {
impl->cameraMutated = true;
impl->transform.setPitch(pitch * util::DEG2RAD, anchor, animation);
impl->onUpdate();
easeTo(CameraOptions().withPitch(pitch).withAnchor(anchor), animation);
}

double Map::getPitch() const {
Expand Down Expand Up @@ -768,7 +724,7 @@ void Map::Impl::onUpdate() {
if (mode != MapMode::Continuous && !stillImageRequest) {
return;
}

TimePoint timePoint = mode == MapMode::Continuous ? Clock::now() : Clock::time_point::max();

transform.updateTransitions(timePoint);
Expand Down
Loading