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

Simplify and fix sprite atlas coordinate calculations #4737

Merged
merged 4 commits into from
May 23, 2017

Conversation

jfirebaugh
Copy link
Contributor

GL JS counterpart of mapbox/mapbox-gl-native#9038.

Copy link
Contributor

@ansis ansis 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! I don't have any feedback or questions.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Do we have to use getter methods instead of properties here? Let's make sure the overhead from creating tons of new array objects doesn't add up in the frame duration benchmark.

@jfirebaugh
Copy link
Contributor Author

I had to revert the getters for other reasons, anyway (object must be plain JSON to send to worker).

Final benchmarks:

benchmark master 02dfbb1 sprite-atlas-pixels 0edbd2a
map-load 121 ms 110 ms
style-load 85 ms 87 ms
buffer 1,036 ms 1,000 ms
fps 60 fps 60 fps
frame-duration 4.1 ms, 0% > 16ms 4 ms, 0% > 16ms
query-point 1.95 ms 2.10 ms
query-box 81.08 ms 84.27 ms
geojson-setdata-small 5 ms 3 ms
geojson-setdata-large 90 ms 89 ms

* Always return image metrics exclusive of padding
* Work with integer coordinates whenever possible
* Eliminate redundant SpriteAtlasElement members
* Fix asymmetric re-padding in getIconQuad when pixelRatio != 1
* Add explanatory comments
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