blender/blender

Vertex Paint: Add set vertex colors option to lock alpha #111002

Merged

Philipp Oeser merged 5 commits from lichtwerk/blender:110014 into main

2023-08-11 10:39:20 +02:00

Philipp Oeser commented

2023-08-10 13:58:39 +02:00

Member

It is a common practice in gamedev to rely on coding various data into
the alpha channel of the mesh and there was no way to preserve that when
using the Set Vertex Colors operator (would always sets the Alpha component to '1').

Now add an "Affect Alpha" option (similar to what we have for brushes)
and when that is disabled, existing alpha is locked.

It is a common practice in gamedev to rely on coding various data into the alpha channel of the mesh and there was no way to preserve that when using the `Set Vertex Colors` operator (would always sets the Alpha component to '1'). Now add an "Affect Alpha" option (similar to what we have for brushes) and when that is disabled, existing alpha is locked. Fixes #110014

Philipp Oeser added 1 commit 2023-08-10 13:58:54 +02:00

Fix #110014 : Set Vertex Colors always sets the Alpha component to '1' fbbe31618f

It is a common practice in gamedev to rely on coding various data into
the alpha channel of the mesh and there was no way to preserve that when
using the `Set Vertex Colors` operator.

Now add an "Affect Alpha" option (similar to what we have for brushes)
and when that is disabled, existing alpha is locked.

Philipp Oeser added this to the Sculpt, Paint & Texture project 2023-08-10 13:59:07 +02:00

Philipp Oeser added the

label 2023-08-10 13:59:18 +02:00

Philipp Oeser requested review from Julien Kaspar 2023-08-10 13:59:26 +02:00

Philipp Oeser requested review from Hans Goudey 2023-08-10 13:59:34 +02:00

Philipp Oeser requested review from Campbell Barton 2023-08-10 13:59:45 +02:00

Philipp Oeser referenced this pull request

2023-08-10 14:02:48 +02:00

Paint > Set Vertex color sets the Alpha component of RGBA attribute to '1' #110014

Julien Kaspar commented

2023-08-10 14:27:00 +02:00

Member

Works fine. The only issue I see is that the redo panel cannot undo the new alpha color that was set.
So if the "Affect Alpha" was disabled when executing the operation and then the option is toggled, it will properly fill the alpha as well.
But when "Affect Alpha" was enabled it will fill the Alpha and toggling the option will not undo this result.

Works fine. The only issue I see is that the redo panel cannot undo the new alpha color that was set. So if the "Affect Alpha" was disabled when executing the operation and then the option is toggled, it will properly fill the alpha as well. But when "Affect Alpha" was enabled it will fill the Alpha and toggling the option will not undo this result.

Philipp Oeser commented

2023-08-10 15:28:50 +02:00

Author

Member

Works fine. The only issue I see is that the redo panel cannot undo the new alpha color that was set.
So if the "Affect Alpha" was disabled when executing the operation and then the option is toggled, it will properly fill the alpha as well.
But when "Affect Alpha" was enabled it will fill the Alpha and toggling the option will not undo this result.

I think I see what is going on, this needs special sculpt undo handling (similar to what transform_active_color is doing -- or using it directly).

> Works fine. The only issue I see is that the redo panel cannot undo the new alpha color that was set. > So if the "Affect Alpha" was disabled when executing the operation and then the option is toggled, it will properly fill the alpha as well. > But when "Affect Alpha" was enabled it will fill the Alpha and toggling the option will not undo this result. I think I see what is going on, this needs special sculpt undo handling (similar to what `transform_active_color` is doing -- or using it directly).

Philipp Oeser changed title from ~~Fix #110014: Set Vertex Colors always sets the Alpha component to '1'~~ to WIP: Fix #110014: Set Vertex Colors always sets the Alpha component to '1'

2023-08-10 15:29:00 +02:00

Hans Goudey approved these changes 2023-08-10 16:13:31 +02:00

Hans Goudey left a comment

Member

Seems reasonable! As far as I can tell this is more of a new feature than a bug fix, might be nice for the commit title to reflect that. Also, what is making the PR WIP? Generally that means it's not ready for review.

Seems reasonable! As far as I can tell this is more of a new feature than a bug fix, might be nice for the commit title to reflect that. Also, what is making the PR WIP? Generally that means it's not ready for review.

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

						
				@ -2250,0 +2261,4 @@

				                  "affect_alpha",

				                  true,

				                  "Affect Alpha",

				                  "When this is disabled, lock existing alpha when setting vertex colors");

Hans Goudey commented

2023-08-10 16:11:56 +02:00

Member

UI descriptions of booleans should generally be phrased to describe what the option does when it's turned on, i.e. "Chane the alpha channel of colors"

https://wiki.blender.org/wiki/Human_Interface_Guidelines/Tooltips

I think that's clear enough, it's not a complex idea

UI descriptions of booleans should generally be phrased to describe what the option does when it's turned on, i.e. "Chane the alpha channel of colors" https://wiki.blender.org/wiki/Human_Interface_Guidelines/Tooltips I think that's clear enough, it's not a complex idea

lichtwerk marked this conversation as resolved

Philipp Oeser added 1 commit 2023-08-10 16:34:23 +02:00

proper (needs special sculpt) undo 9a122529c5

this is basically using the undo strategy from transform_active_color
(16cb13b8a0).

NOTE: it would be even possible to use transform_active_color() all the
way, but this would require more refactoring and additionally, current
code in fill_mesh_color() does a cast which alters Alpha mode (not sure
if this is desired even, but wouldnt want to change this behavior with
this patch) which does not happen in transform_active_color() -- I think
it would be best to look at this separately and possibly unify code at a
later point.

Philipp Oeser added 1 commit 2023-08-10 16:41:20 +02:00

Improve UI description of option 966f7b428d

Philipp Oeser changed title from ~~WIP: Fix #110014: Set Vertex Colors always sets the Alpha component to '1'~~ to Vertex Paint Set Vertex Colors: add option to lock alpha

2023-08-10 16:46:06 +02:00

Philipp Oeser commented

2023-08-10 16:46:18 +02:00

Author

Member

Seems reasonable! As far as I can tell this is more of a new feature than a bug fix, might be nice for the commit title to reflect that.

Changed

Also, what is making the PR WIP? Generally that means it's not ready for review.

This was because of the Undo issue (also see @JulienKaspar comment), would be nice if you could check that again.

>Seems reasonable! As far as I can tell this is more of a new feature than a bug fix, might be nice for the commit title to reflect that. Changed >Also, what is making the PR WIP? Generally that means it's not ready for review. This was because of the Undo issue (also see @JulienKaspar comment), would be nice if you could check that again.

Philipp Oeser requested review from Hans Goudey 2023-08-10 16:46:35 +02:00

Julien Kaspar approved these changes 2023-08-10 17:01:32 +02:00

Julien Kaspar left a comment

Member

Works like expected to me.

Works like expected to me.

Campbell Barton approved these changes 2023-08-11 00:50:40 +02:00

Campbell Barton reviewed 2023-08-11 00:57:09 +02:00

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

						
				@ -2249,1 +2277,4 @@

				  ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;

				  RNA_def_boolean(ot->srna,

				                  "affect_alpha",

Campbell Barton commented

2023-08-11 00:57:09 +02:00

Owner

Prefer calling this "alpha", as we have for bpy.data.images.new(..) & IMAGE_OT_tile_add... or use_alpha.

Prefer calling this "alpha", as we have for `bpy.data.images.new(..)` & `IMAGE_OT_tile_add`... or `use_alpha`.

lichtwerk marked this conversation as resolved

Hans Goudey approved these changes 2023-08-11 07:51:18 +02:00

Hans Goudey reviewed 2023-08-11 07:53:19 +02:00

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

						
				@ -2250,0 +2280,4 @@

				                  "affect_alpha",

				                  true,

				                  "Affect Alpha",

				                  "Set to full Alpha, lock existing alpha when disabled");

Hans Goudey commented

2023-08-11 07:53:19 +02:00

Member

Sorry to be picky, the grammar isn't quite right here though, since these are two sentences separated with a comma, which is also a bit awkward. What about something more like "Set color completely opaque instead of reusing existing alpha"?

Sorry to be picky, the grammar isn't quite right here though, since these are two sentences separated with a comma, which is also a bit awkward. What about something more like "Set color completely opaque instead of reusing existing alpha"?

lichtwerk marked this conversation as resolved

Hans Goudey changed title from ~~Vertex Paint Set Vertex Colors: add option to lock alpha~~ to Vertex Paint: Add set vertex colors option to lock alpha

2023-08-11 07:53:49 +02:00

Philipp Oeser added 2 commits 2023-08-11 10:35:52 +02:00

Merge branch 'main' into 110014 316560ba80

address review 4d1360e8d6

- rename bool
- improve tooltip

Philipp Oeser merged commit 3c26f84225 into main

2023-08-11 10:39:20 +02:00

Philipp Oeser deleted branch 110014

2023-08-11 10:39:21 +02:00

Philipp Oeser referenced this issue from a commit

2023-08-11 10:39:22 +02:00

Vertex Paint: Add option to lock alpha for 'Set Vertex Colors'

Julien Kaspar removed this from the Sculpt, Paint & Texture project 2023-08-25 11:35:34 +02:00

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111002

No description provided.