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

fix missing repeats of CanvasSource when it crosses the antimeridian #7273

Merged
merged 3 commits into from
Sep 12, 2018

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Sep 12, 2018

This fixes #7271 by including an extra copy of image sources and canvas sources on either side of the world. In almost all cases this extra copy is necessary, but it shouldn't add any overhead because rendering two offscreen triangles should be negligible.

It might be slightly nicer to have an approach that actually calculates how many copies we need to render but I think this would be significantly more complex and probably slower.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

awesomeeee! thanks @ansis
let's get this backported into release-frappe as well 🎉

@ryanhamley
Copy link
Contributor

ryanhamley commented Sep 12, 2018

This is probably a major edge case, but I noticed this behavior while randomly playing around with the map.

screen shot 2018-09-12 at 10 27 22 am

screen shot 2018-09-12 at 10 43 47 am

In fact, it happens on master too so it's an existing issue that is not tied to this change. Should this be addressed with this PR or maybe just ticket it and investigate it separately?

In normal usage, this PR seems to work like a charm.

@ansis
Copy link
Contributor Author

ansis commented Sep 12, 2018

@ryanhamley good catch, a54e774 adds a fix for that as well

const utl = this.pointCoordinate(new Point(0, 0), 0);
const utr = this.pointCoordinate(new Point(this.width, 0), 0);
const ubl = this.pointCoordinate(new Point(this.width, this.height), 0);
const ubr = this.pointCoordinate(new Point(0, this.height), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how this fixes the rotation issue, @ansis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using all four corners is necessary since the leftmost/rightmost points could be any of the corners. The following two lines with the Math.min and Math.max handle the case when the map is upside down

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.

Canvas source doesn't render in world repeats at edges
3 participants