Create dot-dash effect for grease pencil strokes. User can manually edit the length, gap and styles for each segment of dashed lines.
The values in each segment can all be key-framed to make animations.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) Antonio Vazquez (antoniov) - Group Reviewers
Grease Pencil User Interface - Commits
- rBa2c5c2b4068d: GPencil: Dot dash modifier.
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Hand drawn grease pencil objects will have different results because of dense points. But for this sampling modifier should be used. The case is the same for adding noise mod to lineart or to Mesh converted to GP. Lineart had sampling option, but was removed for the same reason - this is a job for sampling modifier. Functionality should not be repeated. Plus there are artistic use cases to work with lets dense geometry.
- You have to use the stroke resample operator to get pleasing results, which means the modifier isn't as useful procedurally, it isn't obvious at first, and it adds another manual step for using this modifier.
- Is this the preferred design, or is the sampling done this way for technical reasons?
- I'm skeptical that every segment should have a name. Did this come from a design somewhere?
Materials list works the same way.
- Everything in the list is completely local, so as far as I can tell the names don't serve much of a purpose besides filling the space in the list, or setting it up to be shared and being very explicit about the meaning of each segment, which I'm not sure makes sense?
Imagine having a list of 10 segments, each having different material assigned. Naming is handy for organization
- We should keep in mind that generating many strokes will likely be quite inefficient for large scenes. Doing this in a modifier, I don't think there is really a way around generating very many strokes currently, but it's a downside to keep in mind.
Imagine using this with randomize offset modifier. Splitting into strokes is desired thing and allows for more artistic options. Please don't shut this down. Subsurface is inefficient but we have it for artistic expression
Yimings comment from the chat:
"about name thing, it's for proper operation of the key framing system, it has to have an identifier so when you move the list around it's still gonna stay at the same plce
Sampling... well If it's more convenient we can do it internally first and then dash... because hand-drawn ones already have a lot of points in them so unless it's specific for line art you typically won't have mch trouble"
In short I would dismiss all of those objections.
Comment Actions
Is this(manual sampling) the preferred design, or is the sampling done this way for technical reasons?
humm, maybe an option for internally sample the stroke before generating dash lines. Performance-wise it's gonna be more or less the same, but if we take segments instead of distance, the dash modifier could adapt to the drawing speed of each stroke.
I'm skeptical that every segment should have a name. Did this come from a design somewhere?
The only reason is just for it needs an identifier for properties in each list item to be keyed correctly.
I'm fixing the rest of the problems.
Comment Actions
After some testing, I think the UI works well now! I left some comments suggesting different property descriptions though.
I think there are still some improvements possible to the segment creation code though, I'll be more responsive in the future to work through that.
The "Offset" property seems redundant with the length modifier, what do you think about removing that so that each modifier is a bit simpler and does just one thing?
In general the patch description is still lacking, and has an old screenshot of the UI. The description can explain what the modifier does in more detail, a bit about how it works.
Maybe explaining some conversions that came up during code review is a good source of inspiration.
Hand drawn grease pencil objects will have different results because of dense points. But for this sampling modifier should be used.
To be clear, a sampling modifier doesn't exist, right? The confusion is probably understandable then.
Materials list works the same way.
Yes, but those are data-blocks, so it's obvious they have names. It's not so obvious here, though the other arguments for the names seem to make sense.
Imagine using this with randomize offset modifier. Splitting into strokes is desired thing and allows for more artistic options. Please don't shut this down. Subsurface is inefficient but we have it for artistic expression
I was noting that there are other designs that allow achieving the same results, clearly the results are nice, they basically speak for themselves. Noting a future bottleneck seems quite reasonable for code review.
Further, @Aleš Jelovčan (frogstomp), your comments "Please don't shut this down" and "I would dismiss all of those objections." miss the point of code review. The idea isn't to shut down patches or remove artistic expression, it's to make sure all options are considered, to make sure the change will make sense in the future, and to make sure it's usable.
| source/blender/blenkernel/BKE_gpencil_geom.h | ||
|---|---|---|
| 118–126 ↗ | (On Diff #40818) | This should be removed now too |
| source/blender/blenkernel/intern/gpencil_modifier.c | ||
| 1027 | Use BLI_listbase_is_empty | |
| source/blender/editors/object/object_gpencil_modifier.c | ||
| 962 | Use __func__ | |
| source/blender/gpencil_modifiers/intern/MOD_gpencildash.c | ||
| 96 | Use const | |
| 100–101 | Unused variable. | |
| 108–113 | I know it might be subjective, but I personally think macros like this significantly harm the readability of the code-- adding indirection, short names, and invisible logic. For GAP, if you always have to subtract one from it when you use it, I think that's a sign the value could be stored differently in the first place. | |
| 114–115 | Use LISTBASE_FOREACH macro | |
| 204 | const | |
| 239 | Extra whitespace here. | |
| 332–333 | This commented code should be removed if it isn't necessary. | |
| source/blender/makesdna/DNA_gpencil_modifier_defaults.h | ||
| 322 | There should probably also be an entry for the modifier itself here? | |
| source/blender/makesdna/DNA_gpencil_modifier_types.h | ||
| 524 | When the comment is exactly the same as the variable name, it's a sign that there should be a better comment, or no comment at all. Same with some of the other comments in this struct. | |
| source/blender/makesrna/intern/rna_gpencil_modifier.c | ||
| 3366 | Offset into each stroke before the beginning of the dashed segment generation | |
| 3369–3409 | All of these properties could be at the end of this function, to make it a bit more clear (the same order as the UI). Separately, it would be nice if there was a helper function to define all of these common properties. That's not blocking though. | |
| 3426 | The number of consecutive points from the original stroke to include in this segment | |
| 3431 | The number of points skipped after this segment | |
| 3435 | Any reason to use 0 for the precision value? Seems a bit weird | |
| 3435–3436 | Draw the segment with set radius -> The radius to assign to the new segment Same thing for the opacity description. But actually, it looks like this is a factor, so maybe The factor to apply to the original point's radius for the new points or something like that | |
| 3441 | Let's avoid the word "Draw" here, since this is really creating new segments. Drawing them is what the render engine does, and it's more of a technical word anyway. | |
| 3448 | current material -> `the existing material. This is also a new sentence, the comma should be a period. | |
Comment Actions
@Hans Goudey (HooglyBoogly) agreed about the point of the code review, we are in the same boat :)
I am all in favor for the code optimization. I am just extra careful about optimizing on the expense of removing some possibilities. I am arguing that having the thing split into strokes is desired result. Because you might want to further modify it with randomizing offset, especially in conjunction with weight modifier.
About sampling: Simplify -> sample.
I would like to remind that sampling was internal feature of lineart, but it was removed because we should use this modifier instead.
It follows the principle of not having duplicated functionality.
Anyways, I am getting build error with this patch applied, wanted to provide you with some examples, and if you can feedback this with other approaches to achieve the same result more efficiently I am all in favor.
| source/blender/makesdna/DNA_gpencil_modifier_defaults.h | ||
|---|---|---|
| 322 | THe modifier by itself doesn't have any "value-like" configurations, if I make a default dash list, it would become a static address there and if user were to remove those segments you can't Mem_freeN them. So I manually added one "default segment" in initData(). | |
| source/blender/makesdna/DNA_gpencil_modifier_types.h | ||
| 524 | Ehh right. @Antonio Vazquez (antoniov) Shall we remove other said comments in this file? Those common ones are in every single modifier... I can make a separate patch just for it. | |
| source/blender/makesrna/intern/rna_gpencil_modifier.c | ||
| 3369–3409 | Yeah I think so, those are common ones. Also @Antonio Vazquez (antoniov) | |
Comment Actions
This needs updating for changes in icons in master again.
I didn't see a response about the point about the offset's redundancy with the length modifier?
The point about the patch description is not addressed either.
| source/blender/gpencil_modifiers/intern/MOD_gpencildash.c | ||
|---|---|---|
| 100 | Original Index is a confusing name here, since this variable is really the start index of each new stroke. Maybe something like new_stroke_offset? or simply offset? | |
| 108–113 | You marked this "Done", but I don't see a change here, or a reply to this comment. I say this below too, but this method of declaring macros to access local variables is not standard in Blender, and there's no need for it here. | |
| 114 | ||
| 115 | Do you actually expect this to happen? If not, better to use an assert. | |
| 121–122 | Declare and assign variable on the same line, and declare as const | |
| 125–140 | A couple comments here about the purpose of each loop would go a long way to making this understandable. | |
| 130 | Extra parentheses are unnecessary here. | |
| 143 | There's got to be a better solution than subtracting by zero for every loop iteration except the first. Couldn't you increment original_index by trim_start before the loop? | |
| 145 | The code can be shorter and easier to read if you remove these trivial assignments to local variables and instead just use the values directly below. | |
| 153–160 | Looks like you can replace this whole for loop with a single subtraction. Also, size would be a more standard/understandable name than use_seg | |
| 181 | Use __func__ | |
| source/blender/makesdna/DNA_gpencil_modifier_defaults.h | ||
| 322 | It does though, currently it has an offset property. | |
| source/blender/makesdna/DNA_gpencil_modifier_types.h | ||
| 530 | This is not blocking at all, but did you consider storing the segments as an array instead of a ListBase? There is "rna_iterator_array_next" for the collection property. Since the algorithm loops over the segments, it might be slightly more efficient when there are many segments. Either way, good to know for the future. | |
| source/blender/makesrna/intern/rna_gpencil_modifier.c | ||
| 711–730 | Comment formatting. | |
| 3369–3409 | I meant that every property specific to this modifier should be defined at the top, and then the common ones should be defined at the bottom | |
| 3448 | The second part of this comment was not addressed. | |
| 3452 | RNA property identifiers shouldn't be abbreviated like this. I think material_index would be the proper name here, but you should check. | |
| 3453 | This should use a define or something rather than writing the number out. | |
Comment Actions
@Hans Goudey (HooglyBoogly) Hi! Thanks for the comments
The "Offset" property seems redundant with the length modifier, what do you think about removing that so that each modifier is a bit simpler and does just one thing?
I'm not sure, length modifier does not offset anything.
| source/blender/gpencil_modifiers/intern/MOD_gpencildash.c | ||
|---|---|---|
| 108–113 | I removed the other short names, they are not needed. the real_gap one I commented on the variable to make the purpose clear. | |
| 121–122 | ds is changing. I fixed assign | |
| 143 | That is used to keep track of the starting point along the original stroke. If increase that we actually move the starting point away from 0. I think it's okay to keep it this way otherwise we would write a very weird loop structure where this value is assigned at the end. | |
| source/blender/makesdna/DNA_gpencil_modifier_defaults.h | ||
| 322 | Ahh then I'll add a default of 0 then :D | |
| source/blender/makesrna/intern/rna_gpencil_modifier.c | ||
| 3369–3409 | Oh I see... | |
| source/blender/gpencil_modifiers/intern/MOD_gpencildash.c | ||
|---|---|---|
| 121–122 | const DashGpencilModifierSegment *ds means that the data that ds points to doesn't change, it doesn't mean that the pointer itself doesn't change. | |
Comment Actions
Also fixed logic for when offset<0
| source/blender/gpencil_modifiers/intern/MOD_gpencildash.c | ||
|---|---|---|
| 121–122 | Ah okay I understood now. | |
Comment Actions
Various updates:
- Store dash segment instructions in an array rather than a linked list
- Use proper translation for the new "Segment" name
- Slight cleanups: UI text, use enums
- Use function instead of macro
- Fix modifier dna defaults
| source/blender/makesdna/DNA_gpencil_modifier_defaults.h | ||
|---|---|---|
| 322 | I think this wasn't tested. It would crash Blender because the struct name was incorrect and it wasn't added to dna_defaults.c. I corrected this. | |
| source/blender/makesrna/intern/rna_gpencil_modifier.c | ||
| 293–305 | Cleanups like these should be committed separately, not including in feature patches. | |
Comment Actions
I just committed some updates I talked to Yiming about.
I guess it probably deserves another test from others now, but the code is satisfying to me, and the UI looks good.
Comment Actions
@Hans Goudey (HooglyBoogly), @YimingWu (NicksBest), I tested with multiple scenarios, tried animating every value, all works as expected, zero issues.
Comment Actions
Great addition to the grease pencil toolset. Thanks to all people involved in the patch!