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 mapbox getView scale for Retina display #6039

Merged
merged 3 commits into from Dec 3, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 23, 2021

cc: https://github.com/plotly/dashboard-engine/issues/828

Tested with Safari on iPad Pro 12.9 2021

@plotly/plotly_js

@alexcjohnson
Copy link
Contributor

@archmoj Can you give me a test case that would break previously but works with this PR? I have a mac with both retina and non-retina monitors so ideal to verify this with ;)

I wonder if this will work correctly with maps with pitch - in that case lower areas on the map have a larger scale than those higher up, so the midpoint of the diagonal won't match the center lat/lon.

Could we not just use window.devicePixelRatio?

> window.devicePixelRatio // with the window on my big non-retina monitor
<- 1
> window.devicePixelRatio // after moving the window to my built-in laptop screen (retina)
<- 2

@archmoj
Copy link
Contributor Author

archmoj commented Dec 1, 2021

@archmoj Can you give me a test case that would break previously but works with this PR? I have a mac with both retina and non-retina monitors so ideal to verify this with ;)

I wonder if this will work correctly with maps with pitch - in that case lower areas on the map have a larger scale than those higher up, so the midpoint of the diagonal won't match the center lat/lon.

Could we not just use window.devicePixelRatio?

> window.devicePixelRatio // with the window on my big non-retina monitor
<- 1
> window.devicePixelRatio // after moving the window to my built-in laptop screen (retina)
<- 2

Good call. I agree we should try pixelRatio.

@archmoj
Copy link
Contributor Author

archmoj commented Dec 1, 2021

@alexcjohnson you could apply your pixelRatio suggestion on this PR now that you also have the right monitors to debug and test.
Thanks!

@alexcjohnson
Copy link
Contributor

Sure, happy to take over this PR if you can give me a clear test case :)

@archmoj
Copy link
Contributor Author

archmoj commented Dec 2, 2021

Sure, happy to take over this PR if you can give me a clear test case :)

Great. This is what I did when testing on browserstack. I added a cosole log in the if statement which tests the center longitude and inspect if it is between start and end longitudes.
If we apply pixelRatio to the width and the height of the canvas it should just work without needing that if statement.

The width and height attributes are expanded by devicePixelRatio,
but the style width and height are in css pixels.
var w = canvas.width;
var h = canvas.height;
var w = parseInt(canvas.style.width);
var h = parseInt(canvas.style.height);
Copy link
Contributor

Choose a reason for hiding this comment

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

When you make a canvas, the way to give it correct resolution for a retina display is to set the style width and height to the css pixel size, and the width and height attributes to that size times window.devicePixelRatio - so we just needed to switch to style.

@archmoj
Copy link
Contributor Author

archmoj commented Dec 3, 2021

Nicely done.
💃

@alexcjohnson alexcjohnson merged commit 394fcbd into master Dec 3, 2021
@alexcjohnson alexcjohnson deleted the mapbox-getview-scale branch December 3, 2021 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants