Details

Reviewers

Antonio Vazquez (antoniov)
Pablo Vazquez (pablovazquez)

Group Reviewers

Grease Pencil
User Interface

Commits

rB9a7c4e2d444f: Annotations: Add Stabilization for draw tool

Summary

This patch adds the stabilizer feature of GP to the annotations.

It has a toggle to activate it "Use Stabilizer", and two properties to control the behaviour of the smooth effect (factor and radius).
You can also use shift at start or in the air to temporaly use this feature.

[NEW VIDEO WITH UI CHANGES AND CURSOR]

[OLD]

Diff Detail

Repository: rB Blender

Event Timeline

Juanfran Matheu (jfmatheu) requested review of this revision.May 7 2020, 2:37 PM

Juanfran Matheu (jfmatheu) created this revision.

Quico Noizeux (sembei) awarded a token.May 7 2020, 3:29 PM

Added "Stabilization" label to UI
Cleanup

The code in general is good, but you are saving data that we don't need and it's only required while operator is running.

source/blender/editors/gpencil/annotate_paint.c
2746	Don't use phrases like `Stabilizer is a Stabilizer to smooth` you need something like `Stabilizer is a tool to smooth the....`
source/blender/makesdna/DNA_gpencil_types.h
267 ↗	(On Diff #24482)	These are not properties of the stroke, so never must be here. These flags must be only in the temp data used by the operator.
532 ↗	(On Diff #24482)	Why this? we don't need save the mouse position in a variable? why not use a hidden prop in the operator?

Antonio Vazquez (antoniov) added inline comments.May 7 2020, 3:49 PM

source/blender/editors/gpencil/annotate_draw.c
221 ↗	(On Diff #24482)	The drawing of the cursor must be acallback inside the annotate_paint operator and not in the darw module. The drawing is only while you are using the operator.

Juanfran Matheu (jfmatheu) planned changes to this revision.May 7 2020, 3:53 PM

Juanfran Matheu (jfmatheu) marked 4 inline comments as done.May 8 2020, 9:59 AM

Moved the drawing of cursor from *_draw.c to *_paint.c and handle by a helper callback for drawing it. Some refactor around that, must be much more optimal now.
Removed unnecessary storage and flags.
Rename descripton of "Use Stabilizer" checkbox.

late cleanup

Juanfran Matheu (jfmatheu) updated this revision to Diff 24512.May 8 2020, 10:43 AM

This comment was removed by Juanfran Matheu (jfmatheu).

Juanfran Matheu (jfmatheu) updated this revision to Diff 24513.May 8 2020, 10:47 AM

I get an ASSERT as soon press the Shift key and draw.
The color of cursor must be red (look in grease pencil paint to see how get the color).
Rebase your patch in master to include the arrows...now the patch does not include the arrows, so I cannot test the full panel.

source/blender/editors/gpencil/annotate_paint.c
103	"Flags"
154	Use single * if it's a comment for a group. If you use ** the comment is by variable.

This revision now requires changes to proceed.May 8 2020, 10:50 AM

my fault, the arrow props are in the panel. You don't need rebase.

Forgot to add immUnbindProgram()... :(

Antonio Vazquez (antoniov) retitled this revision from Stabilization for annotation draw tool to Annotations: Add Stabilization for draw tool.May 8 2020, 11:35 AM

Changed color of stabilizer cursor

Fixed some comments

Fix keymap issue.
Exclude smooth when stabilizer is actived...

Fix shift toggle off cursor when using stabilizer on.

• Add feature of toggle off stabilization temporaly when pressing shift even if permanent stabilization toggle (ui) is active.
• Tweak default values.
• Some commenting...

I think the code is ok, pending only of the minor updates in the comments. Update the patch removing that lines and I will approve it. Good job!

source/blender/editors/gpencil/annotate_paint.c
1809	Remove /* XXX */
2302	Remove TODOs

Testing I just detected a small bug. If you start the annotation using D key, the stabilizer is ON by default and must be OFF.

Pablo Vazquez (pablovazquez) added a reviewer: User Interface.May 8 2020, 6:39 PM

stefano severi (Tetone) awarded a token.May 8 2020, 6:46 PM

In terms of UI, for consistency and time saving: every time you add a feature that it's already elsewhere in Blender (this feature already exists for Grease Pencil objects), please try to copy whatever is there, no need to reinvent the wheel.

For example, make the UI use the same names and layout as Grease Pencil:

Subpanel with a checkbox Stabilize Stroke.
Radius listed first, subtype is pixels.
Smooth Factor rename to Factor.

Pablo Vazquez (pablovazquez) requested changes to this revision.May 8 2020, 6:47 PM

This revision now requires changes to proceed.May 8 2020, 6:47 PM

@Pablo Vazquez (pablovazquez) We tried to mimic the GPencil subpanel, but there is a limitation in the UI to put operator properties in subpanels. The stabilizer of GPencil is a property of the brush, but annotations don't use brushes, so the properties are dynamic properties of the operator and this is why we cannot use subpanels.

Yes, true, there's no way to add subpanels to tool settings right now. That should be fine though:

layout.separator()
layout.prop(props, "use_stabilizer")
col = layout.column(align=False)
col.active = props.use_stabilizer
col.prop(props, "stabilizer_radius", text="Radius", slider=True)
col.prop(props, "stabilizer_factor", text="Factor", slider=True)

source/blender/editors/gpencil/annotate_paint.c
2846	"Use" is implied for boolean options. The name and tooltip should describe what happens when the setting is on. So the property should be called "Stabilize Stroke" like @Pablo Vazquez (pablovazquez) suggested. I would also suggest not repeating the name of the property in the tooltip, that's redundant.