Separate annotation sprite atlas from the style, proposed fix for #1488

[adam-mapbox]

@kkaefer @jfirebaugh @incanus

Comments about thread safety, etc. welcome.

[incanus]

Digging into this now @adam-mapbox, but some code review / understanding comments:

1. We should follow the pattern Style& style, SpriteAtlas* atlas for arguments (i.e. consistent spacing).
2. The member sourceSpriteAtlas has some potential for confusion as we have the concept of sources, but this doesn't have anything to do with them.
3. In routines like MapContext::setSprite, we should codify assumptions about threading such as in nearby routines (assert(util::ThreadContext::currentlyOn(util::ThreadType::Map));).
4. I'm not quite following why you'd fall back to times with no layer-specific sprite, such as SpriteAtlas *useSpriteAtlas = layer.sourceSpriteAtlas ? layer.sourceSpriteAtlas : spriteAtlas;. Can you clarify? That might help point to a more descriptive name for sourceSpriteAtlas above.

And I'd be curious if @jfirebaugh or @kkaefer have any thoughts here about adding functionality to the MapContext given longer-term plans to fold that back into the Map proper.

I'm still doing some investigations into thread safety. It seems like we might be able to indicate some const around this to clarify that things aren't mutated unexpectedly, too.

[incanus]

I'm not quite following why you'd fall back to times with no layer-specific sprite

Oh, I think I follow — the normal style sprite. And a single annotation sprite, which doesn't come into play if there are no annotations.

In that case, do we / can we destroy it when all image annotations are removed?

[incanus]

Why is SpriteStore::Observer implemented (with empty method bodies)? As a todo for later? If we don't need to run anything here now, do we need to implement the observer?

[incanus]

AFAICT this looks good thread-wise. The edge cases I fear (but don't seem to have the head for checking for just yet) are:

• Something getting destroyed prematurely, but I think TileWorker is the only transient object involved, and that flows from parsing and works with the current sprite atlas for which a higher up, permanent object is responsible (MapContext, AnnotationManager, etc.)
• Something leaking and living too long, but I don't think we need to destroy anything (pending Separate annotation sprite atlas from the style, proposed fix for #1488 #3049 (comment)) so I don't think there are any issues there, either.

BTW, in case it's not obvious, the fix works as intended 😄

[incanus]

Ah, so SpriteAtlas::getImage() mutates the internal images as a Holder, hence we can't pass a const SpriteAtlas around. Internally, that routine already uses a mutex, so I think we're clear here thread-wise.

[incanus]

Also, yeah, totally weird failing test on Travis here (e.g. https://travis-ci.org/mapbox/mapbox-gl-native/jobs/91681030). It's expecting the custom sprite functionality to log 21 errors and none are. Any insight there @kkaefer since you wrote the test?

[adam-mapbox]

The intent was that sourceSpriteAtlas is the sprite atlas that the SymbolLayer uses as the source for its symbol images.

[adam-mapbox]

I'm at a loss about what to do about this failing test. I'm out of my depth here; the test is failing on linux only, and I don't understand the test or what it's supposed to do, and I don't know how to reproduce it. I could use some assistance.

[incanus]

I'm at a loss about what to do about this failing test. I'm out of my depth here; the test is failing on linux only, and I don't understand the test or what it's supposed to do, and I don't know how to reproduce it. I could use some assistance.

Agree, but the reason we're seeing it only on Linux is that's the only place these core tests are run. So at least we're consistent (i.e. not a flapping test).

Going to play with this test locally a bit.

[incanus]

The intent was that sourceSpriteAtlas is the sprite atlas that the SymbolLayer uses as the source for its symbol images.

Maybe layerSpriteAtlas since it's within the SymbolLayer?

[adam-mapbox]

How do I run the core tests locally? Can I?

[incanus]

I think, as in Travis, it's just make test-*. I don't know offhand how to run a single, but looking into it.

[incanus]

Figured out the format here @adam-mapbox:

make test-Sprite.CustomSpriteImages

http://stackoverflow.com/questions/7208070/googletest-how-to-skip-a-test was useful here.

[incanus]

Clarified docs at b07d4c8.

[jfirebaugh]

👍 Will add some line by line comments for small improvements, but overall I like this approach a lot.

[jfirebaugh]

Avoid reordering the includes here.

[adam-mapbox]

You guys don't alphabetize includes?

[incanus]

They're typically roughly in order of "importance", as best I can read. So in this case, we've got our own class header, then the thing we produce, then the thing we use to produce that, then a specific class of several peers that we need access to. At least that's my logic flow in looking at the old version. Maybe we should settle on a stricter system.

[jfirebaugh]

Yeah, we don't really have a strict system, except that we try to group by path prefix.

[jfirebaugh]

The annotation SpriteAtlas needs to be uploaded in the "upload pass" of Painter::render.

[adam-mapbox]

Never mind, I figured it out :)

[jfirebaugh]

Could we make SpriteAtlas lazy internally, instead of lazily creating the annotation SpriteAtlas? Having possibly null pointers threading through multiple parts of the code gives me pause.

[adam-mapbox]

We certainly could do that. Doesn't that just kind of push the problem around? Making SpriteAtlas lazy is going to significantly increase its complexity and possibly introduce weird bugs to unrelated sections of the code. I don't think it increases the overall quality of the code per se.

[jfirebaugh]

I've encountered several issues in the course of working on annotations today (investigating #3037 and testing this PR):

• Annotation tests had an undiagnosed failure prior to this PR. Fixed in afa04de and filed a followup ticket in MapContext::update should not reset updateFlags wholesale #3082.
• Tests and the GLFW app were still assuming they could use icons from the style sprite for point annotations. As of this change that's no longer the case -- all point annotations must have an icon specified via Map::setSprite. I added changes to this PR to fix that.
• Tests were failing an assert in GLObjectStore. Fixed by moving MapData storage to the MapContext (in this PR).
• Tests are crashing in ~HeadlessView. Still investigating.

[tmpsantos]

Why is SpriteStore::Observer implemented (with empty method bodies)? As a todo for later? If we don't need to run anything here now, do we need to implement the observer?

The tiles always arrive first and from the tiles we see what sprites are needed. Meanwhile we draw whatever we have. When the sprites finally arrive, the SpriteStore::Observer get a signal and repaints the map, this time with sprites. Same goes for the glyphs.

[tmpsantos]

Wouldn't be easier to have these objects living the MapContext object? They would survive a style change anyway and would have a more clear scope. The crash I'm seeing on Linux is related to destruction order, not threading issues.

We are calling glObjectStore.performCleanup() on MapContext::cleanup() just before calling ~MapContext() that will ultimately call the ~MapData() -> ~AnnotationManager() -> ~SpriteAtlas() that tries to dispose the textures but they never get cleaned up.

This patch fixes the crash on your branch: c15e1e6

[jfirebaugh]

They are specific to annotations, so I'd rather have them owned by AnnotationManager.

Your analysis of the issue sounds correct; however I would rather fix it by using std::unique_ptr<AnnotationManager> in MapData and resetting that.

[tmpsantos]

Your analysis of the issue sounds correct; however I would rather fix it by using std::unique_ptr in MapData and resetting that.

That also should work.

[incanus]

Sounds like from voice that @jfirebaugh is wrapping up test accuracy here, then we can crunch this a bit into a final proposed PR @adam-mapbox?

[jfirebaugh]

I'm on it.

[incanus]

👍 Cool just syncing up.

[jfirebaugh]

d55aa79 introduced some changes that conflict with this work. I'm proposing to revert them in #3157, but this PR will now need to wait for that to get sorted out.

[incanus]

🎉

[cowgp]

@incanus Was this fix not included in iOS 3.0.1 (released after fix was committed) ? I'm using 3.0.1 and still seeing this.

[jfirebaugh]

@cowgp That's correct. The 3.0.1 release branch was created before this fix was implemented, and it was deemed too risky to backport. It will be in the next iOS SDK release.

[cowgp]

@jfirebaugh - thanks. That obviously begs the question, what's an ETA on the next release?

[1ec5]

Added to the changelog in 0d3af14.

@cowgp, the 3.1.0 release is coming soon. In the meantime, you can try out the fix in this prerelease (podspec for CocoaPods).

[cowgp]

Thanks @1ec5 - I built a custom version off the master branch last week and pointed to a custom podspec for that. I noticed the 3.1.0-pre.X podspecs have the deployment target bumped to 8.0. Do you know why?
Happy to see 3.1 is rolling out officially soon though - I'll swap back to official cocoapod once it releases.

[1ec5]

I noticed the 3.1.0-pre.X podspecs have the deployment target bumped to 8.0. Do you know why?

Starting in 3.1.0, the pod will download a dynamic framework; dynamic frameworks are only supported by iOS 8.0 and above. If you need iOS 7.x support, you’ll need to download the static framework manually from the releases page.