GPv3: Opacity modifier #116946

Merged
Lukas Tönne merged 52 commits from LukasTonne/blender:gp3-opacity-modifier into main 2024-01-16 16:56:22 +01:00
Member

Opacity modifier implementation based on GP2. Functionality is largely unchanged:

Screenshot_20240112_174347

Color Mode is either Stroke or Fill for modifying color opacity or Hardness.
Uniform Opacity does two things at once (!):

  • Sets the same opacity value for every point in a stroke.
  • Sets opacity as an absolute value rather than a factor.

Weight as Factor (button to the right of Opacity Factor): Use the vertex group as opacity factor rather than an overall influence. This is confusing and hard to convey, but copies behavior from GP2.

The Influence panel contains the same filter settings as the GP2 modifier, with some small changes:

  • Layer selects only strokes in the respective layer (with an Invert option)
  • Material selects only points with the respective material (with an Invert option)
  • Layer Pass and Material Pass select only strokes/points which are rendered in the respective pass.
    Note 1: Layers don't have UI for setting a pass yet, this will be a generic layer attribute. This can be set through the API for testing.
    Note 2: In GP2 a pass value of zero was used to disable pass filters. Since zero is a valid pass ID an explicit flag has been added for the purpose of turning pass filters on and off.
  • Vertex Group: This can be used as an additional influence filter on points. If Weight as Factor is enable the vertex group instead replaces the opacity factor. In Fill mode the vertex group weight of the stroke's first point is used as influence for the entire stroke.
    Note: Vertex groups are not supported in GP3 yet. The modifier uses a stub function that returns a constant weight of 0.0f when a vertex group is used.
  • Custom Curve is another possible influence factor per point. The curve input value is the relative position of a point along its stroke.

The Influence settings have been moved into a separate DNA struct, which should help with reusability in various modifiers. Various utility functions can be found int MOD_grease_pencil_util.hh for handling influence settings and generating IndexMasks for modernized C++ code.

Opacity modifier implementation based on GP2. Functionality is largely unchanged: ![Screenshot_20240112_174347](/attachments/2954228e-ecf6-4fed-a97f-5c9ded4b3001) _Color Mode_ is either `Stroke` or `Fill` for modifying color opacity or `Hardness`. _Uniform Opacity_ does two things at once (!): - Sets the same opacity value for every point in a stroke. - Sets opacity as an absolute value rather than a factor. _Weight as Factor_ (button to the right of Opacity Factor): Use the vertex group as opacity __factor__ rather than an overall __influence__. This is confusing and hard to convey, but copies behavior from GP2. The _Influence_ panel contains the same filter settings as the GP2 modifier, with some small changes: - _Layer_ selects only strokes in the respective layer (with an _Invert_ option) - _Material_ selects only points with the respective material (with an _Invert_ option) - _Layer Pass_ and _Material Pass_ select only strokes/points which are rendered in the respective pass. _Note 1: Layers don't have UI for setting a pass yet, this will be a generic layer attribute. This can be set through the API for testing._ _Note 2: In GP2 a pass value of zero was used to disable pass filters. Since zero is a valid pass ID an explicit flag has been added for the purpose of turning pass filters on and off._ - _Vertex Group_: This can be used as an additional influence filter on points. If _Weight as Factor_ is enable the vertex group instead replaces the opacity factor. In _Fill_ mode the vertex group weight of the stroke's first point is used as influence for the entire stroke. _Note: Vertex groups are not supported in GP3 yet. The modifier uses a stub function that returns a constant weight of `0.0f` when a vertex group is used._ - _Custom Curve_ is another possible influence factor per point. The curve input value is the relative position of a point along its stroke. The Influence settings have been moved into a separate DNA struct, which should help with reusability in various modifiers. Various utility functions can be found int `MOD_grease_pencil_util.hh` for handling influence settings and generating `IndexMasks` for modernized C++ code.
Lukas Tönne added 1 commit 2024-01-09 15:39:40 +01:00
Lukas Tönne force-pushed gp3-opacity-modifier from 48f61cfe8b to ca665e7ad8 2024-01-10 12:19:26 +01:00 Compare
Lukas Tönne added 1 commit 2024-01-10 16:27:09 +01:00
2fe5dd11a1 DNA filter struct for grease pencil modifiers.
This comes with utility functions that generate index masks, which can
then be used to filter layers and strokes by layer name, material, or
pass.
Lukas Tönne added 1 commit 2024-01-10 18:29:23 +01:00
Lukas Tönne added 1 commit 2024-01-10 18:33:24 +01:00
Lukas Tönne added 1 commit 2024-01-10 18:59:36 +01:00
Lukas Tönne added 1 commit 2024-01-11 09:42:31 +01:00
Lukas Tönne added 1 commit 2024-01-11 10:48:08 +01:00
bab7a2f907 Use explicit flags to turn pass filters on/off.
GP2 was using "pass > 0" as a toggle for pass filters, but 0 is a valid
pass ID, so it was never possible to select pass 0.
Lukas Tönne added 1 commit 2024-01-11 11:41:53 +01:00
Lukas Tönne added 1 commit 2024-01-11 11:53:13 +01:00
Lukas Tönne added 2 commits 2024-01-11 12:15:10 +01:00
Lukas Tönne added 1 commit 2024-01-11 13:05:56 +01:00
25cd79488b Split the filters for GP modifiers into separate structs.
This will allow easier combination of influence settings for various
modifier types.
Lukas Tönne added 2 commits 2024-01-11 13:12:22 +01:00
Lukas Tönne added 1 commit 2024-01-11 15:12:01 +01:00
Lukas Tönne added 1 commit 2024-01-11 17:08:35 +01:00
Lukas Tönne added 1 commit 2024-01-11 18:17:45 +01:00
Lukas Tönne added 4 commits 2024-01-11 18:26:03 +01:00
Lukas Tönne added 2 commits 2024-01-12 14:21:10 +01:00
Lukas Tönne added 1 commit 2024-01-12 14:38:44 +01:00
Lukas Tönne added 1 commit 2024-01-12 15:04:30 +01:00
Lukas Tönne added 3 commits 2024-01-12 16:07:27 +01:00
Lukas Tönne added 1 commit 2024-01-12 16:52:34 +01:00
Lukas Tönne added 1 commit 2024-01-12 17:00:48 +01:00
Lukas Tönne changed title from WIP: Grease Pencil Opacity modifier to Grease Pencil Opacity modifier 2024-01-12 18:01:17 +01:00
Iliya Katushenock added this to the Grease Pencil project 2024-01-12 18:04:19 +01:00
Lukas Tönne reviewed 2024-01-12 18:07:12 +01:00
@ -0,0 +109,4 @@
/* TODO Would be nice to have the checkbox in the same line as the pass button. */
row = uiLayoutRow(col, true);
uiItemR(row, ptr, "use_layer_pass_filter", UI_ITEM_NONE, "Filter by layer pass", ICON_NONE);
Author
Member

Would be nice to have the checkbox in the same line as the pass button. I haven't found a good way to combine these buttons yet, could use help from UI experts.

Would be nice to have the checkbox in the same line as the pass button. I haven't found a good way to combine these buttons yet, could use help from UI experts.
Member

It's ugly but possible, here's an example from the limit location constraint:

        row = col.row(heading="Y", align=True)
        row.use_property_decorate = False
        sub = row.row(align=True)
        sub.prop(con, "use_max_y", text="")
        subsub = sub.row(align=True)
        subsub.active = con.use_max_y
        subsub.prop(con, "max_y", text="")
        row.prop_decorator(con, "max_y")
It's ugly but possible, here's an example from the limit location constraint: ``` row = col.row(heading="Y", align=True) row.use_property_decorate = False sub = row.row(align=True) sub.prop(con, "use_max_y", text="") subsub = sub.row(align=True) subsub.active = con.use_max_y subsub.prop(con, "max_y", text="") row.prop_decorator(con, "max_y") ```

I Agree to have the checkbox on the same line. Hans proposal seems a right choice

I Agree to have the checkbox on the same line. Hans proposal seems a right choice
Author
Member

I didn't know about the uiLayoutRowWithHeading feature, thanks.

I didn't know about the `uiLayoutRowWithHeading` feature, thanks.
LukasTonne marked this conversation as resolved
Lukas Tönne added 1 commit 2024-01-12 18:08:22 +01:00
Hans Goudey reviewed 2024-01-12 18:24:53 +01:00
Hans Goudey left a comment
Member

The code here might make more sense in the blender::modifiers namespace, since that matches the module. Potentially blender::modifiers::grease_pencil if that was helpful.

I don't see the "fill_opacity" attribute used in any existing code. I had thought that was replaced with the "opacity" attribute per point.

The code here might make more sense in the `blender::modifiers` namespace, since that matches the module. Potentially `blender::modifiers::grease_pencil` if that was helpful. I don't see the "fill_opacity" attribute used in any existing code. I had thought that was replaced with the "opacity" attribute per point.
@ -0,0 +80,4 @@
}
/* XXX Placeholder for vertex groupn weights. */
static VArray<float> get_grease_pencil_modifier_vertex_weights(
Member

You should just be able to retrieve the vertex group as an attribute

You should just be able to retrieve the vertex group as an attribute
Author
Member

Took some explaining from @filedescriptor how this works (maybe a good topic for technical docs?). As far as i understand the VArray wrapper for the curves->deform_verts() is ok here, since we're only using a single vertex group. For something like the armature modifier i'd guess we still want to use the MDeformVert directly.

Took some explaining from @filedescriptor how this works (maybe a good topic for technical docs?). As far as i understand the `VArray` wrapper for the `curves->deform_verts()` is ok here, since we're only using a single vertex group. For something like the armature modifier i'd guess we still want to use the `MDeformVert` directly.
Member

Right, exactly

Right, exactly
LukasTonne marked this conversation as resolved
@ -0,0 +162,4 @@
if (use_vgroup_opacity) {
/* Use the first stroke point as vertex weight. */
const IndexRange points_range = points_by_curve[curve_i];
Member

points_range -> points is the cononical variable name here. "range" is already described by the type

`points_range` -> `points` is the cononical variable name here. "range" is already described by the type
LukasTonne marked this conversation as resolved
@ -0,0 +163,4 @@
if (use_vgroup_opacity) {
/* Use the first stroke point as vertex weight. */
const IndexRange points_range = points_by_curve[curve_i];
const float stroke_weight = points_range.is_empty() ? 1.0f :
Member

You can count on the fact that curves always have at least one point

You can count on the fact that curves always have at least one point
Author
Member

At least one or at least two? I need two for this length factor calculation.

At least one or at least two? I need two for this length factor calculation.
Author
Member

Falk says curves can have 1 point only, so i still need to check.

Falk says curves can have 1 point only, so i still need to check.
LukasTonne marked this conversation as resolved
@ -0,0 +182,4 @@
const IndexMask &curves_mask)
{
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
bke::SpanAttributeWriter<float> hardnesses = attributes.lookup_or_add_for_write_span<float>(
Member

Could this be skipped if there's no hardness attribute already?

Could this be skipped if there's no hardness attribute already?
Author
Member

I suppose it could. The old modifier is quite inconsistent in how the "uniform" setting is applied (confusingly called "normalized" internally). In the color modes (stroke, fill) it then applies the settings as an offset rather than a factor, with some unexplained bias and clamping. But this isn't used for the hardness mode, so 🤷

I suppose it could. The old modifier is quite inconsistent in how the "uniform" setting is applied (confusingly called "normalized" internally). In the color modes (stroke, fill) it then applies the settings as an _offset_ rather than a factor, with some unexplained bias and clamping. But this isn't used for the hardness mode, so 🤷
LukasTonne marked this conversation as resolved
@ -0,0 +198,4 @@
const ModifierEvalContext *ctx,
bke::CurvesGeometry &curves)
{
GreasePencilOpacityModifierData *omd = (GreasePencilOpacityModifierData *)md;
Member

C++ style casts (elsewhere in this file too)

C++ style casts (elsewhere in this file too)
LukasTonne marked this conversation as resolved
Falk David changed title from Grease Pencil Opacity modifier to GPv3: Opacity modifier 2024-01-12 18:28:12 +01:00
Member

@HooglyBoogly "fill_opacity" is used for strokes with a fill material. Essentially this is the equivalent of the point opacity but for the entire n-gon shape.

@HooglyBoogly "fill_opacity" is used for strokes with a fill material. Essentially this is the equivalent of the point opacity but for the entire n-gon shape.
Member

Okay, I just don't see any mentions of "fill_opacity" in the source code. I guess it's planned?

Okay, I just don't see any mentions of `"fill_opacity"` in the source code. I guess it's planned?
Pratik Borhade reviewed 2024-01-13 08:06:47 +01:00
@ -0,0 +122,4 @@
else if (use_weight_as_factor) {
/* Use vertex group weights as opacity factors. */
const float vgroup_weight = vgroup_weights[point_i];
const float point_factor = vgroup_weight;
Member

remove this and pass vgroup_weight to clamp fn? 😅

remove this and pass `vgroup_weight` to clamp fn? 😅
Author
Member

Haha yeah, this is the result of me trying to make sense of the logic of the old modifier.

Haha yeah, this is the result of me trying to make sense of the logic of the old modifier.
LukasTonne marked this conversation as resolved
@ -0,0 +239,4 @@
*grease_pencil, omd->influence, mask_memory);
Vector<Drawing *> drawings = greasepencil::get_drawings_for_write(
*grease_pencil, layer_mask, frame);
for (Drawing *drawing : drawings) {
Member

threading::parallel_for ?

`threading::parallel_for` ?
LukasTonne marked this conversation as resolved
Iliya Katushenock reviewed 2024-01-13 11:25:34 +01:00
@ -0,0 +131,4 @@
const float vgroup_weight = vgroup_weights[point_i];
const float vgroup_influence = invert_vertex_group ? 1.0f - vgroup_weight : vgroup_weight;
opacities.span[point_i] = std::clamp(
opacities.span[point_i] + omd.color_factor * curve_factor * vgroup_influence - 1.0f,

Wondering if this could be made much clean by using DefaultMixer?

Wondering if this could be made much clean by using `DefaultMixer`?
Author
Member

That seems a bit unnecessary here.

  • The attributes are all floats, no need to handle different types.
  • The math is weird and undocumented. In the "uniform" mode it uses 1 - current as some kind of cutoff point, and then allows ramping up the influence to 2.0, presumably so one can reach full opacity/hardness again. It all seems very ad-hoc.

I'd prefer to not try and "fix" this too much, since the primary purpose is compatibility with GP2.

That seems a bit unnecessary here. - The attributes are all floats, no need to handle different types. - The math is weird and undocumented. In the "uniform" mode it uses `1 - current` as some kind of cutoff point, and then allows ramping up the influence to 2.0, presumably so one can reach full opacity/hardness again. It all seems very ad-hoc. I'd prefer to not try and "fix" this too much, since the primary purpose is compatibility with GP2.
Member

Okay, I just don't see any mentions of "fill_opacity" in the source code. I guess it's planned?

Yes, we don't support it yet.

> Okay, I just don't see any mentions of `"fill_opacity"` in the source code. I guess it's planned? Yes, we don't support it yet.
Falk David requested changes 2024-01-14 16:14:40 +01:00
@ -46,6 +46,7 @@ class DATA_PT_modifiers(ModifierButtonsPanel, Panel):
def draw(self, _context):
layout = self.layout
ob_type = _context.object.type
Member

Was this added by mistake? I don't see where ob_type is used here.

Was this added by mistake? I don't see where `ob_type` is used here.
LukasTonne marked this conversation as resolved
@ -154,2 +155,2 @@
* Only set on modifiers in evaluated objects. The flag indicates that the user modified inputs
* to the modifier which might invalidate simulation caches.
* Only set on modifiers in evaluated objects. The flag indicates that the user modified
* inputs to the modifier which might invalidate simulation caches.
Member

Looks like a clang-format issue here.

Looks like a `clang-format` issue here.
LukasTonne marked this conversation as resolved
@ -7426,0 +7574,4 @@
RNA_def_property_update(prop, 0, "rna_Modifier_update");
}
static void rna_def_modifier_grease_pencil_vertex_group(StructRNA *srna,
Member

Should this be rna_def_modifier_grease_pencil_vertex_group_filter to be consistent with the other filters?

Should this be `rna_def_modifier_grease_pencil_vertex_group_filter` to be consistent with the other filters?
Author
Member

Not really, the other "filters" are purely selection masks based on layers or materials. The vertex group is always either a multiplier or, in the case of the opacity modifier, an offset/factor/something.

I appended the "filter" suffix to layer/material mostly so it would be clear that these are just selection criteria and their settings don't otherwise affect the modifier.

Not really, the other "filters" are purely selection masks based on layers or materials. The vertex group is always either a multiplier or, in the case of the opacity modifier, an offset/factor/something. I appended the "filter" suffix to layer/material mostly so it would be clear that these are just selection criteria and their settings don't otherwise affect the modifier.
LukasTonne marked this conversation as resolved
Member

The code here might make more sense in the blender::modifiers namespace, since that matches the module.

Looks like blenkernel/intern/modifier.cc doesn't have those BKE_ functions in blender::modifiers or blender::bke namespace, is this intentional? Or it's just pending migration? I could maybe update that part in a separate PR as well.

> The code here might make more sense in the `blender::modifiers` namespace, since that matches the module. Looks like `blenkernel/intern/modifier.cc` doesn't have those `BKE_` functions in `blender::modifiers` or `blender::bke` namespace, is this intentional? Or it's just pending migration? I could maybe update that part in a separate PR as well.
Lukas Tönne added 2 commits 2024-01-15 16:29:04 +01:00
Lukas Tönne added 1 commit 2024-01-15 16:58:19 +01:00
Lukas Tönne added 2 commits 2024-01-16 12:08:42 +01:00
Lukas Tönne added 3 commits 2024-01-16 12:36:35 +01:00
Lukas Tönne added 3 commits 2024-01-16 15:25:58 +01:00
Lukas Tönne added 1 commit 2024-01-16 15:30:15 +01:00
Lukas Tönne added 1 commit 2024-01-16 15:37:56 +01:00
Lukas Tönne added 1 commit 2024-01-16 15:50:25 +01:00
Falk David approved these changes 2024-01-16 15:57:00 +01:00
Falk David left a comment
Member

Looks good to me!

Looks good to me!
Hans Goudey approved these changes 2024-01-16 16:05:44 +01:00
@ -103,0 +104,4 @@
"GREASE_PENCIL_OPACITY",
ICON_MOD_OPACITY,
"Opacity",
"Opacity of the strokes"},
Member

Opacity of the strokes -> Change the opacity of the strokes

I know this is just copying the old description, might as well fix it though

`Opacity of the strokes` -> `Change the opacity of the strokes` I know this is just copying the old description, might as well fix it though
LukasTonne marked this conversation as resolved
@ -0,0 +1,338 @@
/* SPDX-FileCopyrightText: 2005 Blender Authors
Member

Copyright year can be updated

Copyright year can be updated
LukasTonne marked this conversation as resolved
@ -0,0 +57,4 @@
static void copy_data(const ModifierData *md, ModifierData *target, const int flag)
{
const GreasePencilOpacityModifierData *omd =
Member

You can write const auto * with the cast, it might keep it on one line

You can write `const auto *` with the cast, it might keep it on one line
LukasTonne marked this conversation as resolved
@ -0,0 +94,4 @@
bke::MutableAttributeAccessor attributes = curves.attributes_for_write();
bke::SpanAttributeWriter<float> opacities = attributes.lookup_or_add_for_write_span<float>(
"opacity", bke::AttrDomain::Point);
const bke::AttributeReader<float> vgroup_weights = attributes.lookup_or_default<float>(
Member

Use the * operator overload to just store the VArray directly here

Use the * operator overload to just store the VArray directly here
LukasTonne marked this conversation as resolved
Lukas Tönne added 4 commits 2024-01-16 16:16:38 +01:00

Some notes regarding UI

image

  • I don't think the word Color should be used in Mode, the Mode is the geometry that is affected by the modifier.
  • Filter word in passes seems also unnecessary.
  • Name should be changed to Vertex Group.

The indent for passes is a little off. Is possible to fix somehow?

Some notes regarding UI ![image](/attachments/b3176fe8-7c82-4d30-9671-3f7f88ee9adc) - I don't think the word Color should be used in Mode, the Mode is the geometry that is affected by the modifier. - Filter word in passes seems also unnecessary. - Name should be changed to Vertex Group. The indent for passes is a little off. Is possible to fix somehow?
Matias Mendiola closed this pull request 2024-01-16 16:19:49 +01:00
Lukas Tönne reopened this pull request 2024-01-16 16:21:17 +01:00
Author
Member

@mendio When you click "Comment and Close" it actually closes the PR. Confusing, i know :)

@mendio When you click "Comment and Close" it actually closes the PR. Confusing, i know :)

sorry for the close, my bad :)

sorry for the close, my bad :)
Lukas Tönne added 1 commit 2024-01-16 16:47:16 +01:00
Lukas Tönne added 1 commit 2024-01-16 16:54:25 +01:00
Author
Member

Names simplified.

The indent for passes is a little off. Is possible to fix somehow?

Thanks for catching, i had the property decorators disabled on the wrong sub-layout.

Names simplified. > The indent for passes is a little off. Is possible to fix somehow? Thanks for catching, i had the property decorators disabled on the wrong sub-layout.
Lukas Tönne merged commit c02f3c94d9 into main 2024-01-16 16:56:22 +01:00
Lukas Tönne referenced this issue from a commit 2024-01-16 16:56:23 +01:00
Lukas Tönne deleted branch gp3-opacity-modifier 2024-01-16 16:56:25 +01:00
Hans Goudey referenced this issue from a commit 2024-01-17 16:39:23 +01:00
Arye Ramaty referenced this issue from a commit 2024-01-21 15:37:41 +01:00
Sign in to join this conversation.
No reviewers
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
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116946
No description provided.