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 canvas size to round up to container size #10936

Merged
merged 13 commits into from
Sep 8, 2021

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Aug 16, 2021

Closes #9455.

Previous bug demonstrated that canvas pixel size would round down from container size, leaving a small gap between the difference of the two sizes (detectable on some retina and mobile displays as per the issue).

clientWidth and clientHeight were previously being used to calculate the size for the canvas which as per the documentation always rounds the value to an integer. Replacing these with getBoundingClientRect() allows decimal values.

EDITED LATER TO ADD: All references to clientWidth and clientHeight have been replaced with getBoundingClientRect in this PR. getBoundingClientRect provides a more exact value which will improve accuracy for calculations.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog> Rounds canvas size up to container size </changelog>

@avpeery avpeery marked this pull request as draft August 16, 2021 21:57
@avpeery avpeery self-assigned this Aug 16, 2021
@rreusser
Copy link
Contributor

rreusser commented Aug 17, 2021

Makes sense. But hmm… the unit tests seem suspiciously far off for a subpixel difference in canvas size. (e.g. 584 --> 72 in the example belolw). I wonder if something works differently when the tests are run in node.

test/unit/ui/popup.test.js .......................... 92/99
  Popup is positioned at the specified LngLat in a world copy
  not ok should be equivalent
    --- wanted
    +++ found
     {
    -  "x": 584
    +  "x": 72
       "y": 150
     }

I'm also not quite 100% clear on what the original example does. Is the author setting the map to the full page size? Is it possible maybe the burden of scaling the canvas to fill the full page without a gap may be on the developer and not the library?

@avpeery
Copy link
Contributor Author

avpeery commented Aug 17, 2021

@rreusser Thanks for taking a look! The PR is still in progress.. When I use the debug page getBoundingClientRect works but does not work in the tests so it is using the default values instead. I'm currently looking into why this is.

@rreusser
Copy link
Contributor

Ah, now I see the draft status. Sorry to jump the gun. Carry on 😄👍

@avpeery
Copy link
Contributor Author

avpeery commented Aug 17, 2021

@rreusser Thanks for checking it out though! That is a good thought to discuss though on if it is something we should be handling. I can only "see" this bug when the canvas isn't full-screen (I'm not sure what the specific use-cases are for someone having a canvas not full-screen). I fixed the unit tests (was missing a mock for getBoundingClient Rect). I discussed with @arindam1993 keeping the canvas.style.width and canvas.style.height as fractional values, but rounding up the canvas size.

@avpeery avpeery marked this pull request as ready for review August 17, 2021 19:41
@arindam1993
Copy link
Contributor

Looks good to me @avpeery ! Nice job. Since this changes a bit of behaviour in the interaction between the browser and GL-JS, it would be helpful to be able to test this manually. Especially eventually during when this lands in a future release. Happy to show you how to add a manual testing page.

@avpeery
Copy link
Contributor Author

avpeery commented Aug 27, 2021

Thanks @arindam1993 for checking out the PR - I added in a query test and manual test. Let me know what you think! Thanks again!

"version": 8,
"metadata": {
"test": {
"height": 128,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be a fractional value to exercise the new codepath?

Copy link
Contributor

@arindam1993 arindam1993 left a comment

Choose a reason for hiding this comment

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

Looks good to me and the render test output is perfect, it demonstrates that the pixel grid snapping is working along with the rounding up of the image size!

Nice work!

@avpeery avpeery merged commit 5d77dd6 into main Sep 8, 2021
@avpeery avpeery deleted the avpeery/canvas-size-rounding branch September 8, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canvas size should be rounding up instead rounding.
4 participants