Fix: Vertex paint filter operators broken undo #106136

Joseph Eagar · 2023-03-25T19:22:45+01:00

Joseph Eagar commented

2023-03-25 19:22:45 +01:00

Vertex paint's filter operators were not properly going through the SculptSession API.
Undo didn't work and sometimes the operators would do nothing or produce corrupted colors.

The transform_active_color helper function now handles all of this. It ensures a valid
SculptSession exists and executes an undo push.

Vertex paint's filter operators were not properly going through the SculptSession API. Undo didn't work and sometimes the operators would do nothing or produce corrupted colors. The `transform_active_color` helper function now handles all of this. It ensures a valid SculptSession exists and executes an undo push.

Joseph Eagar added 1 commit 2023-03-25 19:23:00 +01:00

db8141df4e Sculpt: Fix vertex paint filter operators

Refactored vpaint filter ops to properly set up
a SculptSession and execute an undo push.

Hans Goudey requested changes 2023-03-25 21:00:34 +01:00

Hans Goudey left a comment

Mostly looks reasonable, just a request for an improved comment and another small tweak mentioned inline.

source/blender/blenkernel/intern/paint.cc Outdated

						
				@ -1848,2 +1848,4 @@

				  if (is_paint_tool) {

				    if (ss->vcol_domain == ATTR_DOMAIN_CORNER) {

				      /* Ensure pbvh nodes have loop indices, needed for undo. */

Hans Goudey commented

2023-03-25 20:52:03 +01:00

This sort of comment ends up being confusing a year later, it says "needed for undo" but that brings up more questions than it answers-- why would "undo" need this data? what is meant by "undo" here? etc.

I don't know the actual reason, but I'd expect something like "needed in order to interpolate from face corners to vertices for undo step storage"

And one more question, could the sculpt undo system call this function directly rather than doing it here?

This sort of comment ends up being confusing a year later, it says "needed for undo" but that brings up more questions than it answers-- why would "undo" need this data? what is meant by "undo" here? etc. I don't know the actual reason, but I'd expect something like "needed in order to interpolate from face corners to vertices for undo step storage" And one more question, could the sculpt undo system call this function directly rather than doing it here?

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

						
				@ -13,6 +13,7 @@

				#include "BLI_array.hh"

				#include "BLI_index_mask_ops.hh"

				#include "BLI_index_range.hh"

Hans Goudey commented

2023-03-25 20:53:01 +01:00

Not sure where this BKE_mesh_mapping.h include is coming from, it doesn't look necessary here.

Not sure where this `BKE_mesh_mapping.h` include is coming from, it doesn't look necessary here.

source/blender/editors/sculpt_paint/paint_vertex_color_ops.cc

						
				@ -291,0 +306,4 @@

				  int nodes_num;

				  BKE_pbvh_search_gather(obact->sculpt->pbvh, nullptr, nullptr, &nodes, &nodes_num);

				  for (int i : IndexRange(nodes_num)) {

Hans Goudey commented

2023-03-25 20:55:56 +01:00

I'm not sure, but it seems more clear/common to add the update tags directly after the data is changed rather than before.

Thomas Dinges commented

2023-03-25 21:28:10 +01:00

Do you consider this a regression? At this point (4 days before the release) only critical issues should be fixed in 3.5.

Hans Goudey referenced this pull request

2023-03-25 21:30:25 +01:00

Fix #106094: Snap curves to surface operator does not update geometry #106104

Joseph Eagar added 1 commit 2023-03-25 21:57:06 +01:00

4347e72de7 blender-v3.5-vpaint_fixes: Make requested patch changes

Hans Goudey reviewed 2023-03-25 21:59:53 +01:00

source/blender/editors/sculpt_paint/paint_vertex_color_ops.cc

Hans Goudey commented

2023-03-25 21:59:33 +01:00

This header is still added in this patch.

Joseph Eagar added 1 commit 2023-03-25 22:01:44 +01:00

2fd5295d1d Remove header

Hans Goudey approved these changes 2023-03-25 22:05:29 +01:00

Hans Goudey left a comment

Fix commit titles should begin with Fix:, so maybe something like Fix: Vertex paint filter operators broken undo

Fix commit titles should begin with `Fix:`, so maybe something like `Fix: Vertex paint filter operators broken undo`

Joseph Eagar commented

2023-03-25 22:06:51 +01:00

Do you consider this a regression? At this point (4 days before the release) only critical issues should be fixed in 3.5.

It's a regression, yes, it makes the operators pretty much unusable.

> Do you consider this a regression? At this point (4 days before the release) only critical issues should be fixed in 3.5. It's a regression, yes, it makes the operators pretty much unusable.

👍 1

Joseph Eagar changed title from ~~Sculpt: Fix vertex paint filter operators~~ to Fix: Vertex paint filter operators broken undo

2023-03-25 22:07:47 +01:00

Joseph Eagar merged commit 16cb13b8a0 into blender-v3.5-release

2023-03-25 22:10:20 +01:00

Joseph Eagar referenced this issue from a commit

2023-03-25 22:10:21 +01:00

Fix: Vertex paint filter operators broken undo

Joseph Eagar deleted branch blender-v3.5-vpaint_fixes

2023-03-25 22:10:21 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix: Vertex paint filter operators broken undo #106136