[core] don't hide icons if text is an empty string

[ansis]

port of mapbox/mapbox-gl-js#6164

[ChrisLoer]

I think there's actually a very subtle difference from the GL JS logic here.

GL JS says "if you have a collision box, try to place it", and in symbol_layout.js it only adds a collision box if the shaping is non-empty:

https://github.com/mapbox/mapbox-gl-js/blob/692ee23a301210a4a98c56e88c7e77c6ffbe4324/src/symbol/symbol_layout.js#L341-L344

But on gl-native, this new logic says "if you have a non-empty shaping, try to place your CollisionFeature". I think in gl-native we'll actually create a CollisionFeature (and accompanying box, potentially with non-zero size due to padding) even if the shaping is empty:

mapbox-gl-native/src/mbgl/layout/symbol_layout.cpp

Lines 254 to 257 in 8a283b0

	// if either shapedText or icon position is present, add the feature
	if (shapedTextOrientations.first || shapedIcon) {
	addFeature(std::distance(features.begin(), it), feature, shapedTextOrientations, shapedIcon, glyphPositionMap, tileID, sourceID);
	}

mapbox-gl-native/src/mbgl/layout/symbol_instance.cpp

Lines 33 to 35 in 8a283b0

	// Create the collision features that will be used to check whether this symbol instance can be placed
	textCollisionFeature(line_, anchor, shapedTextOrientations.first, textBoxScale, textPadding, textPlacement, indexedFeature, overscaling),
	iconCollisionFeature(line_, anchor, shapedIcon, iconBoxScale, iconPadding, indexedFeature),

Did the new test fail before you made these changes? It seems to me like they may not be necessary (although we should rationalize the behavior between JS and native, I'm starting to think the current native behavior may be more correct?). As it stands, we'll create a collision box but then not use it.

[ansis]

@ChrisLoer good point. I need to dig into this more

Did the new test fail before you made these changes?

Yep, the test fails without these changes

[ChrisLoer]

I just added a native ignore for this for #11156, let's remember to take that out when we merge the fix here.

[ansis]

Ok, so, there are three cases we care about: no text, empty string of text, text.

native master creates:

• zero-sized shapings and collision features for no text
• zero-plus-padding-sized shapings and collision features for empty strings of text

However, they never get used because this is only true when the text is a non-empty string. So while there are internal differences between native master and old -js, they have the same behaviour.

---

I think it might be fine to continue treating an empty string of text the same as undefined text. Both would never be tested for collisions. Do you see any good reason to start treating empty strings differently?

I pushed a fix that is equivalent to the previous fix but simpler. It just changes the per-bucket data presence check to a per-feature check. If this looks good, I'll change -js to match this approach (which shouldn't change -js behaviour).

[ChrisLoer]

OK, sounds good to me, thanks for digging in. I like the simplicity of the new approach, and it should be a cheap lookup even though it's in an inner loop...

🤔 I wish I could think of a better name for "iconWithoutText", it's not really clear that it's focused on whether a collision happens.

As for whether an empty string should be treated as "undefined" vs "defined but 0-length", I feel like "empty string can be a valid/defined string" is generally a more natural convention. If someone defines the text to be set to the result of some variable, they're going to expect "this symbol has text", and not expect there to be different behavior if the variable happens to evaluate to an empty string. But it's hard for me to see how it would make any practical difference in this case.