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

Added a 'wrapAroundWorld' option to the mapboxgl.Map constructor #3885

Merged

Conversation

Que3216
Copy link
Contributor

@Que3216 Que3216 commented Jan 4, 2017

If wrapAroundWorld is set to false, then only one copy of the world will be rendered, when zoomed out. This solves issue #3785.

If `false`, only one copy of the world will be rendered, when zoomed out.
@@ -316,7 +316,8 @@ class SourceCache extends Evented {
minzoom: this._source.minzoom,
maxzoom: this._source.maxzoom,
roundZoom: this._source.roundZoom,
reparseOverscaled: this._source.reparseOverscaled
reparseOverscaled: this._source.reparseOverscaled,
wrapAroundWorld: this.style ? this.style.wrapAroundWorld : true
Copy link
Contributor

Choose a reason for hiding this comment

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

Can wrapAroundWorld be a property on transform (the update method parameter), so that Style doesn't have to be involved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! That's much nicer. I'll update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh I've updated it -- let me know if this looks good now.

@Que3216
Copy link
Contributor Author

Que3216 commented Jan 4, 2017

@jfirebaugh Is this good to merge now?

@jfirebaugh
Copy link
Contributor

The implementation looks good. Before merging I want to brainstorm a few other options for naming in hopes that we can find something more precise and self-descriptive than wrapAroundWorld, which could be confused with options such as described in #2507 and #2071.

  • In Leaflet, the equivalent option is the TileLayer noWrap option.
  • Keeping it a global option: renderWorldCopies or renderWrappedWorlds.

@lucaswoj
Copy link
Contributor

lucaswoj commented Jan 4, 2017

Looks good! I can see two big picture items before we 🚢 this.

Todo before merging

  • decide on a name ☝️
  • add a render test

@Que3216
Copy link
Contributor Author

Que3216 commented Jan 5, 2017

I think it makes more sense as a global option. I like renderWrappedWorlds. @lucaswoj / @jfirebaugh - are you happy to go with renderWrappedWorlds?

@Que3216
Copy link
Contributor Author

Que3216 commented Jan 5, 2017

Looks like render tests in mapbox-gl-test-suite are currently setup to just use a style.json, and pass selected data through to the map constructor via style.metadata. To support renderWrappedWorlds (which is a map constructor option), we'd have to add a renderWrappedWorlds option to style.metadata and make suite_implementation.js handle it.

Let me know if you're good to go on the name and I can add in a render test.

@jfirebaugh
Copy link
Contributor

I think renderWorldCopies makes more sense than renderWrappedWorlds. LngLat#wrap() establishes that a "wrapped" longitude is one that is guaranteed to be in the range (-180, 180). This implies there's only one wrapped world, and it's the one that this option preserves, as opposed to the "world copies" that it disables.

If you rename to renderWorldCopies I'm ready to approve. I'm okay with not having a render test for this, since it's not a style property and there is a unit test.

@Que3216
Copy link
Contributor Author

Que3216 commented Jan 6, 2017

@jfirebaugh Ok cool! That makes sense. I've renamed it to renderWorldCopies.

@jfirebaugh jfirebaugh merged commit f7da634 into mapbox:master Jan 6, 2017
@cammanderson
Copy link
Contributor

Hi Guys,

With this new render option, is it possible to expose an API to toggle this behaviour opposed to re-instantiating the map? Or is it necessary? I can see some problems where the lnglat of the current camera our out of bounds due to world wrapping.

Just looking at adding this function to our react library react-map-gl-alt but a goal of the project is to be able to configure the map through external exposed props - which means that the developer would have the option to toggle.

Thanks guys!
Cameron

@CrokinoleMaster
Copy link
Contributor

Tried using flyTo with renderWorldCopies: false and map will try to fly to the closest copy coordinate even if that world is not being rendered.

@mollymerp
Copy link
Contributor

hmm @huaruiwu that sounds like a bug -- do you want to open a fresh ticket to report?

@CrokinoleMaster
Copy link
Contributor

@mollymerp Sure! #4449

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.

6 participants