-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(core): correct opacity in interleaved mode #9642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
layers={layers.map(l => l.clone({beforeId: 'watername_ocean'}))} | ||
initialViewState={INITIAL_VIEW_STATES} | ||
layerFilter={this._layerFilter} | ||
{...(!interleaved && {views})} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interleaved and the view prop do not play nicely together with postprocessing as the MapboxOverlay
does a double render, thus applying the postprocess twice to the mapbox canvas.
Details: hasNonMapboxViews
ends up being true
due to the custom views: https://github.com/visgl/deck.gl/blob/master/modules/mapbox/src/deck-utils.ts#L340-L357
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a sense of how we might support multi-view + interleave + post-processing?
Could this look like configuring something on each view, or configuring post-processing to apply only to specific views? Something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most flexible approach would be to have postprocessing be something that is done on a per-layer basis. For:
- A) effects that require multiple layers, they can be combined using
CompositeLayer
(this is basically how the PostProcessUtils do it - B) effects that should only apply to a specific view can use the same approach in conjunction with
layerFilter
to only render the postprocessed layer(s) in the given view
Applying postprocessing to a specific view feels more restrictive as it means all the layers in that view would then be affected.
The only downside I can think of is layers which get shown in multiple views, where one would like to apply postprocess only in one view. In this case the layer would have to be duplicated, but I don't think this is bad - it at least makes clear what is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah great point, per-layer effects are the most flexible of the options.
And I can't think of too many scenarios where I want to only apply an entire effect to a view.
I agree that we can optimize for per-layer and entire canvas effects.
Sounds like the APIs are fine and we just have rendering issues for interleaved + multi-view that can be solved internally. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, except we don't have an official way to do per-Layer effects - it would be best to include that in the core of deck.gl
Some more background, as this is quite tricky to get right. There are two cases to consider:
Here are some example renders: Postprocessing with a shader that just writes out the input colorCurrent behavior (master)pp-opacity-main.movThe blending setup is incorrectly darkening the output, when it should not New behavior (this PR)pp-opacity-fix.movColor is now correct, only the opacity is modified Postprocessing with zoom blurCurrent behavior (master)zoomblur-master.movTranslucent pixels from postprocess are correct, but layer is darkened New behavior (this PR)zoomblur-fix-old.movLayer no longer darkened, but the blending of the postprocess is wrong New behavior including fix in lumaThe issue is that two effects in luma.gl are using per-multiplied alpha in the shader, which is incorrect. The blending should be handled by the blend modes, not in the shader zoomblur-fix-fixshader.movWith the luma fix, we finally get the correct result 🥵 ! |
The failing render test is due to the issue in luma. Running with the luma fix this result is fixed: ![]() Current (incorrect) golden image: |
Thank you very much for this write up and examples! The new results look wayyyyyy better. We never mentioned issues with I'd been aware of an undocumented limitation we had about not being able to achieve basic blending effects like "additive" via deck prop parameters in v8 and below, but I consider that technique a hack since it interfered with the interleaved renderer. I'd prefer officially supporting an "additive" effect via PostProcessing. Great work. I think it's worth mentioning formal support for interleaved |
Background
When using
PostProcessEffect
withinterleaved: true
, due to clearing of the canvas the result is the basemap is completely cleared with a solid grey color:With this change, the clearing is applied only when needed, and the blending in
ScreenPass
is fixed to give correct results. Here is a layer, drawn with opacity with aink
postprocess applied, in interleaved mode.Screen.Recording.2025-05-22.at.09.44.57.mov
In order to test, the layer browser has been enhanced to include a toggle for interleaved rendering
Change List
deck-renderer
whenclearCanvas
is explicitly set tofalse
ScreenPass
(only used inPostProcessEffect
)RasterLayer