Grease Pencil: Bring back paint mode tools using brush assets #125449

Closed
Julian Eisel wants to merge 72 commits from JulianEisel/blender:temp-brush-assets-gpencil-followups into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Implements the design from #126032, making use of it for grease pencil draw mode.

Note that we also plan to bring back some tools for specific brush types in other modes, based on feedback.


This brings back a similar & simplified version of the PaintToolSlot design, see ToolSystemBrushBindings. It works roughly like this:

  • A "main" brush reference is stored for any tool that isn't limited to a specific brush type, such as the general "Brush" tool.
  • For tools that use a specific brush type, a reference to the last used brush is stored, identified by the tool type. This means tools using the same brush type (such as the grease pencil primitives that all use the 'DRAW' type) share the same active brush, which seems desired.
  • Basic storage operations for ToolSystemBrushBindings (file read/write, freeing, copying, etc.) are handled within the BKE_paint module, but otherwise the tool system manages it.
  • Instead of updating the "last used brush" storage from within the paint API code, this is now done with a tool system API call from operators that change the active brush. This way the whole logic stays as part of the tool system, and isn't mixed with paint brush management code. It's possible to do this now thanks to the unified brush asset handling and its unified operators.
Implements the design from #126032, making use of it for grease pencil draw mode. Note that we also plan to bring back some tools for specific brush types in other modes, based on feedback. --- This brings back a similar & simplified version of the `PaintToolSlot` design, see `ToolSystemBrushBindings`. It works roughly like this: - A "main" brush reference is stored for any tool that isn't limited to a specific brush type, such as the general "Brush" tool. - For tools that use a specific brush type, a reference to the last used brush is stored, identified by the tool type. This means tools using the same brush type (such as the grease pencil primitives that all use the `'DRAW'` type) share the same active brush, which seems desired. - Basic storage operations for `ToolSystemBrushBindings` (file read/write, freeing, copying, etc.) are handled within the `BKE_paint` module, but otherwise the tool system manages it. - Instead of updating the "last used brush" storage from within the paint API code, this is now done with a tool system API call from operators that change the active brush. This way the whole logic stays as part of the tool system, and isn't mixed with paint brush management code. It's possible to do this now thanks to the unified brush asset handling and its unified operators.
Julian Eisel added 3 commits 2024-07-25 19:47:32 +02:00
This way we can query the brush types, e.g. to filter out eraser brushes
in the brush asset selector while the eraser tool is selected.

Technically a brush may have different brush types depending on the
mode. So we store a type for all supported modes.

Ideally the name of the custom metadata property would match the name of
the properties in RNA. However these should be renamed to avoid the term
"tool", see #124201, and we currently don't do versioning of asset
metadata. So this uses what I expect the name will be. Could create a
separate PR to duplicate the RNA properties with the new name.
Allows selecting any grease pencil brush asset for drawing with the primitive
tools (Curve, Box, Line, etc) active. Brushes can be selected from the asset
shelf or the brush selector just like with the "Brush" tool.
Julian Eisel added 3 commits 2024-07-26 18:40:36 +02:00
Non functional yet, just displaying it in the toolbar.
Very hacky, just to see how this can work in principle. Changing to the
eraser or fill tool now activates the corresponding default brush.
Julian Eisel added 1 commit 2024-07-31 16:58:19 +02:00
Julian Eisel added 1 commit 2024-07-31 17:47:57 +02:00
There's an option now to flag tools as using brushes. Previously this was done
by setting the `data_block` field for tools, but this was weird naming, and was
causing issues with the logic to switch brush assets when switching tools.

Dynamically setting `data_block` based on the current brush, as we'd do with
brush assets now, means that we couldn't differentiate anymore between tools
that supported any brush, or only brushes of a certain type. The `data_block`
field is still there, but now is used only to indicate that a tool requires a
certain brush type. I can rename this later still.

This way we know now if a tool requires a certain brush type or supports any
brush type, and can set the active brush accordingly when switching tools.
Julian Eisel added 1 commit 2024-08-01 15:18:13 +02:00
Julian Eisel added 1 commit 2024-08-01 15:27:46 +02:00
Julian Eisel added 1 commit 2024-08-01 15:44:01 +02:00
Julian Eisel added 1 commit 2024-08-01 17:30:46 +02:00
Julian Eisel added 2 commits 2024-08-05 16:58:05 +02:00
Julian Eisel added 1 commit 2024-08-06 19:32:19 +02:00
In the asset shelf popups, only show eraser brushes when the eraser tool
is activated, for example. The permanent asset shelf region still shows
all brushes, for convenience.
Julian Eisel added this to the Brush Assets & Asset Shelf project 2024-08-06 19:36:56 +02:00
Julian Eisel added 1 commit 2024-08-07 10:46:38 +02:00
Julian Eisel added 1 commit 2024-08-08 12:36:16 +02:00
Was a bit confusing to have these tools so close together, even though
the cutter is the only one in that cluster that doesn't use brushes.
Julian Eisel added 6 commits 2024-08-08 13:00:42 +02:00
Julian Eisel added 1 commit 2024-08-08 15:50:36 +02:00
Julian Eisel added 1 commit 2024-08-08 15:59:54 +02:00
Julian Eisel added 1 commit 2024-08-12 12:43:08 +02:00
Julian Eisel added 1 commit 2024-08-23 14:57:01 +02:00
Julian Eisel added 1 commit 2024-08-23 16:09:02 +02:00
Julian Eisel added 1 commit 2024-08-23 18:31:01 +02:00
Store a "main brush" separate from the active brush. The toolsystem
manages this like it manages the tool-slots.

Code needs cleaning up, this is just a first working version.
Julian Eisel added 1 commit 2024-08-23 18:38:37 +02:00
Julian Eisel added 1 commit 2024-08-23 18:41:56 +02:00
Julian Eisel added 1 commit 2024-08-26 12:50:55 +02:00
Merge branch 'main' into temp-brush-assets-gpencil-followups
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c67a83e419
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR125449) when ready.
Julian Eisel added 2 commits 2024-08-29 12:03:45 +02:00
Julian Eisel added 1 commit 2024-08-29 12:42:34 +02:00
Fix test failure when copying paint data
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
43e92a041d
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR125449) when ready.
Julian Eisel added 1 commit 2024-08-30 11:28:46 +02:00
Julian Eisel added 1 commit 2024-08-30 17:08:46 +02:00
Julian Eisel added 1 commit 2024-08-30 17:30:45 +02:00
Julian Eisel added 1 commit 2024-09-02 12:47:54 +02:00
Julian Eisel added 3 commits 2024-09-03 15:38:12 +02:00
Julian Eisel added 1 commit 2024-09-03 15:47:39 +02:00
Julian Eisel added 1 commit 2024-09-03 16:06:50 +02:00
Julian Eisel added 1 commit 2024-09-03 17:23:21 +02:00
Otherwise I find this makes the `Paint` struct confusing. Seperate the
related fields into an own struct that gives them a more clear purpose.
Julian Eisel added 1 commit 2024-09-03 17:26:04 +02:00
Julian Eisel added 1 commit 2024-09-04 12:58:38 +02:00
Julian Eisel added 1 commit 2024-09-05 17:12:57 +02:00
Avoid requiring a paint API function to set the main brush, this should
be handled by the tool system like the other brush bindings. Also remove
unnecessary main brush pointer, the asset reference is enough.

Added a version of `BKE_paint_brush_set()` that takes the brush asset
reference and imports if necessary, so all brush importing logic remains
inside the paint API.
Julian Eisel added 1 commit 2024-09-05 17:23:40 +02:00
Julian Eisel added 1 commit 2024-09-05 17:39:53 +02:00
Cleanup: Remove unnecessary changes
All checks were successful
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
eb9f8d0ca6
Julian Eisel changed title from WIP: Brush assets followups for Grease Pencil to Grease Pencil: Bring back paint mode tools using brush assets 2024-09-05 18:37:45 +02:00
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR125449) when ready.
Julian Eisel added 1 commit 2024-09-05 18:58:39 +02:00
Julian Eisel added 1 commit 2024-09-06 14:30:22 +02:00
Julian Eisel added 2 commits 2024-09-06 15:45:36 +02:00
Julian Eisel added 1 commit 2024-09-06 15:50:36 +02:00
Julian Eisel added 1 commit 2024-09-06 16:04:41 +02:00
Julian Eisel added 1 commit 2024-09-06 16:13:34 +02:00
Julian Eisel requested review from Campbell Barton 2024-09-06 16:44:48 +02:00
Julian Eisel requested review from Hans Goudey 2024-09-06 16:44:59 +02:00
Campbell Barton approved these changes 2024-09-11 09:30:13 +02:00
Campbell Barton left a comment
Owner

The tool-system side looks good (besides some minor issues noted inline & in !127204). I didn't check the asset changed in detail.

I've made a suggestion inline regarding the brush-type DNA, although this may not be practical.

Accepting as I don't think an extra iteration is needed from me.

The tool-system side looks good (besides some minor issues noted inline & in !127204). I didn't check the asset changed in detail. I've made a suggestion inline regarding the brush-type DNA, although this may not be practical. Accepting as I don't think an extra iteration is needed from me.
@ -18,0 +22,4 @@
if not tool or tool.brush_type == "":
return True
if not hasattr(cls, "brush_type_prop") or not hasattr(cls, "tool_prop"):

Note that this isn't a big issue, noticed while checking the code.


There is less overhead in calling getattr(cls, "attr", None).
Compared with hasattr(cls, "attr"), then cls.attr (a double lookup internally).

e.g.

if (cls_tool_prop := getattr(cls, "tool_prop")) is None:
    return
...

Even in this case it's a little awkward accessing values that might exist.

Applies to the existing mode_prop too.


Alternatively:

Would there be any issues declaring None values fortool_prop & similar attributes in the base class, then getattr/hasattr checks can be avoided.

This has the benefit that typos aren't hidden when invalid are passed to hasattr/getattr.

*Note that this isn't a big issue, noticed while checking the code.* ---- There is less overhead in calling `getattr(cls, "attr", None)`. Compared with `hasattr(cls, "attr")`, then `cls.attr` (a double lookup internally). e.g. ``` if (cls_tool_prop := getattr(cls, "tool_prop")) is None: return ... ``` Even in this case it's a little awkward accessing values that *might* exist. Applies to the existing `mode_prop` too. ---- **Alternatively:** Would there be any issues declaring `None` values for`tool_prop` & similar attributes in the base class, then getattr/hasattr checks can be avoided. This has the benefit that typos aren't hidden when invalid are passed to hasattr/getattr.
JulianEisel marked this conversation as resolved
@ -21,3 +40,2 @@
return False
if hasattr(cls, "mode_prop"):
return asset.metadata.get(cls.mode_prop, False)
if hasattr(cls, "mode_prop") and not asset.metadata.get(cls.mode_prop, False):

See previous comment WRT hasattr(..) checks.

See previous comment WRT `hasattr(..)` checks.
JulianEisel marked this conversation as resolved
@ -1462,0 +1613,4 @@
{
brush_ref->name = BLI_strdup(brush_ref->name);
brush_ref->brush_asset_reference = MEM_new<AssetWeakReference>(
"active_brush_per_brush_type copy", *brush_ref->brush_asset_reference);

detail just use __func__ ?

*detail* just use `__func__` ?
JulianEisel marked this conversation as resolved
@ -40,6 +41,7 @@ typedef struct bToolRef_Runtime {
/** One of these 3 must be defined. */

This doc-string needs to be updated. (One of the 4 items needs to be set after this PR I suppose).

This doc-string needs to be updated. (One of the 4 items needs to be set after this PR I suppose).
JulianEisel marked this conversation as resolved
@ -40,6 +41,7 @@ typedef struct bToolRef_Runtime {
/** One of these 3 must be defined. */
char keymap[64];
char gizmo_group[64];
char brush_type[64];

Suggestion

Could this be stored as an integer? If not it would be good to note why it's important to store the enum as a string.

It's hard to say without trying to refactor the value into an int, but from what I can see it might be possible and avoids awkward enum/int conversion in the C++ code as the value would be compatible with values returned by BKE_paint_get_brush_type_from_paintmode.

Python can then use the paint mode + value to return the proper enum value (which would be a string).


If this causes more problems than it solves then it'd be good to have a code comment here:

/* The type of brush, used when the  `TOOLREF_FLAG_USE_BRUSHES` flag is ... snip ...
 *
 * \note The brush type must match an RNA enum value from .... even though this is an int internally,
 * it's important to store this as a string because ... reasons :) ... etc. */
*Suggestion* Could this be stored as an integer? If not it would be good to note why it's important to store the enum as a string. It's hard to say without trying to refactor the value into an int, but from what I can see it might be possible and avoids awkward enum/int conversion in the C++ code as the value would be compatible with values returned by `BKE_paint_get_brush_type_from_paintmode`. Python can then use the paint mode + value to return the proper enum value (which would be a string). ---- If this causes more problems than it solves then it'd be good to have a code comment here: ``` /* The type of brush, used when the `TOOLREF_FLAG_USE_BRUSHES` flag is ... snip ... * * \note The brush type must match an RNA enum value from .... even though this is an int internally, * it's important to store this as a string because ... reasons :) ... etc. */ ```
Author
Member

I tried two ideas.

  1. Keep brush type stored as string, but validate it in WorkSpaceTool.setup() - 1b834068f7
  2. What you suggested - 7cb800dea8
    One complication is that there needs to be some unset value. 0 doesn't work because some of the brush type enums use that. So I used -1 ('NONE' in BPY).

Don't have a strong opinion as to which way to go.

I tried two ideas. 1) Keep brush type stored as string, but validate it in `WorkSpaceTool.setup()` - 1b834068f7 2) What you suggested - 7cb800dea8 One complication is that there needs to be some unset value. 0 doesn't work because some of the brush type enums use that. So I used `-1` (`'NONE'` in BPY). Don't have a strong opinion as to which way to go.

Checked, the patch and prefer the int version... some comments.

  • The -1 for NONE is a bit awkward but not terrible as far as I can see although I might be missing something, why we would want a brush that has FLAG_USE_BRUSHES enabled AND doesn't point to a brush type. Couldn't it just be set to the first/default brush? (worth mentioning why none is needed in comments).

  • In general I've found having to use RNA enum look-ups in this context to get unnecessarily verbose - when comparing int values would be a simple check.

  • Validating requires checking all possible strings instead of a range check.

  • Using an int is it simplifies versioning, if we need to map old/new values.

Checked, the patch and prefer the int version... some comments. - The `-1 ` for `NONE` is a bit awkward but not terrible as far as I can see although I might be missing something, why we would want a brush that has `FLAG_USE_BRUSHES` enabled AND doesn't point to a brush type. Couldn't it just be set to the first/default brush? (worth mentioning why none is needed in comments). - In general I've found having to use RNA enum look-ups in this context to get unnecessarily verbose - when comparing int values would be a simple check. - Validating requires checking all possible strings instead of a range check. - Using an int is it simplifies versioning, if we need to map old/new values.
Author
Member

The -1 for NONE is a bit awkward but not terrible as far as I can see although I might be missing something, why we would want a brush that has FLAG_USE_BRUSHES enabled AND doesn't point to a brush type. Couldn't it just be set to the first/default brush? (worth mentioning why none is needed in comments).

Some tools may support any brush type, the brush_type is just to limit a tool to a specific type. The design of having one tool for each brush type is largely gone with brush assets, even if we bring back some tools for some specific types.


For me the main benefit of storing brush types as names is that they encode more context, and are therefore safer. Every sculpt/paint mode has its own brush type, if we store that in the same int we loose type safety: the value 4 may be valid in multiple modes, however 'INFLATE' is only a valid brush type name in sculpt mode. This might be even more relevant for brushes that support multiple modes.

But anyway, are you suggesting additional changes still, or is the current PR version still fine (plus comments on -1/NONE)?

> The -1 for NONE is a bit awkward but not terrible as far as I can see although I might be missing something, why we would want a brush that has FLAG_USE_BRUSHES enabled AND doesn't point to a brush type. Couldn't it just be set to the first/default brush? (worth mentioning why none is needed in comments). Some tools may support any brush type, the `brush_type` is just to limit a tool to a specific type. The design of having one tool for each brush type is largely gone with brush assets, even if we bring back some tools for some specific types. --- For me the main benefit of storing brush types as names is that they encode more context, and are therefore safer. Every sculpt/paint mode has its own brush type, if we store that in the same int we loose type safety: the value 4 may be valid in multiple modes, however `'INFLATE'` is only a valid brush type name in sculpt mode. This might be even more relevant for brushes that support multiple modes. But anyway, are you suggesting additional changes still, or is the current PR version still fine (plus comments on -1/`NONE`)?

Regarding type safety while I see what you mean - if strings from different brushes are being mixed with enums - we're going to have problems, it might work by accident in some cases (if names are shared), but AFAICS we never want that to happen so if it does, it's more of a bug to solve... than a problem which is avoided by using either strings/ints.

No additional review needed, just comments on rationale for -1/ NONE.

Regarding type safety while I see what you mean - if strings from different brushes are being mixed with enums - we're going to have problems, it might work by accident in some cases (if names are shared), but AFAICS we never want that to happen so if it does, it's more of a bug to solve... than a problem which is avoided by using either strings/ints. No additional review needed, just comments on rationale for -1/ NONE.
JulianEisel marked this conversation as resolved
@ -138,0 +231,4 @@
{
MEM_delete(existing_brush_ref->brush_asset_reference);
existing_brush_ref->brush_asset_reference = MEM_new<AssetWeakReference>(
"NamedBrushAssetRefernce update asset ref", *paint->brush_asset_reference);

detail just use __func__ ? (same for other allocations here).

*detail* just use `__func__` ? (same for other allocations here).
JulianEisel marked this conversation as resolved
@ -138,0 +284,4 @@
/**
* Activate a brush compatible with \a tref, call when the active tool changes.
*/
static void activate_compatible_brush_from_toolref(const bContext *C,

This function is getting a bit involved and would probably be due for a refactor if this is ever used for another space (image space for e.g.).

Suggest to split out into something like this:

toolsystem_brush_activate_from_toolref()
{
  if (tref->space_type == SPACE_VIEW3D) {
    if (tref->mode == CTX_MODE_PARTICLE) {
      toolsystem_brush_activate_from_toolref_for_object_particle(..);
    }
    else {
      toolsystem_brush_activate_from_toolref_for_object_paint(..);
    }
  } 
}

... then the loops, lambdas's etc aren't so nested and it's easier to follow the high level logic at a glance.

This function is getting a bit involved and would probably be due for a refactor if this is ever used for another space (image space for e.g.). Suggest to split out into something like this: ``` toolsystem_brush_activate_from_toolref() { if (tref->space_type == SPACE_VIEW3D) { if (tref->mode == CTX_MODE_PARTICLE) { toolsystem_brush_activate_from_toolref_for_object_particle(..); } else { toolsystem_brush_activate_from_toolref_for_object_paint(..); } } } ``` ... then the loops, lambdas's etc aren't so nested and it's easier to follow the high level logic at a glance.
JulianEisel marked this conversation as resolved
Julian Eisel added 2 commits 2024-09-16 14:16:03 +02:00
Julian Eisel added 1 commit 2024-09-16 14:23:55 +02:00
Julian Eisel added 1 commit 2024-09-16 15:33:02 +02:00
Julian Eisel added 2 commits 2024-09-17 12:25:50 +02:00
Julian Eisel added 1 commit 2024-09-17 14:50:15 +02:00
Julian Eisel added 1 commit 2024-09-17 16:01:22 +02:00
Julian Eisel added 2 commits 2024-09-17 16:32:21 +02:00
Julian Eisel added 1 commit 2024-09-17 18:31:04 +02:00
Julian Eisel added 1 commit 2024-09-18 16:01:44 +02:00
Julian Eisel added 1 commit 2024-09-19 11:57:42 +02:00
Julian Eisel added 1 commit 2024-09-20 16:14:40 +02:00
Julian Eisel added 1 commit 2024-09-20 17:48:32 +02:00
Author
Member

Committed in a38c96b92c & a79f9100a6.

Committed in a38c96b92c & a79f9100a6.
Julian Eisel closed this pull request 2024-09-20 18:17:39 +02:00

Pull request closed

Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 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#125449
No description provided.