Grease Pencil: Bring back paint mode tools using brush assets #125449
No reviewers
Labels
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 project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#125449
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "JulianEisel/blender:temp-brush-assets-gpencil-followups"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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, seeToolSystemBrushBindings
. It works roughly like this:'DRAW'
type) share the same active brush, which seems desired.ToolSystemBrushBindings
(file read/write, freeing, copying, etc.) are handled within theBKE_paint
module, but otherwise the tool system manages it.@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
WIP: Brush assets followups for Grease Pencilto Grease Pencil: Bring back paint mode tools using brush assets@blender-bot package
Package build started. Download here when ready.
data_block
tool field, add more specific one 727a0e24d5The 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")
, thencls.attr
(a double lookup internally).e.g.
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.
@ -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.@ -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__
?@ -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).
@ -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:
I tried two ideas.
WorkSpaceTool.setup()
-1b834068f7
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
forNONE
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 hasFLAG_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.
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.
@ -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).@ -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:
... then the loops, lambdas's etc aren't so nested and it's easier to follow the high level logic at a glance.
Committed in
a38c96b92c
&a79f9100a6
.Pull request closed