Stereoscopic viewport didn't support Color Manangement due recent
changes in the color management pipeline. In order to solve the issue we
will migrate the strereo rendering into the GPUViewport. This will share
some textures and reduce required GPU memory.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- T73931 (branched from master)
- Build Status
Buildable 6742 Build 6742: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| source/blender/draw/DRW_engine_types.h | ||
|---|---|---|
| 46–47 ↗ | (On Diff #22033) | We render the left and right eye in serial so we can reuse the depth buffers. We need to keep the color textures around for the color management pipeline. |
| 48–49 ↗ | (On Diff #22033) | I updated the patch to use blending/pixel discarding to implement the Anaglyph/Interlaced rendering. The other Stereo pipelines cannot be done in the shader as they are only compatible with full screen buffers. |
| source/blender/gpu/intern/gpu_viewport.c | ||
|---|---|---|
| 529 | Remove mask. It is incorrect | |
Comment Actions
Disable SRGB encoded framebuffer when attaching an offscreen framebuffer. offscreen buffers don't use the SRGB encoding, but could have been enabled in the rendering pipeline. I would also propose to store the FRAMEBUFFER_SRGB bit to the GPU_VIEWPORT_BIT for saving and restoring.
Comment Actions
Is this patch ready?
It's not clear to me since some of the stereo drawing methods are still in WM. It's also not clear to me if this affects image editor stereo, if that can still work if it moves into GPUViewport.
Comment Actions
IMO this patch is ready. Can do a sanity check on Thursday . Image editor also uses gpu viewport. Only the side by side and top bottom is part of the wm due to its requirement of a full screen buffer what cannot be solved per viewport
Comment Actions
I'm getting a crash with a simple stereo EXR image in the image editor that works fine in 2.82:
https://github.com/AcademySoftwareFoundation/openexr-images/raw/master/MultiView/Balls.exr
//source/blender/gpu/intern/gpu_attr_binding.c:76: get_attr_locations: Assertion input != NULL' failed.`
Full backtrace: P1296
Comment Actions
I believe I know the problem, gpu_shader_image_overlays_stereo_merge_frag.glsl is not using texCoord_interp so it's been optimized out leading to the assert.
For example this fix the problem for me: P1298
Comment Actions
The crash is gone, so for me this can go in and we fix the other issues in master.
For instance, top-bottom mode is working, but not anaglyph or interlace in the image editor. I'm looking at other viewport related fixes, but anyways the situation is so bad without this patch that I don't even mind that this doesn't fix everything.
Comment Actions
The patch looks mostly good for me.
I would like to see a bit more cleanup in the managment of viewports/views to be easier to follow and less error prone.
There is however an error in the composite shader.
| source/blender/gpu/intern/gpu_viewport.c | ||
|---|---|---|
| 71 | Uppercase first letter and end with fullstop! Also the comment should be | |
| 347 | I'm not a huge fan of having a state dependent view here. I would prefer if all viewport functions that are used for a specific view would take the view as argument. | |
| 685 | remove new line | |
| source/blender/gpu/shaders/gpu_shader_image_overlays_stereo_merge_frag.glsl | ||
| 20 ↗ | (On Diff #22783) | Did you mean layout(location = 1) ? Index is only when using dual source blending. Here we are using multitarget rendering. |
| 43 ↗ | (On Diff #22783) | discard implies return. |
| 11 ↗ | (On Diff #22248) | Camel Case for uniform samplers. |
| source/blender/windowmanager/intern/wm_draw.c | ||
| 553 | The only part where the view is really required is DRW_notify_view_update. This is the only time I found GPU_viewport_framebuffer_view_set is required. Can we workaround this by having a function call for this only in DRW_notify_view_update? | |
| 564 | This can be removed and replaced by wm_region_use_viewport. | |
| 638 | So if I understand correctly, you are mixing the stereo views here and saving the result in the first view. | |
| 696–704 | remove branch if there is nothing different in both branches | |
| source/blender/windowmanager/wm_draw.h | ||
| 34 | Why do we still need two offscreen? we use GPUViewport for UV image editor now so that shouldn't be required unless there is other areas i'm not aware of that still draw using stereo. | |
Comment Actions
Wouldn't it be more interesting to pass enum GPU_VIEWPORT_STEREO instead of a bool in GPU_viewport_create?
This makes things a little more descriptive.
| source/blender/gpu/GPU_viewport.h | ||
|---|---|---|
| 99–100 | Cleanlier: add GPU_viewport_stereo_create instead of a boolean. This function would create a GPUViewport with GPU_viewport_create and add necessary stereo buffers. | |
Comment Actions
Code review comments:
- GPU_viewport_active_view_set may only be called from DRW_notify_view_update
- Fragment Shader didn't merge the overlay correctly
- Naming convention uniforms
Note that during final render Grease pencil does not use the correct render buffer. It only renders in a single view (also in master)
Also workbench is crashing during final rendering (also in master).
| source/blender/gpu/GPU_viewport.h | ||
|---|---|---|
| 99–100 | We cannot alloc the buffers yet as that needs a size. | |
| source/blender/windowmanager/intern/wm_draw.c | ||
| 564 | Not sure, wouldn't that fail in DRW_draw_render_loop_ex? if (WM_draw_region_get_bound_viewport(region)) {
/* Don't unbind the framebuffer yet in this case and let
* GPU_viewport_unbind do it, so that we can still do further
* drawing of action zones on top. */
} | |
| 638 | Yes as pixels do not move we don't need additional textures for the merging. The color management process is then also straight forward. I will add a Comment to the GPU_viewport_stereo_composite function. | |
| source/blender/windowmanager/wm_draw.h | ||
| 34 | I will do that in a separate patch. | |
| source/blender/windowmanager/wm_draw.h | ||
|---|---|---|
| 34 | ||
| source/blender/windowmanager/intern/wm_draw.c | ||
|---|---|---|
| 564 | In this instance it only checks if we are rendering of this areas or for offscreen or for window display. Anyways that's a bit of a bad level call even. We should take this decision from a DRW state not from the area state. Leave it as is for now. | |