WIP: Sculpt: Repeat last stroke for sculpt #113270
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
EEVEE & Viewport
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
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
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
4 Participants
Notifications
Due Date
No due date set.
Depends on
#114344 Skip hidden properties in keymap editor
blender/blender
#114380 Core: Repeat Last Clone
blender/blender
Reference: blender/blender#113270
Loading…
Reference in New Issue
No description provided.
Delete Branch "JosephEagar/blender:temp-sculpt-repeat-last-stroke"
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?
This PR adds support for repeating the last sculpt stroke, along with a new "repeat last relative"
operator bound to
alt-shift-R
that repeats the last stroke at the current brush locationusing a tangent frame derived from the surface normal and the camera (sculpt mode only).
Dev Notes:
paint_brush_stroke_add_step
no longer deletes the stroke RNAdata if the paint operator has flag
OPTYPE_REGISTER
set.WM_operator_repeat_clone
that repeatswith a cloned operator (it just calls
wm_operator_invoke
).@blender-bot package
Package build started. Download here when ready.
I tested it for a bit. Overall is a great addition and works. I have some notes:
Repeat Last
is generally way to strongCurrently the repeated strokes never accumulate. Is this expected? Should this depend on the(This is expected for now)Accumulate
brush setting or should repeated strokes always be treated as a brand new stroke?Repeat Last (Relative)
has a hard time following surface curvature correctly. To fix this it could be applied in screen space instead and the stroke is conforming the the new curvature underneath.Repeat Last (Relative)
operator should be added to a menu. We could add it to the Edit menu in the top bar right underRepeat Last
and grey it out anywhere except Sculpt Mode (unless it gets support for other modes in some way).Right. It shouldn't do that, that was part of the reason why I made it clone the operator instead of using the old one in-place.
I'll take a look.
This is expected, yes. Making it accumulate would require special per-brush logic and it's own operator, but it is possible.
I'll try it out.
Will do.
There are some review points. It might also worth adding someone who is more familiar with the WM and operator invocation. That part seems a bit strange to me, but maybe it is just what it is.
@ -5630,6 +5630,40 @@ bool SCULPT_handles_colors_report(SculptSession *ss, ReportList *reports)
return false;
}
void SCULPT_create_repeat_frame(Object *ob, float mat[3][3], float viewinv[4][4], float normal[3])
In the new code we should use the bli::math type of primitives, something like:
float3x3 SCULPT_create_repeat_frame(const Object *object, const float4x4 &view_inv, const float3 %normal)
@ -5632,1 +5632,4 @@
void SCULPT_create_repeat_frame(Object *ob, float mat[3][3], float viewinv[4][4], float normal[3])
{
float viewTan[3] = {1.0f, 0.0f, 0.0f};
Naming: snake_style for variables.
@ -5633,0 +5634,4 @@
{
float viewTan[3] = {1.0f, 0.0f, 0.0f};
invert_m4_m4(ob->world_to_object, ob->object_to_world);
The new code should no overwrite the
world_to_object
. It is done as part of the scene evaluation.@ -5954,3 +5992,3 @@
/* Flags (sculpt does own undo? (ton)). */
ot->flag = OPTYPE_BLOCKING;
ot->flag = OPTYPE_BLOCKING | OPTYPE_REGISTER;
Doing so makes it so this operator becomes available in the "Repeat history". Not sure if that's an intended change, but it seems big enough to be mentioned in the PR description.
This is the brush stroke operator.
it is hard to argue it is a brush stroke operator, but that wasn't a point. Unless there is some other not-so-obvious connection between Repeat History, presentation, and the fact that it is a brush stroke operator.
It won't work without OPTYPE_REGISTER, operators without it aren't inserted into the recent operator list at all.
@ -5959,3 +5997,4 @@
paint_stroke_operator_properties(ot);
RNA_def_float_array(ot->srna, "repeat_tangent_frame", 9, 0, -1, 1, "", "", 0, 0);
This should have
PROP_HIDDEN
, and likelyPROP_SKIP_SAVE
as well. Not sure if that's enough to make the property hidden in the keymap editor. but it definitely should not be exposed there.@ -1362,0 +1387,4 @@
return false;
}
wmWindowManager *wm = CTX_wm_manager(C);
return !BLI_listbase_is_empty(&wm->operators);
repeat_last_get_op
returned true here, so the operator list is not empty. Why is this extra check?You're right.
@ -1362,0 +1448,4 @@
RNA_END;
WM_operator_free_all_after(wm, lastop);
WM_operator_repeat_clone(C, lastop);
Is my understanding correct that this code first overrides an existing operator and then re-executes it under the new undo stack entry?
This seems fragile, as the stroke location seem to run out of sync from the tangent frame, and will also make the "Repeat History" not work as expected.
Oh, I should've replied to this first. No, it clones the operator.
But before the operator is cloned the code above modifies
stroke
property of thelastop
. So intuitively clone happens after modification.I changed it to clone the operator properties. We probably should have the wm changes reviewed by someone, any idea who?
Julian, Campbell, or Brecht would be my guess based on the module page.
@ -1362,0 +1450,4 @@
WM_operator_free_all_after(wm, lastop);
WM_operator_repeat_clone(C, lastop);
return OPERATOR_CANCELLED;
OPERATOR_FINISHED
?The operator itself doesn't do anything, it just executes another one.
SCREEN_OT_repeat_last
does the same thing.@ -746,3 +746,3 @@
int WM_operator_repeat(bContext *C, wmOperator *op);
int WM_operator_repeat_last(bContext *C, wmOperator *op);
/**
int WM_operator_repeat_clone(bContext *C, wmOperator *op);
I am confused by the naming. What does the "clone" refer to?
Instead of re-using the existing operator in the operator stack, it clones a new one. That way repeat last relative isn't changing RNA properties in the original operator. I could remove it if it's okay to change properties in the original operator.
Maybe call it
WM_operator_clone_and_repeat
?And add the description from the in-lined PR comment to the public header as it helps understanding what exactly the function is for.
@ -748,1 +748,3 @@
/**
int WM_operator_repeat_clone(bContext *C, wmOperator *op);
/**
Unrelated change. Run clang-format, should get rid of it.
@ -1649,6 +1653,7 @@ int paint_stroke_exec(bContext *C, wmOperator *op, PaintStroke *stroke)
if (stroke->stroke_started) {
RNA_BEGIN (op->ptr, itemptr, "stroke") {
stroke->ups->overlap_factor = RNA_float_get(&itemptr, "overlap_factor");
Isn't the overlap factor calculated once at the beginning of a stroke and used for all brush steps?
It's not , it's calculate per dab.
It's not , it's calculated per dab.
@ -5633,0 +5641,4 @@
float3 view_tan = {1.0f, 0.0f, 0.0f};
bool success;
float4x4 world_to_object = invert(float4x4(ob->object_to_world), success);
Why not
float4x4 world_to_object = ob->world_to_object
?@ -5960,2 +6000,4 @@
paint_stroke_operator_properties(ot);
PropertyRNA *prop = RNA_def_float_array(ot->srna, "repeat_tangent_frame", 9, 0, -1, 1, "", "", 0, 0);
RNA_def_property_flag(prop, PROP_SKIP_SAVE|PROP_HIDDEN);
Clang-format.
Also, did you check on your end the property is hidden in the keymap editor? I was testing the patch and the property was still exposed there.
It is, yes.
@ -3939,1 +3939,4 @@
prop = RNA_def_property(srna, "overlap_factor", PROP_FLOAT, PROP_NONE);
RNA_def_property_flag(prop, PROP_IDPROPERTY);
RNA_def_property_ui_text(prop, "Factor used to attenuate strength from spacing.", "");
Seems like property description is used as property name. Also, don't think description is to include full-stop.
On a user level it does seem very interesting and cool feature.
The implication of adding tangent matrix as an operator property is that it is visible in the keymap editor. Even after it was marked as skip and hidden. Is the keymap editor just ignores the hidden flag? In any case, I do not think we should be exposing such internal properties anywhere in the UI.
Not sure how useful it is to have this operator in the menu. Having it there will make the operator run relative to the mouse position when the menu item was clicked?
There are also some inlined questions. Maybe it will also worth adding a reviewer for the WM part, as some things which were more internal are more public now.
@Sergey The menu operator should use the
OPTYPE_DEPENDS_ON_CURSOR
flag like many others. Not that useful to access the operator that way but great for discoverability.@JulienKaspar Oh, that's interesting!
Looks like it can't be added to the Edit menu since it expects a viewport context. I added it to the top of the Sculpt menu instead.
It's not in the keymap editor anymore
@blender-bot package
Package build started. Download here when ready.
One more issue I noticed with Repeat Last:
When using an operation that opens the redo panel and then doing a stroke, the redo panel doesn't disappear. If any settings in the redo panel are changed, the stroke that came after and any repeated strokes will disappear.
Example:
A
-> Smooth MaskShift R
a few timesApart from this the repeat last operator is working exactly like expected. Imo that could go into main anytime, without the Repeat Last (Relative).
It still appears to be there
Ok I added a PR fixing the keymap editor so it doesn't display hidden properties, #114344
Ok, I've split the WM part off into #114380
I tested it again to see where it's at. Some new notes:
My previous comment is also not yet addressed.
I also thought the Repeat Last (Relative) operator should be split from this PR for now until it works like expected.
Sculpt: Repeat last stroke for sculptto WIP: Sculpt: Repeat last stroke for sculptMarking as Work-in-Progress, as the feedback needs to be incorporated, and it helps to introduce some clarity over the state of the PR in the workboard.
Checkout
From your project repository, check out a new branch and test the changes.