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

Per-tile glyph and icon atlases #5190

Merged
merged 5 commits into from Sep 12, 2017
Merged

Per-tile glyph and icon atlases #5190

merged 5 commits into from Sep 12, 2017

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Aug 24, 2017

Ports mapbox/mapbox-gl-native#9213; see that PR for extended explanation.

A breaking change to Map#addImage is included as a prerequisite; see the first commit.

Fixes #4876, fixes #141, fixes #4797.

benchmark master 4d16dc2 atlases 05a7c5a
map-load 107 ms 76 ms
style-load 58 ms 50 ms
buffer 969 ms 970 ms
fps 60 fps 60 fps
frame-duration 6 ms, 0% > 16ms 6.1 ms, 0% > 16ms
query-point 0.58 ms 0.56 ms
query-box 35.91 ms 38.25 ms
geojson-setdata-small 3 ms 2 ms
geojson-setdata-large 111 ms 98 ms

Launch Checklist

Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

Overall, this is tricky to review given its size and the many file renames. Could you please provide a short list of the fundamental changes/steps you did and whether there's anything in particular that you're concerned about or that could use a second pair of eyes?


- `Map#addImage` now requires the image as an `HTMLImageElement`, `ImageData`, or object with `width`, `height`, and
`data` properties with the same format as `ImageData`. It no longer accepts a raw `ArrayBufferView` in the second
argument and `width` and `height` options in the third argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason for this change or just API cleanliness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it internally at first, in order to simplify SpriteAtlas#addImage, and then realized it's just inherently a better design to separate required and optional parameters, and how we should have made the public API from the beginning. Though it's a breaking change, I think it's worth it now, with low usage. In the future it probably wouldn't be.

@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented Aug 29, 2017

Sure.

  • In broad outline, this PR follows the native PR fairly closely.

  • In cases where new code had been written from scratch, I copied the C++ over, then adjusted the syntax for JS. That includes {image,glyph}_atlas.{cpp,js} and image_manager.js. It would include glyph_manager.js, but in JS there's the added ability to use TinySDF for local rendering, so JS deviates from native quite a bit.

  • I also introduced a Texture class, like in native. (Preserve depth buffer between fill-extrusion layers + optimize render order #5101 adds RenderTexture; they should probably be combined/refactored at some point.)

  • I introduced AlphaImage and RGBAImage, equivalents of native's UnassociatedImage and AlphaImage. There's no equivalent to PremultipliedImage because in a web context, non-premultiplied is the defacto standard due to the convention established by ImageData. The types work a little differently than in native: due to the constraints of Web Worker transfer, they can't be classes, so the "methods" are actually static functions that take "this" as their first parameter, and we do some casting to let flow treat plain old objects with the appropriate properties as "instances".

  • In native, when loading the sprite sheet, we change it so that it's split up into individual images a while back. I ported that behavior.

  • A significant cross-cutting change is to switch to image and glyph related types that follow native:

    Native JS
    GlyphMetrics GlyphMetrics
    Glyph StyleGlyph
    style::Image StyleImage
    UnassociatedImage RGBAImage
    PremultipliedImage no equivalent
    AlphaImage AlphaImage
    GlyphPosition GlyphPosition
    ImagePosition ImagePosition

Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

Overall this is a great change! Reviewed the code but didn't find any issues with it. It could benefit from some more comments that make the flow of information clearer.

const halfAdvance = glyph.advance / 2;
// The rects have an addditional buffer that is not included in their size;
const glyphPadding = 1.0;
const rectBuffer = 3.0 + glyphPadding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does 3.0 come from?

@kkaefer
Copy link
Contributor

kkaefer commented Sep 8, 2017

I also introduced a Texture class, like in native. (#5101 adds RenderTexture; they should probably be combined/refactored at some point.)

Can you please ticket this so we can track it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants