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

Improve image/glyph atlas packing algorithm #7171

Merged
merged 4 commits into from
Aug 29, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 22, 2018

Since our current binpacking uses are mostly static, I found a simple packing algorithm that produces smaller glyph and image atlases than shelf-pack — for a typical streets z11 view, about 20% less pixels in average. This may not be reflected noticeably on the benchmarks, but it means textures will use less memory and be faster to upload to GPU on tile loads.

In this PR, I've only switched worker-side atlases to the new packing since they're fully static. The next step would be to replace shelf-pack in image_manager.js for currently used patterns, but that would mean replacing dynamic insertion/removal of individual images with repacking all patterns every time one is added/removed. This may be fine because there are only a couple patterns on the screen at a time on typical styles, not thousands where it could be perf a problem.

If the approach sounds OK, I can work on this further and also likely publish the algorithm as a separate ES module. shelf-pack would still remain our module of choice for dynamic use cases.

cc @jfirebaugh @bhousel

Benchmarks

image

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

@bhousel
Copy link
Contributor

bhousel commented Aug 23, 2018

Awesome @mourner !
I don't have any specific feedback other than to make sure to benchmark it against tiles with heavy CJK labels.. Chinese maps stress the binpacker much more than Latin/Cyrillic maps, however CJK glyphs tend to have more uniform dimensions, so they pack easier. Shelf-pack was more optimized for insertion speed than maximizing the space used, so I'm sure there are opportunities to improve it 👍

A good place to test is around here - rotate and pan to load as many tiles as possible!

@mourner
Copy link
Member Author

mourner commented Aug 28, 2018

@bhousel I ran some modified benchmarks from ShelfPack that stress the "low height variation" case (with autoResize: true and inPlace: true) — looks like Potpack is multiple times faster, so CJK shouldn't be a problem.

@mollymerp
Copy link
Contributor

@mourner the ImageManager is only used for background-pattern now, all other pattern properties use the tile-specific ImageAtlas created on the Worker as of #6289

As part of #7207, however, we may need to be able to modify/add sprites to these ImageAtlases, so we should consider that if we move forward here, and in that case we'd have to repack all icons used in the tile whenever we add new ones used in feature-state expressions. I don't have a strong opinion on this because it is probably not a very likely use case, but I would be curious about the perf trade-offs there.

@mourner
Copy link
Member Author

mourner commented Aug 28, 2018

@mollymerp great! I'll push the ImageManager-updating change then. I think the performance should be good enough — e.g. packing 10,000 boxes is sub-1ms according to this benchmark.

Quick question — one render test (fill-extrusion-pattern/missing) failed after rebasing on top of the pattern work, and I had to add f547f6e to fix this — any ideas why? It doesn't fail on master because it creates an empty 64x64 atlas even if no icons/glyphs are used, but I wonder if there's a potential bug hiding in there somewhere.

@mourner
Copy link
Member Author

mourner commented Aug 29, 2018

This is now ready for review.

@mourner mourner merged commit 8dd624d into master Aug 29, 2018
@mourner mourner deleted the better-atlas-packing branch August 29, 2018 19:10
@@ -75,6 +74,7 @@
"pngjs": "^3.0.0",
"postcss-cli": "^5.0.0",
"postcss-inline-svg": "^3.1.1",
"potpack": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a regular dependency rather than a dev dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my misstep, although it actually doesn't affect anything because GL JS is only usable with the built bundle. We might want to move most other regular deps to devdeps then.

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.

4 participants