GPv3: Set caps operator #113978
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#113978
Loading…
Reference in New Issue
No description provided.
Delete Branch "antoniov/blender:GPv3_capsmode"
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 operator is similar to V2 GPENCIL_OT_caps_set to set the type of cap used by the stroke curve.
Related to #113977
It works well, but the menu name should be "Toggle Caps" and the Both submenu is in uppercase ( BOTH) when it should be "Both"
@mendio
Both
changed.Thanks for working on this :) Added some comments.
@ -951,0 +955,4 @@
enum class CapsMode : int8_t {
/* Toggle both ends. */
TOGGLE,
I think it would be better to call this
BOTH
.START
andAND
also perform a toggle of the cap type.@ -951,0 +1001,4 @@
selected_curves.foreach_index([&](const int curve_index) {
if (mode == CapsMode::TOGGLE || mode == CapsMode::START) {
start_caps.span[curve_index] += 1;
We generally don't do arithmetics on enum types.
I think it would be better to just do something like:
Also notice that the enum is a different one for gpv3. It's
GreasePencilStrokeCapType
.I used this code because the old code was ready to have more caps modes, but now it's better use if. Changed.
@ -951,0 +1012,4 @@
end_caps.span[curve_index] = GP_STROKE_CAP_ROUND;
}
}
if (mode == CapsMode::DEFAULT) {
This case doesn't need to be in this loop. We can add an if case above and put the
selected_curves.foreach_index
loop in theelse
:Actually, for
CapsMode::DEFAULT
I think we can just remove the attribute(s). It's nice to do that where possible to reduce the memory usage later on.Could "Default" be renamed to "Round"? Generally we try to avoid calling things just "default" in the UI since it's not so descriptive.
@ -951,0 +997,4 @@
attributes.lookup_or_add_for_write_span<int8_t>("end_cap", ATTR_DOMAIN_CURVE);
IndexMaskMemory memory;
IndexMask selected_curves = ed::curves::retrieve_selected_curves(curves, memory);
The
has_anything_selected
andpoints_num() == 0
checks are redundant with this index mask. Could be simplified by moving this above the access of the attributes, and return early ifselected_curves.is_empty()
. That can replace thehas_anything_selected
check.Also
IndexMask selected_curves
->const IndexMask selected_curves
Although I agree with your point we already called it
Default
in the old operator:So if we were to change it it would break the API and must be documented in the GP2 -> GP3 API migration document (which may or may not exist). I personally would avoid any change and just stick to the GP2 behaviour for simplicity sake. Not my call though.
About change
Default
... I think would be better change toRounded
too. We are breaking the API a lot, so this change is only one more.@ -960,0 +1009,4 @@
index_mask::masked_fill(end_caps.span, int8_t(GP_STROKE_CAP_TYPE_ROUND), selected_curves);
}
else {
selected_curves.foreach_index([&](const int curve_index) {
The logic inside the lambda could be extracted to a separate function with
const IndexMask &selection
andMutableSpan<int8_t> caps
arguments, and called once for start and end capsI really don't know what you mean. The lines of your code have changed since you made the comment. Could you explain more about the change?
A function
toggle_caps(const IndexMask &selection, MutableSpan<int8_t> caps)
can process one array at a time.Since we changed the
Default
operator option toRounded
, and the menu is calledSet Caps
, I would suggest also adding theFlat
option and renaming the others like the mockup below. IMO this change makes the options clearer and easier to use.Rounded
(Sets both stroke caps rounded)Flat
(Sets both stroke caps flat)Toggle start
(Inverts start stroke cap)Toggle end
(Inverts end stroke cap)Note: With this change, there is no need to keep
Both
optionNow the menu looks like this:
how about adding more caps ? like those : sword and its sheath
@hamza-el-barmaki right now we have to focus on port the same features that we have in Grease Pencil v2, when we finish with that we could revisit everything making updates and adding new features.
@filedescriptor I guess we can merge this too, no?
I left a comment about the structure of the main loop that should still be resolved.
@HooglyBoogly is this ok now?