Workbench Next: Sculpt Support #109294

Miguel Pozo · 2023-06-23T16:11:52+02:00

Miguel Pozo commented

2023-06-23 16:11:52 +02:00

Add Sculpt support for the new Draw Manager and implement Sculpt rendering in Workbench Next.
Instead of using a callback approach, it's now possible to retrieve a vector of GPUBatches and their material index, so engines can use them as they see fit.

Add Sculpt support for the new Draw Manager and implement Sculpt rendering in Workbench Next. Instead of using a callback approach, it's now possible to retrieve a vector of GPUBatches and their material index, so engines can use them as they see fit.

Miguel Pozo added 2 commits 2023-06-23 16:12:00 +02:00

615c7a04e4 Single Color implementation

441b8c5897 Multi material support

Miguel Pozo added this to the EEVEE & Viewport project 2023-06-23 16:12:43 +02:00

Miguel Pozo added the

Module

EEVEE & Viewport

label 2023-06-23 16:12:56 +02:00

Miguel Pozo requested review from Clément Foucault 2023-06-23 16:13:13 +02:00

Clément Foucault requested changes 2023-06-23 17:52:42 +02:00

Clément Foucault left a comment

A few cosmetic changes but otherwise looks good to me.

source/blender/draw/intern/draw_sculpt.cc

						
				@ -0,0 +136,4 @@

				  int attrs_len = 0;

				  /* NOTE: these are NOT #eCustomDataType, they are extended values, ASAN may warn about this. */

				  attrs[attrs_len++].type = (eCustomDataType)CD_PBVH_CO_TYPE;

Clément Foucault commented

2023-06-23 17:44:48 +02:00

Use functional cast for all eCustomDataType.

Use functional cast for all `eCustomDataType`.

Clément Foucault commented

2023-06-23 17:44:49 +02:00

Use functional cast for all eCustomDataType.

Use functional cast for all `eCustomDataType`.

pragma37 marked this conversation as resolved

source/blender/draw/intern/draw_sculpt.cc Outdated

						
				@ -0,0 +171,4 @@

				    }

				  }

				  return sculpt_batches_get_ex(ob, features & SCULPT_BATCH_WIREFRAME, false, attrs, attrs_len);

Clément Foucault commented

2023-06-23 17:38:50 +02:00

The feature masking here is not obvious nor safe (bool could be different from 0/1). Prefer defining bool use_wire = features & SCULPT_BATCH_WIREFRAME != 0 and use as sculpt_batches_get_ex argument.

The feature masking here is not obvious nor safe (`bool` could be different from 0/1). Prefer defining `bool use_wire = features & SCULPT_BATCH_WIREFRAME != 0` and use as `sculpt_batches_get_ex` argument.

Miguel Pozo commented

2023-06-23 21:03:28 +02:00

I personally find flags & mask more readable than (flags & mask) != 0.
This idiom is used all over the place, including the original functions from draw_mamager_data.
Not sure why this one triggers your spider sense. 🤔

Regarding safety, it's guaranteed by the standard to be true.
AFAIK, the binary representation of a true boolean is not guaranteed in any case, but that's only relevant for serialization.
In any case, all the compilers we support do "the right thing".
https://godbolt.org/z/xbv5e6hcd

I personally find `flags & mask` more readable than `(flags & mask) != 0`. This idiom is used all over the place, including the original functions from `draw_mamager_data`. Not sure why this one triggers your spider sense. 🤔 Regarding safety, it's guaranteed by the standard to be `true`. AFAIK, the binary representation of a `true` boolean is not guaranteed in any case, but that's only relevant for serialization. In any case, all the compilers we support do "the right thing". https://godbolt.org/z/xbv5e6hcd

Clément Foucault commented

2023-06-24 01:05:44 +02:00

Might have been some gidelines we had when code was mostly C. Disregard that then.

fclem marked this conversation as resolved

source/blender/draw/intern/draw_sculpt.hh

						
				@ -0,0 +19,4 @@

				  SCULPT_BATCH_DEFAULT = 0,

				  SCULPT_BATCH_WIREFRAME = 1 << 0,

				  SCULPT_BATCH_MASK = 1 << 1,

				  SCULPT_BATCH_FSET = 1 << 2,

Clément Foucault commented

2023-06-23 17:33:28 +02:00

Avoid contraction. SCULPT_BATCH_FACE_SET.

pragma37 marked this conversation as resolved

Clément Foucault commented

2023-06-23 17:59:21 +02:00

About the debug code, I would maybe make it part of workbench-next or overlay-next (preferably) instead.

Miguel Pozo added 3 commits 2023-06-23 20:36:54 +02:00

2e6409371e Code Style

fe3d0dad6d Debug Draw

c8fe28e031 Disable Frustum Culling

Miguel Pozo commented

2023-06-23 21:06:09 +02:00

The mesh color part can only be done on the Workbench side, so I implemented only that.
It matches the intended current behavior (it actually doesn't work in the current Workbench),
but I find kind of weird that the bounding box color doesn't match the mesh color.
We may want to look into that when we do the Overlay Next part.

The mesh color part can only be done on the Workbench side, so I implemented only that. It matches the intended current behavior (it actually doesn't work in the current Workbench), but I find kind of weird that the bounding box color doesn't match the mesh color. We may want to look into that when we do the Overlay Next part.

Clément Foucault approved these changes 2023-06-24 01:12:01 +02:00

source/blender/draw/intern/draw_sculpt.cc

						
				@ -0,0 +63,4 @@

				  }

				  batch.material_slot = drw_pbvh_material_index_get(batches);

				  /** NOTE: This doesn't match the index used in DRW_sculpt_debug_cb (debug_node_nr). */