Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] shader program must always match bucket in render symbol layer #13667

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Jan 3, 2019

Before this change, RenderSymbolLayer with updated style was trying to render
symbols using the previous bucket (with paint property binders that matched a previous program).

Now, symbol bucket caches the latest corresponding paint properties (caching is happening on
complete tiles only). As a result, RenderSymbolLayer always picks the shader program and
its parameters in sync with the obtained bucket.

Fixes: #13407

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with storing the paint property values in the bucket is that if you modify just a paint property but no layout properties, the bucket doesn't get re-generated -- so with this code, your paint property change wouldn't show up until something else triggered a tile reload. This is why I tried to save just the constant Bitset (for shader selection) instead of the values themselves in the bucket-bitset branch -- although it's somewhat more complicated.

On top of the "paint changes, layout doesn't" case, there's also the case where someone changes both paint and layout properties at the same time, triggering re-layout. The current (and intended) behavior is that the paint property change takes effect immediately, while the layout change doesn't take effect until the tile reloads. You could definitely argue that it would be more consistent for both changes to take place at the same time, but I think so far we've prioritized making constant-value paint property changes instantaneous.

Also, however we fix this, we should do it across all the layer types, right?

@pozdnyakov pozdnyakov force-pushed the mikhail_sync_bucket_in_render_symbol_layer branch from f2817a6 to 01356b5 Compare January 9, 2019 16:35
@pozdnyakov pozdnyakov added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jan 9, 2019
@pozdnyakov
Copy link
Contributor Author

@ChrisLoer Thanks for your comments! Paint properties are now updated for the complete tile buckets. PTAL.

@pozdnyakov
Copy link
Contributor Author

Also, however we fix this, we should do it across all the layer types, right?

Sure thing! Once we agree on the approach I'd be happy to spread it to other layer types.

@pozdnyakov pozdnyakov removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jan 10, 2019
@tobrun
Copy link
Member

tobrun commented Jan 10, 2019

#13705 adds a manual regression test.
Validated that this PR fixes the issue from #13407

Before this change, `RenderSymbolLayer` with updated style was trying to render
symbols using the previous bucket (with paint property binders that matched a previous program).

Now, symbol bucket caches the latest corresponding paint properties (caching is happening on
complete tiles only). As a result, `RenderSymbolLayer` always picks the shader program and
its parameters in sync with the obtained bucket.
@pozdnyakov pozdnyakov force-pushed the mikhail_sync_bucket_in_render_symbol_layer branch from 01356b5 to e01182c Compare January 10, 2019 12:48
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants