Page MenuHome
Differential D13730

Fix T93179: geonodes UVs and Vertex colors do not work in EEVEE
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Jan 4 2022, 12:32 PM.

Details

Reviewers
Clément Foucault (fclem)
Brecht Van Lommel (brecht)
Maniphest Tasks
T93179: Eevee Attribute Rendering Has Some Left Out Nodes Comparing with Cycles
Commits
rB281bcc1c1dd6: Fix T93179: geonodes UVs and Vertex colors do not work in EEVEE
Summary

Overwriting UV map or vertex color data in Geometry nodes will move the
layers to another CustomData channel, and as such, will make attribute
lookup fail from the UVMap and Vertex Color nodes in EEVEE as the
CustomDataType will also be modified (i.e. no longer CD_MTFACE or
CD_MCOL).

As discussed in T93179, the solution is to use CD_PROP_AUTO_FROM_NAME
so that the render engine is able to find the attributes. This also makes
EEVEE emulate Cycles behaviour in this regard.

Apparently it is enough to just modify the CustomDataType parameter in
calls to GPU_attribute, as the rest of the draw code is already
handling CD_PROP_AUTO_FROM_NAME cases and already properly alias
attribute names. CD_MTFACE and CD_MCOL may still be used for default
layers (with no names) so their handling in the draw code was not
removed.

Demo files downloaded from blender.org, as well as some selected
production files from the Blender Cloud, are still rendering fine.
However, this change might introduce some rendering issues due to name
collision between UV maps and Vertex colors.

Diff Detail

Repository
rB Blender

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Jan 4 2022, 12:32 PM
Kévin Dietrich (kevindietrich) created this revision.
Harbormaster completed remote builds in B19747: Diff 46594.Jan 4 2022, 12:32 PM
Clément Foucault (fclem) added a comment.Jan 4 2022, 3:03 PM

However, this change might introduce some rendering issues due to name collision between UV maps and Vertex colors.

This is my main concern. It partially defeat the purpose of having different nodes for accessing the Custom Data. The only real benefit after that is that the lists are correctly populated. Maybe we should prevent name clashing in the naming operators itself. But that means patching old files too, which is not great.

Hans Goudey (HooglyBoogly) awarded a token.Jan 4 2022, 3:30 PM
Hans Goudey (HooglyBoogly) added a task: T93179: Eevee Attribute Rendering Has Some Left Out Nodes Comparing with Cycles.Jan 4 2022, 3:45 PM
Deen (Bit) awarded a token.Jan 4 2022, 5:50 PM
Brecht Van Lommel (brecht) requested changes to this revision.Jan 5 2022, 1:44 PM

I think this is fine but would then replace CD_MTFACE, CD_MCOL and CD_PROP_COLOR with CD_AUTO_FROM_NAME in all shader nodes, to make it fully consistent with Cycles.

In D13730#363439, @Clément Foucault (fclem) wrote:

However, this change might introduce some rendering issues due to name collision between UV maps and Vertex colors.

This is my main concern. It partially defeat the purpose of having different nodes for accessing the Custom Data. The only real benefit after that is that the lists are correctly populated. Maybe we should prevent name clashing in the naming operators itself. But that means patching old files too, which is not great.

Populating the list was the main motivation for adding those nodes. Personally I think it's fine to just have the name clashing problem for now, it's already been there for a long time with Cycles and we haven't really seen user complaints.

I think we can move towards a future where geometry nodes and renderers don't necessarily care about the type of attribute and just use a name as identifier.

This revision now requires changes to proceed.Jan 5 2022, 1:44 PM
Kévin Dietrich (kevindietrich) updated this revision to Diff 46647.Jan 5 2022, 4:00 PM

Use CD_AUTO_FROM_NAME for all nodes.

This also removes CD_MCOL and CD_PROP_COLOR from the GPU codegen attribute
prefix generation routine. VBOs have been updated to use the "a" attribute
prefix instead of "c" in those cases.

CD_MTFACE remains for the prefix selection as it is the fallback custom
data type used when the attribute name is empty and the type is
CD_AUTO_FROM_NAME.

Harbormaster completed remote builds in B19781: Diff 46647.Jan 5 2022, 4:00 PM
Brecht Van Lommel (brecht) accepted this revision.Jan 5 2022, 4:21 PM
Brecht Van Lommel (brecht) added 1 blocking reviewer(s): Clément Foucault (fclem).
Clément Foucault (fclem) added a comment.Jan 5 2022, 8:03 PM

c prefix is for colors. You would need to remove the u prefix for uvs too.

Kévin Dietrich (kevindietrich) updated this revision to Diff 46857.Jan 11 2022, 8:34 AM

Remove "u" prefix as well. This also removes the CD_MTFACE fallback from
gpu_node_graph_add_attribute as it was only used for prefixing. The various
attribute lookup functions are still defaulting to UVs if the name is empty.

Harbormaster completed remote builds in B19891: Diff 46857.Jan 11 2022, 8:34 AM
Clément Foucault (fclem) requested changes to this revision.Apr 28 2022, 8:34 PM

This does need to be updated to the new codegen. I'm not opposed to simplification to this system. I'll accept after it is updated.

This revision now requires changes to proceed.Apr 28 2022, 8:34 PM
Kévin Dietrich (kevindietrich) updated this revision to Diff 51063.May 2 2022, 9:20 AM

Rebase on master.

The node_uvmap GLSL function was modified to take a vec4 as input.

This removes attr_load_uv and attr_load_color from all shaders as
the generic attribute code is always used now.

Harbormaster completed remote builds in B21920: Diff 51063.May 2 2022, 9:20 AM
Clément Foucault (fclem) added inline comments.May 2 2022, 9:31 AM
source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_uv.cc
51

Does that mean we get any attribute type if not using any name on a UV node? Why do color attrib keep the c but uvs do not keep the u?

Clément Foucault (fclem) accepted this revision.May 2 2022, 9:40 AM

You might have to remove the attr_load_color and attr_load_uv from eevee_next too now. You can do that during commit.

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_uv.cc
51

I see now:

/* Fall back to the UV layer, which matches old behavior. */
 if (type == CD_AUTO_FROM_NAME && name[0] == '\0') {
   type = CD_MTFACE;
 }

Not an issue then.

This revision is now accepted and ready to land.May 2 2022, 9:40 AM
Kévin Dietrich (kevindietrich) added a comment.May 2 2022, 1:07 PM
In D13730#398509, @Clément Foucault (fclem) wrote:

You might have to remove the attr_load_color and attr_load_uv from eevee_next too now. You can do that during commit.

I guess this should be done in tmp-eevee-next-merge?

Clément Foucault (fclem) added a comment.May 2 2022, 2:12 PM

I guess this should be done in tmp-eevee-next-merge?

Nope. Do it in master or the release branch. The changes have been merged already.

Closed by commit rB281bcc1c1dd6: Fix T93179: geonodes UVs and Vertex colors do not work in EEVEE (authored by Kévin Dietrich (kevindietrich)). · Explain WhyMay 3 2022, 10:50 PM
This revision was automatically updated to reflect the committed changes.
Kévin Dietrich (kevindietrich) added a commit: rB281bcc1c1dd6: Fix T93179: geonodes UVs and Vertex colors do not work in EEVEE.
Kévin Dietrich (kevindietrich) added a comment.May 3 2022, 10:54 PM

I commited the fix in the release without changes to`eevee_next` as attr_load_uv and attr_load_color are also used for the grease pencil which uses a couple of global variables (g_uvs and g_color). These globals are set via another shader, so I am not really sure what should be done here. I haven't looked deeply at the surrounding code, so maybe there is an easy solution I have missed.

Clément Foucault (fclem) added a comment.May 4 2022, 12:24 AM
In D13730#398973, @Kévin Dietrich (kevindietrich) wrote:

I commited the fix in the release without changes to`eevee_next` as attr_load_uv and attr_load_color are also used for the grease pencil which uses a couple of global variables (g_uvs and g_color). These globals are set via another shader, so I am not really sure what should be done here. I haven't looked deeply at the surrounding code, so maybe there is an easy solution I have missed.

To be fair, the Grease Pencil object rendering is turned off for now until we have a per object option to use the render engine.
I am not sure how gpencil will be able to identify its attributes since they are nameless. Maybe we should have some builtin names to access them? Just like density for volume grids.

Revision Contents
Changeset List

PathSize
source/
blender/
draw/
engines/
eevee/
shaders/
18 lines
8 lines
18 lines
9 lines
intern/
16 lines
mesh_extractors/
7 lines
12 lines
shaders/
6 lines
gpu/
intern/
8 lines
12 lines
shaders/
material/
4 lines
nodes/
shader/
nodes/
2 lines
2 lines
6 lines
5 lines

Diff 51113

View Options

source/blender/draw/engines/eevee/shaders/shadow_vert.glsl

Loading...
View Options

source/blender/draw/engines/eevee/shaders/surface_frag.glsl

Loading...
View Options

source/blender/draw/engines/eevee/shaders/surface_vert.glsl

Loading...
View Options

source/blender/draw/engines/eevee/shaders/volumetric_frag.glsl

Loading...
View Options

source/blender/draw/intern/draw_cache_impl_particles.c

Loading...
View Options

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_uv.cc

Loading...
View Options

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_vcol.cc

Loading...
View Options

source/blender/draw/intern/shaders/common_attribute_lib.glsl

Loading...
View Options

source/blender/gpu/intern/gpu_codegen.cc

Loading...
View Options

source/blender/gpu/intern/gpu_node_graph.c

Loading...
View Options

source/blender/gpu/shaders/material/gpu_shader_material_uv_map.glsl

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_coord.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_image.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_uvmap.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_vertex_color.cc

Loading...