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

Renderer state #13123

Merged
merged 3 commits into from
Oct 19, 2018
Merged

Renderer state #13123

merged 3 commits into from
Oct 19, 2018

Conversation

brunoabinader
Copy link
Member

Introduces a Memento-like RendererState object that will be responsible for fetching renderer state-related data (being camera positioning the first in the list), while preserving the encapsulation of mbgl::UpdateParameters.

This PR also fixes an inconsistency in our public APIs, in which mbgl::CameraOptions should only accept (and outputs) bearing and pitch values in degrees. We should only use such values in radians internally.

/cc @kkaefer @tmpsantos @1ec5 @tobrun

@brunoabinader brunoabinader self-assigned this Oct 17, 2018
@brunoabinader brunoabinader added bug feature Core The cross-platform C++ core, aka mbgl labels Oct 17, 2018
Copy link
Member

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

You're changing the unit of the CameraOptions::angle from radians to degrees, but the comment didn't change:

/** Bearing, measured in radians counterclockwise from true north. Wrapped
to [−π rad, π rad). */
optional<double> angle;

include/mbgl/renderer/renderer_state.hpp Show resolved Hide resolved
platform/android/src/native_map_view.cpp Show resolved Hide resolved
src/mbgl/map/transform.cpp Show resolved Hide resolved
include/mbgl/renderer/renderer_state.hpp Show resolved Hide resolved
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Thank you for streamlining degrees vs radiants, we have a couple of instrumentation tests on Android that depend on this code and they aren't affected. 👍 from my end.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Sorry for the belated review; I was away for a bit. This PR didn’t replace usage of MGLRadiansFromDegrees() or MGLDegreesFromRadians(), which resulted in #13210.

@@ -1970,7 +1970,7 @@ - (MGLMapCamera *)cameraByRotatingToDirection:(CLLocationDirection)degrees aroun
MGLMapCamera *camera;

mbgl::ScreenCoordinate anchor = mbgl::ScreenCoordinate { anchorPoint.x, anchorPoint.y };
currentCameraOptions.angle = degrees * mbgl::util::DEG2RAD;
currentCameraOptions.angle = degrees;
Copy link
Contributor

@1ec5 1ec5 Oct 26, 2018

Choose a reason for hiding this comment

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

This incidentally fixes a missing - that caused -[MGLMapViewDelegate mapView:shouldChangeFromCamera:toCamera:] to get called with an incorrectly rotated “to” camera.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

These accidentally fixed bugs should be noted in the iOS map SDK v4.5.0 and macOS map SDK v0.12.0 changelogs.

platform/darwin/src/MGLMapSnapshotter.mm Show resolved Hide resolved
platform/darwin/src/MGLMapSnapshotter.mm Show resolved Hide resolved
@brunoabinader
Copy link
Member Author

These accidentally fixed bugs should be noted in the iOS map SDK v4.5.0 and macOS map SDK v0.12.0 changelogs.

Glad this helped @1ec5 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants