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

Simplify and fix sprite atlas coordinate calculations #4737

Merged
merged 4 commits into from
May 23, 2017
Merged

Simplify and fix sprite atlas coordinate calculations #4737

merged 4 commits into from
May 23, 2017

Conversation

jfirebaugh
Copy link
Contributor

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

@jfirebaugh jfirebaugh requested review from ansis and anandthakker May 22, 2017 20:07
ansis
ansis approved these changes May 23, 2017
View reviewed changes
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.

mourner
mourner reviewed May 23, 2017
View reviewed changes
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 jfirebaugh force-pushed the sprite-atlas-pixels branch from def33fa to 0edbd2a Compare May 23, 2017 18:38
@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

@jfirebaugh jfirebaugh force-pushed the sprite-atlas-pixels branch from 0edbd2a to 3ec524e Compare May 23, 2017 18:40
jfirebaugh added 3 commits May 23, 2017 11:59
@jfirebaugh
Simplify and fix sprite atlas coordinate calculations
333168d 
* 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
@jfirebaugh
Add tests for 1.5x image
e869abb
@jfirebaugh
Enable passing test in gl-native
7478d79
@jfirebaugh jfirebaugh force-pushed the sprite-atlas-pixels branch from 3ec524e to 7478d79 Compare May 23, 2017 19:08
@jfirebaugh jfirebaugh merged commit ca1e1cf into master May 23, 2017
@jfirebaugh jfirebaugh deleted the sprite-atlas-pixels branch May 23, 2017 19:28
@jfirebaugh jfirebaugh mentioned this pull request May 25, 2017
Closed
@jfirebaugh jfirebaugh mentioned this pull request Jul 17, 2017
Merged
@jmunowitch jmunowitch mentioned this pull request Sep 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

@ansis ansis ansis approved these changes

@anandthakker anandthakker anandthakker 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
@jfirebaugh @mourner @ansis @anandthakker