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.

Sign up for GitHub

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

Jump to bottom

make flyTo check if it can fly to worldCopies #4459

Merged
merged 4 commits into from
Mar 22, 2017
Merged

make flyTo check if it can fly to worldCopies #4459

merged 4 commits into from
Mar 22, 2017

Conversation

CrokinoleMaster
Copy link
Contributor

@CrokinoleMaster CrokinoleMaster commented Mar 20, 2017 •
edited
Loading

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page

Issue is described here: #4449.
For some reason I just get a Not Found when I try the debug page.

mollymerp
mollymerp reviewed Mar 20, 2017
View reviewed 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.

Thank you for contributing @huaruiwu.
I manually tested the debug page (yarn start) and can confirm this fixes the linked issue!
There is still an issue where calling map.flyTo({center:[lng greater than 180, lat]}) will cause the map to fly to an unrendered world.

@jfirebaugh @lucaswoj do you think we should normalize all calls to flyTo in unrendered worlds, or fire a warning of some sort?

@andrewharvey
Copy link
Collaborator

@mollymerp With the changes I've proposed in #4451 calling flyTo to another world, it should wrap back to world 0. I'll do a quick manual test to confirm this.

@andrewharvey
Copy link
Collaborator

For some reason I just get a Not Found when I try the debug page.

@huaruiwu when yarn start starts the server the main debug page is at http://localhost:9966/debug/debug.html.

@mollymerp
Copy link
Contributor

@andrewharvey great thank you! #4451 takes care of my earlier comment.

@huaruiwu sorry I missed your comment about the debug page - the default is localhost:9966/debug/ and you can access the various other pages in the debug/ folder by adding those filenames to the url as @andrewharvey suggested

@CrokinoleMaster
Copy link
Contributor Author

@mollymerp @andrewharvey Thanks! It does say localhost:9966/debug/ in the documentation but I just didn't see it.

@andrewharvey
Copy link
Collaborator

andrewharvey commented Mar 21, 2017 •
edited
Loading

@mollymerp With the changes I've proposed in #4451 calling flyTo to another world, it should wrap back to world 0. I'll do a quick manual test to confirm this.

@mollymerp @huaruiwu I made some changes to #4451 to deal with a false renderWorldCopies, It will currently exit the main world renderWorldCopies is false but you call flyTo with coordinates in another world.

I think this circles back to your original question @mollymerp. What should the expected behaviour be here? I don't think it blocks this PR since it's a separate issue. I'm leaning towards leaving it as is, flyingTo to the white space. API users can always wrap their input to flyTo to prevent this. Leaving as is means that a box zoom outside the main world zooms in to that white area which I think is best.

@andrewharvey andrewharvey mentioned this pull request Mar 21, 2017
Merged
5 tasks
jfirebaugh
jfirebaugh suggested changes Mar 21, 2017
View reviewed changes
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks @huaruiwu. Just a couple of code quality issues to take care of.

test/unit/ui/camera.test.js Outdated
@@ -1011,6 +1011,46 @@ test('camera', (t) => {
camera.flyTo({ center: [170, 0], duration: 10 });
});

t.test('does not pan eastward across the antimeridian if no world copies', (t) => {
const camera = createCamera();
camera.transform._renderWorldCopies = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please adjust the test so that it sets this option via the public API (an argument to the Transform constructor) rather than a private property.

src/ui/camera.js Outdated
@@ -667,7 +667,7 @@ class Camera extends Evented {

// If a path crossing the antimeridian would be shorter, extend the final coordinate so that
// interpolating between the two endpoints will cross it.
if (Math.abs(tr.center.lng) + Math.abs(center.lng) > 180) {
if (tr._renderWorldCopies && Math.abs(tr.center.lng) + Math.abs(center.lng) > 180) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a public renderWorldCopies getter on Transform rather than using an underscored (private) property.

@CrokinoleMaster
Copy link
Contributor Author

@jfirebaugh Thanks for the review! updated

@jfirebaugh jfirebaugh mentioned this pull request Mar 21, 2017
Closed
@jfirebaugh jfirebaugh merged commit 9941dde into mapbox:master Mar 22, 2017
@CrokinoleMaster CrokinoleMaster deleted the flyto-worldcopies branch March 22, 2017 00:46
@CrokinoleMaster
Copy link
Contributor Author

@jfirebaugh thanks for the merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfirebaugh jfirebaugh jfirebaugh approved these changes

@mollymerp mollymerp mollymerp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CrokinoleMaster @andrewharvey @mollymerp @jfirebaugh