Fix collision box calculation

[kkaefer]

It looks like we're calculating collision boxes that are too small for some labels:

(zoom ~1.7)

This happens on both master and on #10436

/cc @ChrisLoer @ansis

[kkaefer]

Looks like this is an artifact from collision boxes not changing size after the initial zoom level (overzoomed tile):

[jfirebaugh]

I believe this may be the same issue as a render test failure I've been debugging as part of porting native's "possibly evaluated" property types to JS. The problem is that SymbolLayout::addFeature expects to be able to evaluate text-size and icon-size at zoom + 1:

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

Lines 268 to 269 in 3bef759

	const float layoutTextSize = layout.evaluate<TextSize>(zoom + 1, feature);
	const float layoutIconSize = layout.evaluate<IconSize>(zoom + 1, feature);

However, layout here is SymbolLayoutProperties::PossiblyEvaluated. PossiblyEvaluated is an optimization to avoid re-evaluating constants and camera functions for every feature -- what it means is "evaluated if the original value was a constant or camera function, unevaluated if the original value was a source or composite function". In the former case, evaluate just returns the value previously evaluated for zoom, and the zoom + 1 input is ignored:

mapbox-gl-native/src/mbgl/style/properties.hpp

Lines 154 to 156 in 3bef759

	[&] (const T& t) {
	return t;
	},

I.e. layout.evaluate<TextSize>(zoom + 1, feature) will return the same result as layout.evaluate<TextSize>(zoom, feature), which is not the desired behavior.

[jfirebaugh]

I think the fix I'm going to make in JS is to remove the zoom parameter from PossiblyEvaluated:: evaluate, and instead make it a member variable that gets set when the PossiblyEvaluated is created as a result of (possibly) evaluating properties at a certain zoom level. This will make it clear that you can't supply a different zoom level at the final evaluation step. The SymbolLayout constructor will then need to create individual PossiblyEvaluatedPropertyValues for text-size and icon-size at zoom + 1, and use those for final evaluation in addFeature.

[jfirebaugh]

Ok, it looks like the fix will be a bit more complicated than that, even. If I'm reading things right, the symbol layout process needs text-size evaluated at up to five (!) different zoom levels, and icon-size at three:

• text-size at the zoom level of the bucket. Used to calculate the box for icon-text-fit (incorrectly: Fix incorrect rendering when zoom-dependent text-size is used with icon-text-fit mapbox-gl-js#5656)
• text-size and icon-size at the zoom level of the bucket, plus one. Used to calculate collision boxes.
• text-size at zoom level 18. Used for something line-symbol-placement-related?
• For composite *-size expressions: *-size at the zoom levels of the two stops that "cover" the zoom level of the bucket. This goes into a vertex buffer and is used by the shader to interpolate the size at render time.

[ChrisLoer]

When we fix this, we should recheck the render tests currently ignored against #9732, #10409, and #10412 -- it could be the explanation for some or all of the changes we see.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.