Fix selection color applied to point labels

[matteblair]

Resolves #1167

This solves the observed problem, but I'm not sure that this is the intended data flow :\

[karimnaaji]

This actually breaks the unique selection color that each label would be given. After some debugging I think I found where the issue lives.

Several things that makes this bug appear:

• On the issue Unable to get callback for client data source point with properties #1167, it is mentioned that the draw rule under the layer layers.mz_current_location_gem.draw.ux-location-gem-overlay.interactive is marked interactive, but it is actually not. I had to add that on my debugging code:

        std::vector<Tangram::SceneUpdate> updates = {
            {"layers.mz_current_location_gem.draw.ux-location-gem-overlay.interactive", "true"},
        };
        map->loadSceneAsync("https://raw.githubusercontent.com/tangrams/bubble-wrap/gh-pages/bubble-wrap.yaml",
            true, nullptr, nullptr, updates);

cc @ecgreb on that to confirm the value of the interactive flag.

• Some recent optimization uses the set m_labels within labels.cpp to retrieve the label associated to a specific selection color read from the framebuffer. The bug is that this set is only added labels that can not occlude, which is true from the stylesheet here.

I guess we would need to have a separate vector in labels.cpp that contains all labels (including the ones that can't occlude) to be able to retrieve the matching selectionColor -> label.

[karimnaaji]

@blair1618 pushed a hot fix on this branch https://github.com/tangrams/tangram-es/tree/hot-fix-1167.

[hjanetzek]

@karimnaaji ah right! missing these interactive labels. We could add only the labels with selection color to m_selectionLabels instead of collecting all.

[karimnaaji]

@hjanetzek , right maintaining that set from labels.cpp would be the right way to handle that!

[matteblair]

Ah good catch, I forgot that each label needs it's own selection identifier and it can't just use the same one as the feature. I'll test that fix you pushed.

[tallytalwar]

@karimnaaji I see what you are mentioning.

1. Current location gem should be marked non interactive in the style sheet. It might be for the purposes of the example, @ecgreb could have referenced it. But yes I see the point of the bug :D.

2. I see the bug of non colliding labels are not checked when retrieving the labels for selection color. Instead of putting all visible labels for selection color checking, does it make sense to only put labels with "interactive" property set for selection color checking? Say if only pois are marked interactive, it will be wasteful to check for all text labels for selection colors. Moving this comment to Hot fix for https://github.com/tangrams/tangram-es/issues/1167 #1185

[ecgreb]

In the version of the stylesheet I was using at the time current location gem was marked interactive. However after this commit was merged that is now no longer the case.

But yes I did verify the layer was interactive when reproducing the issue. And I agree the current location gem should NOT be interactive moving forward I was just using it for the test case.

[matteblair]

Yes, bubble-wrap was very recently revised to reduce the interactive layers.

Closing in favor of #1185

[nvkelso]

Some apps allow interactivity, we can modify our behavior to indicate say "current location" or "my location" on tap of the gem.

…

On Dec 16, 2016, at 08:10, Matt Blair ***@***.***> wrote: Yes, bubble-wrap was very recently revised to reduce the interactive layers. Closing in favor of #1185 ― You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.