Page MenuHome
Differential D14455

Painting: Canvas switcher for painting brushes/tools.
ClosedPublic
Actions

Authored by Jeroen Bakker (jbakker) on Mar 25 2022, 3:38 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Julien Kaspar (JulienKaspar)
Joseph Eagar (joeedh)
Maniphest Tasks
T96709: Select painting target for paint brushes
Commits
rB8b7cd1ed2a17: Painting: Canvas switcher for painting brushes/tools.
Summary

This patch adds color attributes to TexPaintSlot. This allows an easier selection
when painting color attributes.

Previously when selecting a paint tool the user had to start a stroke, before the
UI reflected the correct TexPaintSlot. Now when switching the slot the active
tool is checked and immediate the UI is drawn correctly.

In the future the canvas selector will also be used to select an image or image texture node
to paint on. Basic implementation has already been done inside this patch.

A limitation of this patch is that is isn't possible anymore to rename images directly from
the selection panel. This is currently allowed in master. But as CustomDataLayers
aren't ID fields and not owned by the material supporting this wouldn't be easy.

In the future we should update the create slot operator to also include color attributes.
Sources could also be extended to use other areas of the object that use image textures
(particles, geom nodes, etc... ).

Diff Detail

Repository
rB Blender
Branch
temp-T96709-painting-target
Build Status
Buildable 21273
Build 21273: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jeroen Bakker (jbakker) added inline comments.Mar 29 2022, 3:08 PM
source/blender/blenkernel/BKE_material.h
109 ↗(On Diff #49838)

Use flags to select what types should be created.
Also add a flag for image slots.

source/blender/editors/sculpt_paint/paint_canvas.cc
11 ↗(On Diff #49838)

Cleanup unused imports.

source/blender/makesdna/DNA_material_types.h
34 ↗(On Diff #49838)

Use doc style comments.

source/blender/makesrna/intern/rna_object.c
47

Revert change

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 29 2022, 3:10 PM
Jeroen Bakker (jbakker) updated this revision to Diff 49844.Mar 29 2022, 3:55 PM
  • Revert indentation.
  • Cleaup include statements.
  • Updated comments.
  • Remove unused include.
  • Use flag to update paint slots.
Harbormaster completed remote builds in B21324: Diff 49844.Mar 29 2022, 3:55 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 29 2022, 3:57 PM
Jeroen Bakker (jbakker) updated this revision to Diff 49845.Mar 29 2022, 4:03 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
  • Rename helper class.
Harbormaster completed remote builds in B21325: Diff 49845.Mar 29 2022, 4:03 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 29 2022, 4:07 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) added inline comments.Mar 29 2022, 4:24 PM
release/scripts/startup/bl_ui/space_view3d_toolbar.py
459

Rename to SelectPaintSlotHelper

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 29 2022, 5:07 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 29 2022, 5:13 PM
Jeroen Bakker (jbakker) updated this revision to Diff 49850.Mar 29 2022, 5:13 PM
  • Show icon of tex paint slot.
Harbormaster completed remote builds in B21326: Diff 49850.Mar 29 2022, 5:14 PM
Jeroen Bakker (jbakker) added inline comments.Mar 29 2022, 9:39 PM
release/scripts/startup/bl_ui/space_view3d_toolbar.py
283

This removes the option to rename the images directly from the panel. Could still be supported.

source/blender/blenkernel/BKE_material.h
10 ↗(On Diff #49850)

Revert new line.

22 ↗(On Diff #49850)

Enum doesn't need to be public visible.

source/blender/draw/engines/workbench/workbench_engine.c
325

ED_paint_shading_color_override

source/blender/editors/include/ED_paint.h
15

Remove EnumPropertyItem.

source/blender/editors/sculpt_paint/CMakeLists.txt
35

paint_slot.cc

source/blender/makesrna/intern/rna_sculpt_paint.c
1031

comment style.

1043

comment style. mode=canvas

Erick Tukuniata (erickblender) awarded a token.Mar 29 2022, 11:33 PM
Jeroen Bakker (jbakker) updated this revision to Diff 49891.Mar 30 2022, 8:19 AM
  • Merge remote-tracking branch 'origin/master' into temp-T96709-painting-target
  • Small changes to BKE_material API.
  • Rename to ED_paint_shading_color_override.
  • Remove unneeded forward declaration.
  • Comments.
  • TexPaintSlot comments.
Harbormaster completed remote builds in B21347: Diff 49891.Mar 30 2022, 8:19 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 30 2022, 8:24 AM
Jeroen Bakker (jbakker) updated this revision to Diff 49903.Mar 30 2022, 11:17 AM
  • Update paint slots when object changes.
  • Fix refreshing after load.
Harbormaster completed remote builds in B21355: Diff 49903.Mar 30 2022, 11:17 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 30 2022, 11:17 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mar 30 2022, 11:21 AM
Jeroen Bakker (jbakker) added inline comments.Mar 30 2022, 11:24 AM
source/blender/editors/sculpt_paint/CMakeLists.txt
35

Not done as it also refers to a higher level than paint slots.

Jeroen Bakker (jbakker) added inline comments.Mar 30 2022, 6:44 PM
source/blender/draw/engines/workbench/workbench_engine.c
98

remove line. it should not be asserted anymore.

Jeroen Bakker (jbakker) added a child revision: D14504: PBVH Pixel extractor..Apr 1 2022, 1:41 PM
Jeroen Bakker (jbakker) mentioned this in D14504: PBVH Pixel extractor..
Jeroen Bakker (jbakker) updated this revision to Diff 50074.Apr 4 2022, 2:19 PM
  • Merge branch 'master' into temp-T96709-painting-target
Harbormaster completed remote builds in B21451: Diff 50074.Apr 4 2022, 2:19 PM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Apr 4 2022, 2:38 PM
Brecht Van Lommel (brecht) requested changes to this revision.Apr 4 2022, 8:40 PM

Personally, I'm not sure if PAINT_CANVAS_SOURCE_IMAGE is something worth supporting at all, if it's actually useful in practice or just a legacy thing from before there was more convenient material integration.

release/scripts/startup/bl_ui/space_view3d_toolbar.py
560

I get an error with this panel in the properties editor.

  File "/home/brecht/dev/build_linux/bin/3.2/scripts/startup/bl_ui/space_view3d_toolbar.py", line 560, in poll
    return context.space_data.uses_paint_canvas()
AttributeError: 'SpaceProperties' object has no attribute 'uses_paint_canvas'

I'm not sure this worth adding to the RNA API instead of duplicating logic here, since this is only temporary while vertex colors are in sculpt mode?

source/blender/editors/sculpt_paint/paint_canvas.cc
50 ↗(On Diff #50074)

It's not obvious to me that making this color override depend on the active tool works well. When using masking or face set tools, it's not great if the shading suddenly changes.

However, I guess this is only a temporary thing until we move these tools out of sculpt mode and into a dedicated paint mode? Or is this meant to be a more permanent thing?

54 ↗(On Diff #50074)

Surprised that using a keyword as a variable name works, maybe best to avoid it.

source/blender/editors/space_view3d/space_view3d.c
1386 ↗(On Diff #50074)

This doesn't seem right. It's listening to change to workspace tools, to call DEG_id_tag_update for an object to be updated, and doing that as part of 3D viewport code.

What is this used for exactly? We don't normally tag depsgraph updates through this kind of mechanism, and doing such tags for every tool change could be a performance problem.

source/blender/makesrna/intern/rna_material.c
312 ↗(On Diff #50074)

Don't hardcode 64.

317 ↗(On Diff #50074)

Don't hardcode 64.

1033 ↗(On Diff #50074)

Are you sure it uses the pointer size? As far as I know defining rna_TexPaintSlot_name_length should make it a dynamically sized string, and there should be no need to define a maximum length.

source/blender/makesrna/intern/rna_sculpt_paint.c
1032

I think canvas_source is a better name, not sure why it's a different name internally and in the Python API.

1044–1045

Maybe canvas_image is a better name both internally and in the API?

source/blender/makesrna/intern/rna_space_api.c
83 ↗(On Diff #50074)

I don't think this belongs in SpaceView3D, seems more like a property of the mode or tool.

This revision now requires changes to proceed.Apr 4 2022, 8:40 PM
Jeroen Bakker (jbakker) updated this revision to Diff 50118.Apr 5 2022, 3:26 PM
Jeroen Bakker (jbakker) marked 10 inline comments as done.
  • Merge remote-tracking branch 'origin/master' into temp-T96709-painting-target
  • Don't use override as variable name.
  • Added sticky_shading_color (temporarily until paint mode has been added).
  • Remove commented out code in workbench.
  • Replace depsgraph update with pbvh node tagging.
  • WIP. switching machines for proper GPU debugging session.
  • Don't trigger depsgraph update when switching tools.
  • Use MAX_NAME.
  • Change RNA API.
  • Fix memory leak.
Harbormaster completed remote builds in B21473: Diff 50118.Apr 5 2022, 3:26 PM
Jeroen Bakker (jbakker) added a subscriber: Simon Thommes (simonthommes).EditedApr 5 2022, 3:26 PM
In D14455#390489, @Brecht Van Lommel (brecht) wrote:

Personally, I'm not sure if PAINT_CANVAS_SOURCE_IMAGE is something worth supporting at all, if it's actually useful in practice or just a legacy thing from before there was more convenient material integration.

A niche case for PAINT_CANVAS_SOURCE_IMAGE, would be to paint on an image texture that is used in geometry nodes, without PAINT_CANVAS_SOURCE_IMAGE a user would need to add it to a material temporarily to paint on, what is not nice.
Although in this case a weight paint layer might be more useful. Will check with @Simon Thommes (simonthommes) and @Julien Kaspar (JulienKaspar).

source/blender/editors/sculpt_paint/paint_canvas.cc
50 ↗(On Diff #50074)

I added a comment that this is temporarily and added logic that some tools can follow the shading color of the last used tool. This is also temporarily, but visible to the user)

source/blender/editors/space_view3d/space_view3d.c
1386 ↗(On Diff #50074)

It is used to change the visibility of the canvas when switching tools. The workbench needs to receive an view_update trigger what was done via the depsgraph. Changed this to use DRW_notify_view_update directly.

source/blender/makesrna/intern/rna_material.c
1033 ↗(On Diff #50074)

Not sure, I copied it as name could contain an attribute name and assumed the same limitation as uv_layer is required. Will remove it for now.

source/blender/makesrna/intern/rna_sculpt_paint.c
1032

PaintModeSettings and TexturePaintSettings should have the same interface for now as they use the same UI panel.

We could rename TexturePaintSettings.mode to canvas_source, but that leads to an API change. I chose to use mode for now to keep the API clear.

Will fix this in the panel.

1044–1045

Same here. Kept the API to canvas, but will change the panel to support both api's

Philipp Oeser (lichtwerk) added a subscriber: Philipp Oeser (lichtwerk).Apr 5 2022, 3:44 PM
In D14455#390802, @Jeroen Bakker (jbakker) wrote:
In D14455#390489, @Brecht Van Lommel (brecht) wrote:

Personally, I'm not sure if PAINT_CANVAS_SOURCE_IMAGE is something worth supporting at all, if it's actually useful in practice or just a legacy thing from before there was more convenient material integration.

A niche case for PAINT_CANVAS_SOURCE_IMAGE, would be to paint on an image texture that is used in geometry nodes, without PAINT_CANVAS_SOURCE_IMAGE a user would need to add it to a material temporarily to paint on, what is not nice.
Although in this case a weight paint layer might be more useful. Will check with @Simon Thommes (simonthommes) and @Julien Kaspar (JulienKaspar).

This is the about the (former) "Single Image" mode right?
Isnt this also useful to quickly paint/author an image used as Stencil Mask for example?
Or (even if this this is all EOL) particle influence textures?
Probably more even, but having to add an image to a material just to paint on it seems like a step backwards I think, would keep the option personally

Brecht Van Lommel (brecht) added a comment.Apr 5 2022, 8:12 PM

We can keep the single image source. For geometry nodes (including future particles replacement), it would be nice if any image textures used in them would also show up in the paint slots, rather than having to select them separately. But not something I expect to be done in this patch.

Regarding the behavior of the 3D viewport display changing with the active tool, am I understanding correctly this is meant to affect sculpt vertex colors in 3.2? Maybe that's a design choice we need to sign off on in the meeting tomorrow.

The implementation looks ok if we are going with that design, the hacky bits are due to taking into account the active tool.

Jeroen Bakker (jbakker) added a comment.EditedApr 6 2022, 7:45 AM
In D14455#390948, @Brecht Van Lommel (brecht) wrote:

Regarding the behavior of the 3D viewport display changing with the active tool, am I understanding correctly this is meant to affect sculpt vertex colors in 3.2? Maybe that's a design choice we need to sign off on in the meeting tomorrow.

As I understood that sculpt vertex colors in 3.2 would force vertex colors when starting a stroke. This isn't convenience as you don't have the visual feedback when placing the first stroke. This patch forces the vertex colors when activating the tool that allows users to start painting with visual feedback immediatly.
That said in paint mode this should not be an issue as the canvas selector would be visible at all times and workbench would always be forces to use the color mode that would show the canvas. Will add it to the topic of the meeting.

Jeroen Bakker (jbakker) updated this revision to Diff 50143.Apr 6 2022, 8:21 AM
  • Remove unused code.
Harbormaster completed remote builds in B21482: Diff 50143.Apr 6 2022, 8:21 AM
Jeroen Bakker (jbakker) updated this revision to Diff 50144.Apr 6 2022, 10:50 AM
  • Merge remote-tracking branch 'origin/master' into temp-T96709-painting-target
  • Add specific UIList for color attribute selection.
  • Only show material attributes that are supported.
  • Add attribute selection using material source.
  • Remove 'sculpt vertex mode' shading mode switch.
Harbormaster completed remote builds in B21483: Diff 50144.Apr 6 2022, 10:51 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Apr 6 2022, 10:52 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Apr 6 2022, 6:36 PM
Jeroen Bakker (jbakker) updated this revision to Diff 50235.Apr 8 2022, 8:21 AM
  • Merge remote-tracking branch 'origin/master' into temp-T96709-painting-target
  • Show Canvas selector only when experimental flag is enabled.
  • Revert removal of force shading color to vertex colors.
Harbormaster completed remote builds in B21522: Diff 50235.Apr 8 2022, 8:21 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Apr 8 2022, 8:24 AM
Jeroen Bakker (jbakker) added a comment.Apr 8 2022, 8:26 AM

@Brecht Van Lommel (brecht) I updated the patch with what was discussed on Wednesday during the meeting.
I kept the 'hacky' bits for now, otherwise it influences the workflow too much when the experimental mode is active.

Brecht Van Lommel (brecht) accepted this revision.Apr 8 2022, 4:24 PM
This revision is now accepted and ready to land.Apr 8 2022, 4:24 PM
Closed by commit rB8b7cd1ed2a17: Painting: Canvas switcher for painting brushes/tools. (authored by Jeroen Bakker (jbakker)). · Explain WhyApr 8 2022, 4:43 PM
This revision was automatically updated to reflect the committed changes.
Jeroen Bakker (jbakker) added a commit: rB8b7cd1ed2a17: Painting: Canvas switcher for painting brushes/tools..
Omar Emara (OmarSquircleArt) mentioned this in T99868: Regression: Texture slots thumbnails in Texture painting mode are missing.Jul 20 2022, 1:10 PM

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
1 line
48 lines
source/
blender/
blenkernel/
5 lines
draw/
engines/
workbench/
9 lines
editors/
include/
16 lines
sculpt_paint/
1 line
428 lines
makesdna/
26 lines
2 lines
makesrna/
intern/
53 lines
7 lines
80 lines
4 lines

Diff 49767

View Options

release/scripts/startup/bl_ui/space_userpref.py

Loading...
View Options

release/scripts/startup/bl_ui/space_view3d_toolbar.py

Loading...
View Options

source/blender/blenkernel/BKE_paint.h

Loading...
View Options

source/blender/draw/engines/workbench/workbench_engine.c

Loading...
View Options

source/blender/editors/include/ED_paint.h

Loading...
View Options

source/blender/editors/sculpt_paint/CMakeLists.txt

Loading...
View Options

source/blender/editors/sculpt_paint/paint_canvas_material.cc

Loading...
View Options

source/blender/makesdna/DNA_scene_types.h

Loading...
View Options

source/blender/makesdna/DNA_userdef_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_object.c

Loading...
View Options

source/blender/makesrna/intern/rna_scene.c

Loading...
View Options

source/blender/makesrna/intern/rna_sculpt_paint.c

Loading...
View Options

source/blender/makesrna/intern/rna_userdef.c

Loading...