This option would be useful for primitives. With the patch it is only possible to access the new brush property using the Draw tool, maybe this property should be outside any particular brush menu

Antonio Vazquez (antoniov) added a reviewer: Pablo Vazquez (pablovazquez).Jul 22 2021, 5:40 PM

Antonio, feel free to assign me as a reviewer for patches like this in the future. Pablo mentioned this to me and there are a quite a few pretty straightforward tweaks to make here.

source/blender/makesdna/DNA_brush_enums.h
158 ↗	(On Diff #39813)	I think this should reuse the existing `eGPDstroke_Caps` enum, I don't see a reason to add a new one. This adds a new place to maintain when adding types, more defines to find when reading code, etc.
source/blender/makesdna/DNA_brush_types.h
79–81	Rather than listing the options here, mention the enum type: `#eGPDstroke_Caps`
80	You can use `int8_t` here, and keep one char of padding.
source/blender/makesrna/intern/rna_brush.c
272	This enum can be defined directly in the function that uses it, like `gppaint_mode_types_items`
273	I'd suggest "Round" instead of "Rounded" here, it's just a bit simpler, and more similar to "Flat"
1762	In most cases only the first letter of the UI description should be capitalized. Additionally, the description is not useful here. We have to take care to imagine the perspective of someone who hadn't seen this before. I suggest something like: "The shape of the start and end of the stroke"
1763	This property is missing `RNA_def_property_update`

This revision now requires changes to proceed.Jul 22 2021, 6:19 PM

GPencil: Changes after first review

Harbormaster completed remote builds in B15965: Diff 39825.Jul 22 2021, 6:54 PM

Antonio Vazquez (antoniov) added inline comments.Jul 22 2021, 6:55 PM

source/blender/makesrna/intern/rna_brush.c
1763	Don't need update...it's only for new strokes

Antonio Vazquez (antoniov) edited the summary of this revision. (Show Details)Jul 22 2021, 7:00 PM

Nice Antonio, functionality is much better now.

We have to look for a better UI solution in Active tool panel in properties editor. IMO in that panel a dropdown would work better than the icon toggle.

GPencil: Expand Enum only for tool header

Harbormaster completed remote builds in B15967: Diff 39832.Jul 22 2021, 10:19 PM

Antonio Vazquez (antoniov) edited the summary of this revision. (Show Details)Jul 22 2021, 10:19 PM

Antonio Vazquez (antoniov) added a subscriber: Falk David (filedescriptor).

Rebase

Harbormaster completed remote builds in B15982: Diff 39855.Jul 23 2021, 11:01 AM

Matias Mendiola (mendio) accepted this revision.Jul 24 2021, 12:39 AM

I don't think this should be committed to master with those icons.
With the option presented in the UI this way, the two types should have their own icons.

source/blender/makesdna/DNA_gpencil_types.h
339 ↗	(On Diff #39855)	Adding this flag here is confusing since it duplicates the stroke type, even with the "used only while drawing" comment. I would replace one of the padding bytes under `sbuffer_sflag` with `int8_t sbuffer_cap_type;` Then assigning the value becomes much simpler, since you don't have to go back and forth between a flag and the direct values.

This revision now requires changes to proceed.Jul 24 2021, 3:42 AM

Simplify code and reduce variables

Harbormaster completed remote builds in B16014: Diff 39910.Jul 24 2021, 11:27 AM

@Hans Goudey (HooglyBoogly) Thanks a lot for the idea to use the runtime flag... After reading your comment, I realised I could simplify a lot the code and remove the flag because the runtime has a pointer to Brush, so don't need the flag for drawing engine.

I have updated the patch and now all it's more clear.

In D11999#310161, @Hans Goudey (HooglyBoogly) wrote:

I don't think this should be committed to master with those icons.
With the option presented in the UI this way, the two types should have their own icons.

Agree, having those icons first would be great

Agree with Icons... who is the person in charge of making new icons?

@Pablo Vazquez (pablovazquez) @Hans Goudey (HooglyBoogly) Matias is working in the new icons, but IMHO it would be better apply this patch and open a task for the icons later in order to close patches. We have this patch waiting afor something that can be fixed later easily.

What do you think?

Okay, as long as icon work is actually underway I'm fine with committing this.

This revision is now accepted and ready to land.Aug 3 2021, 3:05 PM

Antonio Vazquez (antoniov) mentioned this in T90414: GPencil: New icons.Aug 3 2021, 8:45 PM

Rebase on master

Harbormaster completed remote builds in B16217: Diff 40313.Aug 4 2021, 3:52 PM

Closed by commit rBc1730ed16568: GPencil: New Brush option to define Caps type (authored by Antonio Vazquez (antoniov)). · Explain WhyAug 4 2021, 4:01 PM

This revision was automatically updated to reflect the committed changes.

Antonio Vazquez (antoniov) added a commit: rBc1730ed16568: GPencil: New Brush option to define Caps type.

GPencil: New Brush option to define Caps type
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 40315

release/scripts/startup/bl_ui/properties_paint_common.py

source/blender/draw/intern/draw_cache_impl_gpencil.c

source/blender/editors/gpencil/gpencil_paint.c

source/blender/editors/gpencil/gpencil_primitive.c

source/blender/makesdna/DNA_brush_types.h

source/blender/makesrna/intern/rna_brush.c

GPencil: New Brush option to define Caps typeClosedPublicActions