GPv3: Vertex Paint Mode #125674

I'd suggest changing the grid initialization loops for the smear tool, but other optimizations can be done separately if and when needed. I expect most time in larger drawings will be spent in the common point mask constructors anyway.

Good work!

Found just some minor formatting errors. Works nicely in my feeble programmer art attempts. I'd suggest changing the grid initialization loops for the smear tool, but other optimizations can be done separately if and when needed. I expect most time in larger drawings will be spent in the common point mask constructors anyway. Good work!

scripts/startup/bl_ui/space_view3d.py

						
				@ -2372,0 +2406,4 @@

				    def draw(self, _context):

				        layout = self.layout

				        # layout.operator("gpencil.vertex_color_set", text="Set Color Attribute")

Lukas Tönne commented

2024-08-23 19:14:18 +02:00

Can be removed, or todo?

Falk David commented

2024-08-23 21:40:25 +02:00

I have a branch that implements the vertex paint operators. I can remove the commented code in this PR though.

filedescriptor marked this conversation as resolved

source/blender/editors/gpencil_legacy/gpencil_edit.cc Outdated

						
				@ -793,1 +797,3 @@

				    mode = OB_MODE_VERTEX_GPENCIL_LEGACY;

				    /* Just toggle paintmode flag... */

				    gpd->flag ^= GP_DATA_STROKE_VERTEXMODE;

				    /* set mode */

Lukas Tönne commented

2024-08-23 19:22:17 +02:00

Comment formatting

filedescriptor marked this conversation as resolved

source/blender/editors/gpencil_legacy/gpencil_edit.cc Outdated

						
				@ -802,1 +818,3 @@

				      mode = ob->restore_mode;

				    if (ob->type == OB_GPENCIL_LEGACY) {

				      bGPdata *gpd = static_cast<bGPdata *>(ob->data);

				      /* try to back previous mode */

Lukas Tönne commented

2024-08-23 19:23:12 +02:00

formatting

filedescriptor marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_draw_ops.cc Outdated

						
				@ -371,3 +388,3 @@

				static void GREASE_PENCIL_OT_sculpt_paint(wmOperatorType *ot)

				{

				  ot->name = "Grease Pencil Draw";

				  ot->name = "Grease Pencil Sculpt";

Lukas Tönne commented

2024-08-23 19:32:10 +02:00

Unrelated fix?

👍 1

filedescriptor marked this conversation as resolved

source/blender/editors/sculpt_paint/grease_pencil_vertex_smear.cc Outdated

						
				@ -0,0 +105,4 @@

				      /* Compute the colors in the grid by averaging the vertex colors of the points that

				       * intersect each cell. */

				      Array<int> points_per_cell(grid_array_length, 0);

				      for (const int y : IndexRange(color_grid_.size)) {

Lukas Tönne commented

2024-08-23 20:25:33 +02:00

Hmm, doing a single loop over the point selection should be faster and simpler here, especially if it's single-threaded. I know i suggested doing this per grid cell in the first place, sorry 😁My thinking was that grid cells could be filled in parallel that way, but it's actually not necessary if the point selection mask construction is the slowest part (which it probably is). I would expect a lot more points than grid cells, so avoiding multiple loops over points is probably best.

If necessary i think this can be optimized with a parallel_reduce or EnumableThreadSpecific:

Local color grid per thread.
Threaded loop over points:
- Find grid cell index
- Add point to local grid
Accumulate grids

Hmm, doing a single loop over the point selection should be faster and simpler here, especially if it's single-threaded. I know i suggested doing this per grid cell in the first place, sorry 😁My thinking was that grid cells could be filled in parallel that way, but it's actually not necessary if the point selection mask construction is the slowest part (which it probably is). I would expect a lot more points than grid cells, so avoiding multiple loops over points is probably best. If necessary i think this can be optimized with a `parallel_reduce` or `EnumableThreadSpecific`: - Local color grid per thread. - Threaded loop over points: * Find grid cell index * Add point to local grid - Accumulate grids

filedescriptor marked this conversation as resolved

Iliya Katushenock reviewed 2024-08-24 09:09:53 +02:00

source/blender/editors/grease_pencil/intern/grease_pencil_utils.cc

						
				@ -918,0 +969,4 @@

				        /* The stroke is editable if the material is editable. If the material is not editable,

				         * then the stroke is only editable if the layer disables the locked material option. */

				        return (editable_material_indices.contains(material_index) ||

				                layer.use_locked_material()) &&

Iliya Katushenock commented

2024-08-24 09:09:53 +02:00

Move this condition outside of the predicate and just switch between 2 indexmask constructor calls.

Falk David commented

2024-09-10 17:07:21 +02:00

Opened a separate PR to handle this #127423. Will update this function to use this option.

Opened a separate PR to handle this https://projects.blender.org/blender/blender/pulls/127423. Will update this function to use this option.

Falk David added 5 commits 2024-09-09 11:29:17 +02:00

Merge branch 'main' into gpv3-vertex-paint-mode 4b47d9a620

Remove dead code 67e2a24a63

Cleanup comments b54cac1f9d

Revert unrelated fix 2b61547c03

Fix compile errors c2fe9c9ccd

Falk David added 5 commits 2024-09-10 15:14:33 +02:00

Iterate over points first when initializing smear grid f10f9d1d73

Cleanup: Use early returns 93b71b1ced

Cleanup: remove callback function 601161ad09

Cleanup: Move helper functions into ColorGrid 251b3d6e72

Merge branch 'main' into gpv3-vertex-paint-mode 1ce38d2e50

Falk David requested review from Lukas Tönne 2024-09-10 15:16:25 +02:00

Lukas Tönne approved these changes 2024-09-10 17:37:48 +02:00

Lukas Tönne left a comment

There may still be some bias in the smear brush calculations, but it's difficult to be sure without extensive testing. Should be fine for now.

source/blender/editors/sculpt_paint/grease_pencil_vertex_smear.cc Outdated

						
				@ -0,0 +36,4 @@

				  int2 coords_to_pos(const float2 coord, const float2 center) const

				  {

				    const int2 pos = int2(math::floor((coord - center) / float(this->cell_size_px)));

				    return pos + this->size / 2;

Lukas Tönne commented

2024-09-10 17:33:12 +02:00

Should this be ceil(size/2) (or (size + 1)/2)? I think this could discard points on the fringes if the grid size is odd.

Example (just one axis):

Cell size 1.0 for simplicity
Grid size 5
coordinate -2.1 -> position -3
Relative to corner: -3 + int(5/2) = -1
Now the point is considered outside the grid. The corner position relative to the center should be rounded up, so the relative position becomes -3 + int((5+1)/2) = 0

Should this be `ceil(size/2)` (or `(size + 1)/2`)? I think this could discard points on the fringes if the grid size is odd. Example (just one axis): - Cell size `1.0` for simplicity - Grid size `5` - coordinate `-2.1` -> position `-3` - Relative to corner: `-3 + int(5/2) = -1` Now the point is considered outside the grid. The corner position relative to the center should be rounded up, so the relative position becomes `-3 + int((5+1)/2) = 0`

filedescriptor marked this conversation as resolved

Falk David added 1 commit 2024-09-10 18:04:32 +02:00

Fix "coords_to_pos"

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.