GPv3: Outline modifier #118911

Merged
Lukas Tönne merged 34 commits from LukasTonne/blender:gp3-outline-modifier into main 2024-03-04 16:54:18 +01:00
Member

Port of GPv2 outline modifier.

TODO

  • Set stroke radius from OutlineGpencilModifierData::thickness.
  • Implement miter offset to avoid pinching inside corners.
  • Implement arcs for outside corners.
  • Support keep_shape option (offset perimeter toward the curve by 1 radius)
  • Include object scale in the radius factor like GPv2 does (code)
  • Assign outline_material to the resulting strokes.
  • Support start point selection based on proximity to OutlineGpencilModifierData::object. This object is not used for anything else.
    (Not the best feature, but we'll keep it for GPv2 compatibility)
  • Implement re-sampling after perimeter generation using OutlineGpencilModifierData::sample_length.
Port of GPv2 outline modifier. ## TODO - [x] Set stroke radius from `OutlineGpencilModifierData::thickness`. - [x] Implement miter offset to avoid pinching inside corners. - [x] Implement arcs for outside corners. - [x] Support `keep_shape` option (offset perimeter toward the curve by 1 radius) - [x] Include object scale in the radius factor like GPv2 does ([code](https://projects.blender.org/blender/blender/src/commit/3a4220001b5831c12e41a28ba473689efdd32a3e/source/blender/gpencil_modifiers_legacy/intern/MOD_gpencil_legacy_outline.cc#L119)) - [x] Assign `outline_material` to the resulting strokes. - [x] Support start point selection based on proximity to `OutlineGpencilModifierData::object`. This object is not used for anything else. (Not the best feature, but we'll keep it for GPv2 compatibility) - [x] Implement re-sampling after perimeter generation using `OutlineGpencilModifierData::sample_length`.
Lukas Tönne added 9 commits 2024-02-29 16:29:58 +01:00
Lukas Tönne added this to the Grease Pencil project 2024-02-29 16:30:03 +01:00
Member

For resampling there is geometry::resample_to_length now.

For resampling there is `geometry::resample_to_length` now.
Lukas Tönne added 1 commit 2024-02-29 17:53:34 +01:00
Lukas Tönne added 1 commit 2024-02-29 18:16:49 +01:00
Lukas Tönne added 1 commit 2024-03-01 10:26:25 +01:00
Lukas Tönne added 1 commit 2024-03-01 11:22:10 +01:00
7ed9b55b53 Always generate end caps, even for cyclic curves.
In principle generating separate inside/outside perimeters is better,
but the renderer does not support holes in cyclic curves atm, so the
GPv2 method with end caps is preferable.
Lukas Tönne added 1 commit 2024-03-01 11:27:44 +01:00
Lukas Tönne added 1 commit 2024-03-01 13:12:31 +01:00
Lukas Tönne added 1 commit 2024-03-01 14:10:48 +01:00
Lukas Tönne added 1 commit 2024-03-01 14:21:14 +01:00
Lukas Tönne added 1 commit 2024-03-01 18:16:20 +01:00
Lukas Tönne added 1 commit 2024-03-01 19:05:51 +01:00
Lukas Tönne added 1 commit 2024-03-01 21:18:55 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
fcb7ab0d4d
Fixed the point offset feature, use std::rotate.
Lukas Tönne changed title from WIP: GPv3: Outline modifier to GPv3: Outline modifier 2024-03-01 21:19:56 +01:00
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2024-03-01 21:27:24 +01:00
Dismissed
Hans Goudey left a comment
Member

Wow, that's a solid chunk of work! From skimming the code, I'm guessing it's much faster than the original :)

Wow, that's a solid chunk of work! From skimming the code, I'm guessing it's much faster than the original :)
@ -0,0 +121,4 @@
namespace array_utils {
template<typename T>
inline void rotate_group_to_group(const OffsetIndices<int> src_offsets,
Member

I'm concerned about the increase in binary size as we add more uses of convert_to_static_type and more attribute types. Especially for something like a legacy modifier where performance isn't the greatest concern, and where performance will probably be vastly better than the old modifier anyway, it doesn't feel worth it.

The alternative would be building the full Array<int> index map from dst to src, then using array_utils::gather instead of this template.

I'm concerned about the increase in binary size as we add more uses of `convert_to_static_type` and more attribute types. Especially for something like a legacy modifier where performance isn't the greatest concern, and where performance will probably be vastly better than the old modifier anyway, it doesn't feel worth it. The alternative would be building the full `Array<int>` index map from dst to src, then using `array_utils::gather` instead of this template.
Author
Member

I can do it with gather_attributes, but unfortunately this does not work for in-place rotation of buffers: one half of the buffer is moved first, overwriting part of the other half before it gets copied. The rotate function takes care to swap elements to allow in-place rotation. I'll have to create a full copy of the curves geometry, which is probably acceptable in this case because it's a somewhat clumsy legacy feature that isn't expected to be very performant.

I can do it with `gather_attributes`, but unfortunately this does not work for in-place rotation of buffers: one half of the buffer is moved first, overwriting part of the other half before it gets copied. The `rotate` function takes care to swap elements to allow in-place rotation. I'll have to create a full copy of the curves geometry, which is probably acceptable in this case because it's a somewhat clumsy legacy feature that isn't expected to be very performant.
LukasTonne marked this conversation as resolved
@ -0,0 +674,4 @@
using bke::greasepencil::Layer;
using modifier::greasepencil::LayerDrawingInfo;
auto *omd = reinterpret_cast<GreasePencilOutlineModifierData *>(md);
Member

const auto * and const GreasePencilOutlineModifierData

`const auto *` and `const GreasePencilOutlineModifierData`
LukasTonne marked this conversation as resolved
@ -0,0 +707,4 @@
PointerRNA ob_ptr;
PointerRNA *ptr = modifier_panel_get_property_pointers(panel, &ob_ptr);
// auto *omd = static_cast<GreasePencilOutlineModifierData *>(ptr->data);
Member

Unused

Unused
LukasTonne marked this conversation as resolved

Tested, generally works great, but consistently crashes when drawing with modifier on with no existing strokes. This is a case for every frame.
If modifier is set to Off, stroke is drawn and then turned On, you can continue drawing without issues.

Tested, generally works great, but consistently crashes when drawing with modifier on with no existing strokes. This is a case for every frame. If modifier is set to Off, stroke is drawn and then turned On, you can continue drawing without issues.
Member

@frogstomp @LukasTonne I suspect that the case where a stroke has a single point is not properly handled.

@frogstomp @LukasTonne I suspect that the case where a stroke has a single point is not properly handled.
Author
Member

Tested, generally works great, but consistently crashes when drawing with modifier on with no existing strokes. This is a case for every frame.
If modifier is set to Off, stroke is drawn and then turned On, you can continue drawing without issues.

Looks like gather_attributes does not like empty domains, it ends up trying to write to the last entry in an empty offsets array. This should probably get a lower-level fix to make gather_attributes more robust, but i'll add an early exit to avoid the case.

> Tested, generally works great, but consistently crashes when drawing with modifier on with no existing strokes. This is a case for every frame. > If modifier is set to Off, stroke is drawn and then turned On, you can continue drawing without issues. Looks like `gather_attributes` does not like empty domains, it ends up trying to write to the last entry in an empty offsets array. This should probably get a lower-level fix to make `gather_attributes` more robust, but i'll add an early exit to avoid the case.
Author
Member

Start and end caps currently "flicker" while drawing or changing modifier settings, like the direction flips randomly.
EDIT: fixed.

Start and end caps currently "flicker" while drawing or changing modifier settings, like the direction flips randomly. EDIT: fixed.
Lukas Tönne added 4 commits 2024-03-02 12:51:01 +01:00
Lukas Tönne added 1 commit 2024-03-02 15:31:45 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
6b88210e0d
Fixed point index used for cap generation and stable angle.
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-03-02 15:36:36 +01:00
Lukas Tönne added 1 commit 2024-03-02 15:37:38 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
69dea1dbe1
Merge branch 'main' into gp3-outline-modifier
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne requested review from Hans Goudey 2024-03-02 16:05:04 +01:00
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-03-03 10:54:07 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
850b64c660
Merge branch 'main' into gp3-outline-modifier
Author
Member

@blender-bot build

@blender-bot build
Aleš Jelovčan closed this pull request 2024-03-03 11:32:27 +01:00
Iliya Katushenock reopened this pull request 2024-03-03 11:33:09 +01:00

Previously reported bugs are now fixed, found one more.
Crash with flat caps, steps to reproduce:

  • make a stroke with flat caps On
Previously reported bugs are now fixed, found one more. Crash with flat caps, steps to reproduce: - make a stroke with flat caps On
Lukas Tönne added 1 commit 2024-03-03 12:44:27 +01:00
Falk David requested changes 2024-03-04 10:54:35 +01:00
Dismissed
Falk David left a comment
Member

Got some cleanup comments.

Got some cleanup comments.
@ -0,0 +108,4 @@
ctx->node, ctx->object, DEG_OB_COMP_TRANSFORM, "Grease Pencil Outline Modifier");
}
static bke::CurvesGeometry reorder_cyclic_curve_points(const bke::CurvesGeometry &src_curves,
Member

I can't quite decipher what this function does. Would be helpful if there was some comment at the top.

I can't quite decipher what this function does. Would be helpful if there was some comment at the top.
LukasTonne marked this conversation as resolved
@ -0,0 +128,4 @@
return;
}
const int shift_raw = curve_offsets[curve_i];
const int shift = shift_raw >= 0 ? shift_raw % points.size() :
Member

And what this shift is.

And what this shift is.
LukasTonne marked this conversation as resolved
@ -0,0 +153,4 @@
static int find_closest_point(const Span<float3> positions, const float3 &target)
{
if (positions.is_empty()) {
return -1;
Member

I think this can just return 0, so that we can directly use the index instead of the closest_i >= 0 ? closest_i : 0;.

I think this can just return 0, so that we can directly use the index instead of the `closest_i >= 0 ? closest_i : 0;`.
LukasTonne marked this conversation as resolved
Lukas Tönne added 4 commits 2024-03-04 14:25:38 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
0293f50a8b
Always return a valid point index as the closest.
Lukas Tönne requested review from Falk David 2024-03-04 14:25:52 +01:00
Falk David approved these changes 2024-03-04 14:29:21 +01:00
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2024-03-04 15:48:45 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
7a0a521236
Cleanup: formatting.
Hans Goudey reviewed 2024-03-04 15:52:40 +01:00
@ -0,0 +152,4 @@
* source indices are not ordered. */
bke::CurvesGeometry dst_curves(src_curves);
bke::MutableAttributeAccessor dst_attributes = dst_curves.attributes_for_write();
bke::gather_attributes(src_attributes, bke::AttrDomain::Point, {}, {}, indices, dst_attributes);
Member

Looks like this is losing curve attributes?

Looks like this is losing curve attributes?
Member

Sorry, never mind, I read this wrong!

Sorry, never mind, I read this wrong!
Author
Member

Curves are copied in their entirety first, then point attributes get rewritten: bke::CurvesGeometry dst_curves(src_curves). It's fine, couldn't find anything wrong in testing either, custom attributes from a preceding GN modifier get propagated correctly on curve and point domains.

Curves are copied in their entirety first, then point attributes get rewritten: `bke::CurvesGeometry dst_curves(src_curves)`. It's fine, couldn't find anything wrong in testing either, custom attributes from a preceding GN modifier get propagated correctly on curve and point domains.
LukasTonne marked this conversation as resolved
Hans Goudey approved these changes 2024-03-04 16:49:17 +01:00
Lukas Tönne merged commit d6aadd6692 into main 2024-03-04 16:54:18 +01:00
Lukas Tönne referenced this issue from a commit 2024-03-04 16:54:18 +01:00
Lukas Tönne deleted branch gp3-outline-modifier 2024-03-04 16:54:21 +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
4 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#118911
No description provided.