Add multiview_data for multiview information #8410

z3moon · 2025-02-04T01:56:09Z

Add a varying variable multiview_data for both surface and post-processing shaders. Currently this only works as intended in surface shader, and additional implementation to facilitate it for post-processing will be following.

The multiview_data contains multiview information like whether multiview is enabled for the current material and what the eye index is in that case. This variable is globally available including fragment shaders so that it can be referenced in user shaders in .material files. Also this allows us to directly use the constant variables such as gl_ViewID_OVR for OpenGL and gl_ViewIndex for Vulkan.

Make the preprocessor MATERIAL_FEATURE_LEVEL available for both surface and post-processing shaders, it used to be only available for surface shader, and this will be referenced in post-processing shaders later to support multiview for post-processing.

Add a varying variable `multiview_data` for both surface and post-processing shaders. Currently this only works as intended in surface shader, and additional implementation to facilitate it for post-processing will be following. The `multiview_data` contains multiview information like whether multiview is enabled for the current material and what the eye index is in that case. This variable is globally available including fragment shaders so that it can be referenced in user shaders in .material files. Also this allows us to indirectly reference the constant variables such as `gl_ViewID_OVR` and `gl_ViewIndex` in a platform-agnostic way. Make the preprocessor `MATERIAL_FEATURE_LEVEL` available for both surface and post-processing shaders, it used to be only available for surface shader, and this will be referenced in post-processing shaders later to support multiview for post-processing.

poweifeng

lgtm but I think this needs Mathias and/or Ben to chime in as well.

poweifeng · 2025-02-04T06:52:27Z

shaders/src/common_varyings.glsl

+// to the fragment shader. `x` means whether multiview is used, `y` means the current view index.
+// Neither the multiview feature nor the `flat` keyword is supported in FL0. So, for FL0, declare
+// it as a non-flat and non-varying variable and set it to zeros as a placeholder.
+#if MATERIAL_FEATURE_LEVEL > 0


nit: why not wrap this in #if defined(VARIANT_HAS_STEREO) && defined(FILAMENT_STEREO_MULTIVIEW) ?

as powei says, if multiview/stereo is not enabled, we might as well use a constant and avoid a varying.

I don't see any getter for multiview_data.x we should not let user code access it directly.

Also maybe we should name it something less likely to colide with use variable. Maybe filament_multiview_data.

do we need multiview_data.x ? if multiview is not enabled, would we be in the defined(VARIANT_HAS_STEREO) && defined(FILAMENT_STEREO_MULTIVIEW) case? In other words, why is multiview_data.x not a compile time option?

This pattern should be used for post-processing as well where VARIANT_HAS_STEREO is unavailable unfortunately. So this is a workaround to make this line available for PP. I think I can add FILAMENT_STEREO_MULTIVIEW check though.

multiview_data.x is necessary because VARIANT_HAS_STEREO is unavailable for post-processing where it needs to know whether the current pass is multiview. For example, it's used to calculate the index to the bent normal layers for the SSAO logic that I'm going to add later.

Actually, we don't need this for post-process materials for now, because they're not public. This can all be done with a spec-constant and custom variables.

I see spec constants in shader files are all const and its value is determined in compile time. E.g., const int BACKEND_FEATURE_LEVEL = SPIRV_CROSS_CONSTANT_ID_0;

However, the value of gl_ViewID_OVR is increasingly changing over each shader execution (from 0 to 3) and needs to be passed/inherited to fragment shader from its corresponding vertex shader. So we cannot achieve this with spec-constant IIUC. (or I might be missing something)

Custom variables look like they work in the same way as varying variables except that they're not flat, which could be slightly worse in performance. Besides, if we put the inheritance logic via custom variable in the NEW post-processing shaders (which contain the multiview = true, field), the same boilerplate code that handles selecting either gl_ViewIndex or gl_ViewID_OVR based on the target api should be added as well.

I personally see using the same pattern here for PP could be better for maintainability, WDYT?

I think we should absolutely add the concept of flat custom variables. So maybe we can start with that as a separate change.

I agree that for maintainability we should do the same thing for postfx, but I do not want to add more variants for all postfx. And I don't really want to have this runtime check either, nor the extra varying.
Since postfx materials are not really public right now, I think it's okay to "hack" this using a custom variable and a custom spec constant. PostProcessManager already does stuff like that for TAA (I mean using spec constants).

We can file a bug to revisit postfx stsereo.

Addtionnaly, I think you should split this change in two. Nobody should be relying on postfx right now, so maybe you can make the change, just for surface materials, so we unblock people depending on it. And we do the postfx fixes as a separate PR. wdyt?

I think you should split this change in two

This works for me. Let me wrap this up and circle back on how to deal with the poxtfx case.

FYI, this approach doesn't add additional variants for postfx. I added separate new materials for multiview, which are mipmapDepthMultiview.mat, saoBentNormalsMultiview.mat, and saoMultiview.mat with multiview = true, in the json section in the file. Only these materials are compiled with the varying variable.

For multiview, one draw call invokes multiple vertex shader executions (2 or 4 times depending on the engine setting) per vertex, and the gl_ViewID_OVR value is set properly by the driver in each execution. What I want to do is to capture the value in each vertex execution and deliver it to the fragment shader. If it's achievable without using varying, it'd be nice. But I don't see how this can be done using spec-constant yet.

pixelflinger · 2025-02-04T18:32:22Z

shaders/src/common_varyings.glsl

+#if MATERIAL_FEATURE_LEVEL > 0
+LAYOUT_LOCATION(99) flat VARYING ivec2 multiview_data;
+#else
+ivec2 multiview_data = ivec2(0, 0);


this can probably be const

We can't put const here because FL0 materials like lib/filamentapp/materials/transparentColor.mat can't be compiled otherwise.

pixelflinger · 2025-02-04T18:35:58Z

shaders/src/common_varyings.glsl

+// to the fragment shader. `x` means whether multiview is used, `y` means the current view index.
+// Neither the multiview feature nor the `flat` keyword is supported in FL0. So, for FL0, declare
+// it as a non-flat and non-varying variable and set it to zeros as a placeholder.
+#if MATERIAL_FEATURE_LEVEL > 0


as powei says, if multiview/stereo is not enabled, we might as well use a constant and avoid a varying.

pixelflinger · 2025-02-04T18:36:40Z

shaders/src/common_varyings.glsl

+// Neither the multiview feature nor the `flat` keyword is supported in FL0. So, for FL0, declare
+// it as a non-flat and non-varying variable and set it to zeros as a placeholder.
+#if MATERIAL_FEATURE_LEVEL > 0
+LAYOUT_LOCATION(99) flat VARYING ivec2 multiview_data;


can we do that? use a location of 99 ? are you sure it works on ES3.0 and ES3.1 and Vulkan?

I picked this number -1 from LAYOUT_LOCATION(100) out float filament_gl_ClipDistance[2];, assuming it's safer. Let me change it to 12 since it's the next available number.

note that GL_MAX_VARYING_VECTORS minimum value is 15, and GL_MAX_VARYING_COMPONENTS is 60.

note that GL_MAX_VARYING_VECTORS minimum value is 15, and GL_MAX_VARYING_COMPONENTS is 60.

~~This makes me think we might have a problem already. Because materials can have up to 4 custom varyings vectors and the total number of vectors cannot be more than 15.~~
It looks like the 4 user varying are reserved 0-3 location.

Thanks for the info! Is there any guidance on how to pick the right available number? or picking 12 would be sufficient?

pixelflinger · 2025-02-04T18:38:16Z

shaders/src/common_varyings.glsl

+// to the fragment shader. `x` means whether multiview is used, `y` means the current view index.
+// Neither the multiview feature nor the `flat` keyword is supported in FL0. So, for FL0, declare
+// it as a non-flat and non-varying variable and set it to zeros as a placeholder.
+#if MATERIAL_FEATURE_LEVEL > 0


I don't see any getter for multiview_data.x we should not let user code access it directly.

Also maybe we should name it something less likely to colide with use variable. Maybe filament_multiview_data.

pixelflinger · 2025-02-04T18:39:38Z

shaders/src/common_varyings.glsl

+// to the fragment shader. `x` means whether multiview is used, `y` means the current view index.
+// Neither the multiview feature nor the `flat` keyword is supported in FL0. So, for FL0, declare
+// it as a non-flat and non-varying variable and set it to zeros as a placeholder.
+#if MATERIAL_FEATURE_LEVEL > 0


do we need multiview_data.x ? if multiview is not enabled, would we be in the defined(VARIANT_HAS_STEREO) && defined(FILAMENT_STEREO_MULTIVIEW) case? In other words, why is multiview_data.x not a compile time option?

bejado

Generally LGTM but I think that filament_multiview_data.x could be a compile-time constant instead of a varying. I'm also a bit concerned that we're burning another varying, I think on some platforms we're nearing the limit.

bejado · 2025-02-14T18:13:35Z

shaders/src/surface_main.vs

+#   else
+    int multiview_view_index = int(gl_ViewID_OVR);
+#   endif // TARGET_VULKAN_ENVIRONMENT
+    filament_multiview_data = ivec2(1, int(multiview_view_index));


nit: I don't think you need the int cast here.

Suggested change

filament_multiview_data = ivec2(1, int(multiview_view_index));

filament_multiview_data = ivec2(1, multiview_view_index);

z3moon added the internal Issue/PR does not affect clients label Feb 4, 2025

z3moon requested review from pixelflinger, bejado and poweifeng February 4, 2025 01:56

z3moon force-pushed the zm/add-multiview-data branch from a3529be to 4ded5cf Compare February 4, 2025 02:50

z3moon force-pushed the zm/add-multiview-data branch from 4ded5cf to 923ac98 Compare February 4, 2025 03:48

poweifeng approved these changes Feb 4, 2025

View reviewed changes

pixelflinger requested changes Feb 4, 2025

View reviewed changes

feedback

cf75b44

z3moon requested a review from pixelflinger February 5, 2025 00:36

bejado reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiview_data for multiview information #8410

Add multiview_data for multiview information #8410

z3moon commented Feb 4, 2025

poweifeng left a comment

poweifeng Feb 4, 2025

pixelflinger Feb 4, 2025

pixelflinger Feb 4, 2025

pixelflinger Feb 4, 2025

z3moon Feb 4, 2025

z3moon Feb 4, 2025

pixelflinger Feb 4, 2025 •

edited

Loading

z3moon Feb 4, 2025

pixelflinger Feb 5, 2025

z3moon Feb 5, 2025

pixelflinger Feb 4, 2025

z3moon Feb 4, 2025

pixelflinger Feb 4, 2025

pixelflinger Feb 4, 2025

z3moon Feb 4, 2025

pixelflinger Feb 4, 2025

pixelflinger Feb 4, 2025 •

edited

Loading

z3moon Feb 4, 2025

pixelflinger Feb 4, 2025

pixelflinger Feb 4, 2025

bejado left a comment

bejado Feb 14, 2025

	filament_multiview_data = ivec2(1, int(multiview_view_index));
	filament_multiview_data = ivec2(1, multiview_view_index);

Add multiview_data for multiview information #8410

Are you sure you want to change the base?

Add multiview_data for multiview information #8410

Conversation

z3moon commented Feb 4, 2025

poweifeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflinger Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflinger Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bejado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelflinger Feb 4, 2025 •

edited

Loading

pixelflinger Feb 4, 2025 •

edited

Loading