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

Rework Camera#easeTo to address problems with pitch #3130

Closed
wants to merge 3 commits into from

Conversation

yeldarby
Copy link
Contributor

@yeldarby yeldarby commented Sep 1, 2016

Description

Fixes #3112 and #3119

Previously, when the map was pitched panning caused unexpected easing and relative adjustments. Pitching the map should not affect easing or the final location of panBy.

This is fixed by calculating the finish state of the map prior to the transition beginning and interpolating from start state to end state accordingly.

One catch: when zooming you don't want to zoom and position to interpolate together since zoom affects the distance between start and end on the screen (so there's an adjustment to take that into account).

Launch Checklist

  • briefly describe the changes in this PR
  • post benchmark scores - not sure how to do this?
  • manually test the debug page
  • get a PR review

</script>
<div id="centerpoint"><div id="dot"></div></div>
</body>
</html>
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense that you built a debug page to use while developing this feature.

Because repository size, simplicity, and stability is important to us, we would prefer not to merge this file into master. Can you please remove this file from the PR?

endTransform.resize(tr.width, tr.height);
endTransform.zoom = zoom;
endTransform.bearing = bearing;
endTransform.pitch = 0; // fixes #3119 by pretending the map is not pitched; use endTransform.pitch = pitch to revert to the old behavior
Copy link
Contributor

@lucaswoj lucaswoj Sep 14, 2016

Choose a reason for hiding this comment

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

Let's be mindful of how using Transform in this way affects the architecture of GL JS.

This is a new way to use the Transform class. Transform has all sorts of functionality that isn't being used at this call site. Up until now, we have had exactly one Transform per instance of Camera and the two classes have been tightly coupled together. This change further complicates the role of Transform and its coupling to other modules.

Here is a proposal for a quick refactoring Transform into a few different pieces to enable this use case and improve our overall system architecture:

  • use an object that implements the CameraOptions interface in place of Transform
  • factor out all map size state into Camera or Map
  • factor out all other functionality in Transform into separate util functions which take CameraOptions as an argument

EDIT: See comment below 👇

Does this make sense?

ref #2801

Copy link
Contributor

@lucaswoj lucaswoj Sep 14, 2016

Choose a reason for hiding this comment

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

Oh! I have another idea that'll require much less work:

  • factor Trasform#pointLocation into a standalone utility function with the interface
function pointLocation(
    point: Point,
    viewport: CameraOptions & {width: number, height: number}
)

@lucaswoj
Copy link
Contributor

I am closing this PR because it has been inactive for a long time. The branch isn't going anywhere so please keep working on this feature and re-open the PR when it is ready!

Let me know if you have any questions.

@lucaswoj lucaswoj closed this Nov 22, 2016
joswinter added a commit to delftswa2017/mapbox-gl-js that referenced this pull request Feb 17, 2017
Continues on changes in mapbox#3130 pull request
joswinter added a commit to delftswa2017/mapbox-gl-js that referenced this pull request Feb 21, 2017
Continues on changes in mapbox#3130 pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure camera animations interpolate geographic coordinates rather than screen coordinates
2 participants