Details

Reviewers

Kévin Dietrich (kevindietrich)
Jeroen Bakker (jbakker)

Commits

rB1998269b109f: Refactor: Extract color attributes as generic attributes

Summary

Previously there was a special extraction process for "vertex colors"
that copied the color data to the GPU with a special format. Instead,
this patch replaces this with use of the generic attribute extraction.
This reduces the number of code paths, allowing easier optimization
in the future.

To make it possible to use the generic extraction system for attributes
but also assign aliases for use by shaders, some changes are necessary.
First, the GPU material attribute can now store whether it actually refers
to the default color attribute, rather than a specific name. This replaces
the hack to use CD_MCOL in the color attribute shader node. Second,
the extraction code checks the names against the default and active
names and assigns aliases if the request corresponds to a special active
attribute. Finally, support for byte color attributes was added to the
generic attribute extraction.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jun 15 2022, 1:51 PM

Hans Goudey (HooglyBoogly) created this revision.

Add format aliases

Cleanup: Remove unnecessary changes

For vertex paint (or object mode), issue is that is_active_color and is_default_color from DRWAttributeRequest are not always initialized, as they are initialized to true accordingly in GPU material code paths, but there is no GPU material for vertex paint.

This fixes the issue:

diff --git a/source/blender/draw/intern/draw_cache_impl_mesh.cc b/source/blender/draw/intern/draw_cache_impl_mesh.cc
index afcf48c7ac3..ff7420d7b9f 100644
--- a/source/blender/draw/intern/draw_cache_impl_mesh.cc
+++ b/source/blender/draw/intern/draw_cache_impl_mesh.cc
@@ -984,22 +984,30 @@ static void request_active_and_default_color_attributes(DRW_Attributes &attribut
   BKE_id_attribute_copy_domains_temp(
       ID_ME, cd_vdata, nullptr, cd_ldata, nullptr, nullptr, &me_query.id);
 
-  auto request_color_attribute = [&](const char *name) {
+  auto request_color_attribute = [&](const char *name, bool is_active, bool is_default) {
     int layer_index;
     eCustomDataType type;
     if (drw_custom_data_match_attribute(cd_vdata, name, &layer_index, &type)) {
-      drw_attributes_add_request(&attributes, type, layer_index, ATTR_DOMAIN_POINT);
+      auto req = drw_attributes_add_request(&attributes, type, layer_index, ATTR_DOMAIN_POINT);
+      if (req) {
+        req->is_active_color = is_active;
+        req->is_default_color = is_default;
+      }
     }
     else if (drw_custom_data_match_attribute(cd_ldata, name, &layer_index, &type)) {
-      drw_attributes_add_request(&attributes, type, layer_index, ATTR_DOMAIN_CORNER);
+      auto req = drw_attributes_add_request(&attributes, type, layer_index, ATTR_DOMAIN_CORNER);
+      if (req) {
+        req->is_active_color = is_active;
+        req->is_default_color = is_default;
+      }
     }
   };
 
   if (const CustomDataLayer *active = BKE_id_attributes_active_color_get(&me_query.id)) {
-    request_color_attribute(active->name);
+    request_color_attribute(active->name, true, false);
   }
   if (const CustomDataLayer *render = BKE_id_attributes_render_color_get(&me_query.id)) {
-    request_color_attribute(render->name);
+    request_color_attribute(render->name, false, true);
   }
 }

Also, byte color is missing from the attribute extraction, so this should added (the patch description is not clear on that, but I understand it is WIP).

Fix texture paint mode with Kevin's patch
Fix object mode and render view by removing use of CD_MCOL further
Add support for Byte colors to attribute extraction

I want to simplify the DRW_AttributeRequest process a bit more.

Simplify the attribute request process -- only store the attribute name there, extract active data separately

Merge master

Jacob Merrill (blueprintrandom) awarded a token.Jun 18 2022, 8:36 PM

This patch does not seem to compile, see inline comment.

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_attributes.cc
159–164	Apparently `mr` is undefined here.

This revision now requires changes to proceed.Jun 20 2022, 12:26 PM

Right, that comes from a cleanup in master, sorry about that.

Hans Goudey (HooglyBoogly) marked an inline comment as done.Jun 20 2022, 2:25 PM

Overall this looks good, and seems to work fine. I only have a couple of minor comments.

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_attributes.cc
345–350	This is useless, the coarse VBO does not need aliases, it is only used for interpolation. Aliases are only for the final buffer.
386–387	I noticed there is a bug in master, the last argument, `false`, should be `request.cd_type == CD_PROP_COLOR` or `ELEM(request.cd_type, CD_PROP_COLOR, CD_PROP_BYTE_COLOR)` with your patch, see rBddc6b86a5bab833c7599f5545b9aff0ad7349e02. So you would need to update the patch to take byte colors into account here.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Jun 27 2022, 6:00 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_attributes.cc
345–350	Good point, thanks.

Fixes/cleanup from review comments

Hans Goudey (HooglyBoogly) mentioned this in T98731: Vertex Paint mode can edit only 8 color attributes.Jul 1 2022, 9:20 PM

Sorry for the delay.

There are a couple issues though:

the patch would need to be rebased on master as it does not apply anymore (although the fix is minimal as it is caused by rB4cba209edd39: GPUMaterial: Remove the max attribute check).
vertex color painting is not working with GPU subdivision, see inline comment

Apart from that, everything seems to be in order, so I'll accept the patch, as I trust I don't have to come back to check that the issues are fixed. Adding @Jeroen Bakker (jbakker) as blocking reviewer though.