Incorrect rendering when transitioning a style property to and from a property function

[jfirebaugh]

In order for rendering to obtain the correct result, three things must be mutually consistent, in the sense that they must all be calculated using a consistent view of layer paint properties:

• Bucket#paintAttributes (or programConfigurations, post Introduce ProgramConfiguration #3439)
• paintVertexArrayTypes
• paintVertexArrays (worker) / paintVertexBuffers (main thread)

The worker serializes and sends paintVertexArrayTypes along with paintVertexArrays, ensuring that the latter two items are mutually consistent. However, it's possible that the first two items are inconsistent. Consider the following scenario:

1. A style is loaded, with layer L using a constant value for property P. Label this state (L, P, constant).
2. Workers are asked to prepare buckets for (L, P, constant).
3. map.setPaintProperty(L, P, propertyFunction), resulting in state (L, P, propertyFunction).
4. The buckets for (L, P, constant) are completed and received by the main thread.
5. Render.

This will produce an incorrect rendering. In step 4, the Buckets construct paintAttributes (or programConfigurations, post #3439) using the current layer state, rather than the state seen by the worker. Thus they produce paintAttributes corresponding to (L, P, propertyFunction), indicating that P is data-driven and bound via a vertex attribute. However, paintVertexArrays are for the state (L, P, constant), and do not contain this attribute. In my testing, this resulted in no features being drawn for this layer. I suspect it may also trigger WebGL errors, which we are not currently checking for.

The solution I have in mind is to fully serialize and transfer paintAttributes along with paintVertexArrayTypes and paintVertexArrays, and never pass current style state into Bucket deserialization functions.

This is difficult to write an automated test for. It's actually not too hard to get the timing right -- the difficulty is that our runtime styling tests have a "wait" command but no "rerender now" command. Simply omitting "wait" does not trigger a rerender. You can queue a rerender with e.g. a camera operation, but then there's no way to wait for it to occur without also waiting for tile loads to finish, and the bug only manifests in the interim period.

[ansis]

The solution I have in mind is to fully serialize and transfer paintAttributes along with paintVertexArrayTypes and paintVertexArrays, and never pass current style state into Bucket deserialization functions.

Even with this, I think you'll still have problems with the current recalculated style not matching the programConfiguration of a tile. In your example the tiles need the style to calculate a constant value for property P in order for the rendering to work. But the style has already been changed to use a property function so it can't calculate a constant value for it. And what if multiple loaded tiles in a layer use different programConfigurations?

I don't think the answer is to have multiple versions of the style recalculated for different tiles. I think we need to make sure all tiles being rendered are on the same style version. I'm imagining:

1. All tiles are loaded with style version A.
2. Switch a constant property to a property function, creating style version B.
3. Request all current tiles be reparsed with version B.
4. Continue rendering existing tiles with version A while you wait for all the versions for B to load.
5. Once all the versions for B have loaded, swap all the tiles, switch the style and start rendering with version B.

[lucaswoj]

I don't think the answer is to have multiple versions of the style recalculated for different tiles. I think we need to make sure all tiles being rendered are on the same style version.

I agree this is the best long-term solution. 👍

[jfirebaugh]

But the style has already been changed to use a property function so it can't calculate a constant value for it.

Good point; I hadn't considered that. It would be possible for ProgramConfiguration to evaluate and serialize calculated values, but then they'd be fixed to integer zoom levels. Alternatively, it could serialize the style property value it saw. But I agree, this feels less than ideal.

I think we need to make sure all tiles being rendered are on the same style version.

This would also avoid the partial redraws as single tiles come in with updated paint vertex buffers. It seems like it could be a big, difficult architectural change though...

[mourner]

I think we need to make sure all tiles being rendered are on the same style version.

@jfirebaugh This would also address one of the major performance regressions caused by DDS, per #3342. So while it's a big, difficult change, I think it's worth it by a wide margin.

[mourner]

Capturing another related idea from chat:

@ansis: can we just skip tiles that don't match the layer's first tile's program configuration? The rendering is already buggy in these cases
@jfirebaugh: That might convert a slight rendering artifact to heavy flickering with the background color.
@ansis: It would only flicker when making a style change that changes the program configuration, which is relatively rare. And it would only flicker that single layer. It would look similar to removing the layer and adding a new one.
Taking that approach would let us write draw functions as if the tiles were all on consistent program configurations. The skip check can be just removed in the future when this is actually the case
@jfirebaugh: Yeah, if the effect is not too bad it's probably a good change.

[jfirebaugh]

→ #4012.