Sculpt: Initialize hairs with custom radius #118339

Merged
Hans Goudey merged 10 commits from Zyq-XDz/blender:custom-hair-radius into main 2024-02-27 18:22:16 +01:00
Contributor

As #117101 (comment) mentioned, Curves Sculpting will initialize the radius of hair with zero, which is quite inconvenient. So I added the property curve_radius to BrushCurvesSculptSettings. Users could manually modify the initial radius through a slider, as demonstrated below:

image

Without any modification, the initial radius will be 0.01 instead of 0.

test scene: sculpt_add.blend

The alternative solution could simply initialize the radius property in curves_sculpt_add and curves_sculpt_density. However, I believe users need more options for the radius.

As https://projects.blender.org/blender/blender/issues/117101#issue-124440 mentioned, Curves Sculpting will initialize the radius of hair with zero, which is quite inconvenient. So I added the property ```curve_radius``` to ```BrushCurvesSculptSettings```. Users could manually modify the initial radius through a slider, as demonstrated below: ![image](/attachments/0671fe8b-5ba4-41a3-89d4-8e7cdb220733) Without any modification, the initial radius will be *0.01* instead of *0*. test scene: [sculpt_add.blend](/attachments/b4117f1f-db85-425b-981e-b8e33bc6a7bc) The alternative solution could simply initialize the radius property in ```curves_sculpt_add``` and ```curves_sculpt_density```. However, I believe users need more options for the radius.
XDzZyq added 5 commits 2024-02-15 17:26:53 +01:00
Iliya Katushenock added this to the Nodes & Physics project 2024-02-15 17:27:52 +01:00
Hans Goudey requested changes 2024-02-15 17:43:24 +01:00
Hans Goudey left a comment
Member

Thanks for the PR! My only larger comment is that this should implement the interpolate_radius option as well, for consistency with the other settings here, and because it can be an optimization to avoid adding the attribute unnecessarily in some cases. That could probably be turned on by default too.

Thanks for the PR! My only larger comment is that this should implement the `interpolate_radius` option as well, for consistency with the other settings here, and because it can be an optimization to avoid adding the attribute unnecessarily in some cases. That could probably be turned on by default too.
@ -521,7 +521,6 @@ Brush *BKE_brush_add(Main *bmain, const char *name, const eObjectMode ob_mode)
if (ob_mode == OB_MODE_SCULPT_CURVES) {
BKE_brush_init_curves_sculpt_settings(brush);
}
Member

Unrelated change

Unrelated change
Zyq-XDz marked this conversation as resolved
@ -285,6 +285,14 @@ struct DensityAddOperationExecutor {
selection.finish();
}
if (bke::GSpanAttributeWriter radius = attributes.lookup_for_write_span("radius")) {
Member

This should probably always create the attribute if the brush_settings_->curve_radius != 0.01f. Otherwise the setting wouldn't have any effect.

This should probably always create the attribute if the `brush_settings_->curve_radius != 0.01f`. Otherwise the setting wouldn't have any effect.
Zyq-XDz marked this conversation as resolved
@ -286,2 +286,4 @@
}
if (bke::GSpanAttributeWriter radius = attributes.lookup_for_write_span("radius")) {
blender::GMutableSpan radius_span = radius.span.slice(
Member

blender:: should be unnecessary here

Also, the radius attribute is guaranteed to always be on the point domain, no need to check that here.

`blender::` should be unnecessary here Also, the radius attribute is guaranteed to always be on the point domain, no need to check that here.
Zyq-XDz marked this conversation as resolved
XDzZyq added 1 commit 2024-02-16 04:18:44 +01:00
Now users could decide to interpolate the initial radius or not. And I moved the code of initialization from ```curves_sculpt_add.cc``` and ```curves_sculpt_density.cc``` into ```add_curves_on_mesh.cc```
Author
Contributor

Hi @HooglyBoogly,

I have implemented interpolate_radius in add_curves_on_mesh.cc. All functionalities seem to work, but I am not sure I used the right method to initialize the radius.

image

here is an example: sculpt_add.blend

Hi @HooglyBoogly, I have implemented ```interpolate_radius``` in ```add_curves_on_mesh.cc```. All functionalities seem to work, but I am not sure I used the right method to initialize the radius. ![image](/attachments/bedc4806-90f5-440f-9f68-424b106fbc26) here is an example: [sculpt_add.blend](/attachments/77d78077-02a4-4b23-8556-c12a997fd238)
XDzZyq requested review from Hans Goudey 2024-02-16 04:27:55 +01:00
XDzZyq changed title from WIP: Initialize hairs with custom radius to Initialize hairs with custom radius 2024-02-16 06:46:28 +01:00
XDzZyq changed title from Initialize hairs with custom radius to Sculpt: Initialize hairs with custom radius 2024-02-16 06:46:43 +01:00
Hans Goudey requested changes 2024-02-16 21:17:54 +01:00
Hans Goudey left a comment
Member

It looks like the radius interpolation is more complcated than I expected. The current PR fills the whole curve with the same value. Ideally it would interpolate along the length of the nearby curves-- I think the behavior is unexpected without that.

Let me know if you want to talk that through. Ideally we wouldn't need to sample the nearby curves more times for this, but the sampling factors/indices would be reused.

It looks like the radius interpolation is more complcated than I expected. The current PR fills the whole curve with the same value. Ideally it would interpolate along the length of the nearby curves-- I think the behavior is unexpected without that. Let me know if you want to talk that through. Ideally we wouldn't need to sample the nearby curves more times for this, but the sampling factors/indices would be reused.
@ -200,6 +200,9 @@ class CurvesGeometry : public ::CurvesGeometry {
Span<float3> positions() const;
MutableSpan<float3> positions_for_write();
Span<float> radiuses() const;
Member

We've avoided adding these so far, because there are some places that use the wrong defaults for when the attribute doesn't exist (basically it's just more complicated than it should be because of some questionable decisions from a while ago).

It should be fine to just use the attribute API to access the data instead.

We've avoided adding these so far, because there are some places that use the wrong defaults for when the attribute doesn't exist (basically it's just more complicated than it should be because of some questionable decisions from a while ago). It should be fine to just use the attribute API to access the data instead.
Zyq-XDz marked this conversation as resolved
XDzZyq added 1 commit 2024-02-17 11:18:24 +01:00
Now the interpolation will consider the the variation of the radius within neighbors.
XDzZyq requested review from Hans Goudey 2024-02-17 11:18:49 +01:00
Hans Goudey requested review from Jacques Lucke 2024-02-20 14:42:56 +01:00
Jacques Lucke reviewed 2024-02-20 18:17:35 +01:00
Jacques Lucke left a comment
Member

Seems like the code should create the radius attribute if it doesn't exist. Otherwise the behavior is kind of unexpected. It's weird to set a radius that is then not used.

Will leave the rest of the review to Hans who might want to do some deduplication in this area later.

Seems like the code should create the radius attribute if it doesn't exist. Otherwise the behavior is kind of unexpected. It's weird to set a radius that is then not used. Will leave the rest of the review to Hans who might want to do some deduplication in this area later.
Hans Goudey requested changes 2024-02-20 19:48:59 +01:00
Hans Goudey left a comment
Member

Seems like the code should create the radius attribute if it doesn't exist

Instead, I guess we can skip the interpolation if the attribute doesn't exist. Since the default value will still be fine. If interpolation is turned off though, the attribute should be added, yes.

>Seems like the code should create the radius attribute if it doesn't exist Instead, I guess we can skip the interpolation if the attribute doesn't exist. Since the default value will still be fine. If interpolation is turned off though, the attribute should be added, yes.
@ -236,13 +236,99 @@ static void interpolate_position_with_interpolation(CurvesGeometry &curves,
});
}
static void interpolate_radius_without_interpolation(CurvesGeometry &curves,
Member

"do thing without doing thing" is a confusing name! How about "calc_radius_without_interpolation"?

"do thing without doing thing" is a confusing name! How about "calc_radius_without_interpolation"?
Zyq-XDz marked this conversation as resolved
@ -239,0 +267,4 @@
bke::SpanAttributeWriter radius_attr =
curves.attributes_for_write().lookup_for_write_span<float>("radius");
MutableSpan<float3> positions_cu = curves.positions_for_write();
MutableSpan<float> radiuses_cu = radius_attr.span;
Member

Spelling: radiuses -> radii

Spelling: `radiuses` -> `radii`
Zyq-XDz marked this conversation as resolved
@ -239,0 +279,4 @@
const float root_cu = root_radiuses_cu[i];
if (neighbors.is_empty()) {
/* If there are no neighbors, just using uniform radius. */
Member

If there are no neighbors, where is root_radiuses_cu coming from?

If there are no neighbors, where is `root_radiuses_cu` coming from?
Zyq-XDz marked this conversation as resolved
@ -350,0 +439,4 @@
const bool has_radius_attr = !radius_attr.span.is_empty(); // check if the attribute exists
Array<float> root_radius_cu(added_curves_num);
if (inputs.interpolate_radius && has_radius_attr) {
interpolate_from_neighbors<float>(
Member

It's not clear why this is necessary. The radius of the first point of a curve shouldn't be special, since we're interpolating the radii from all points at factors along neighboring curves. Am I missing something?

It's not clear why this is necessary. The radius of the first point of a curve shouldn't be special, since we're interpolating the radii from all points at factors along neighboring curves. Am I missing something?
Zyq-XDz marked this conversation as resolved
XDzZyq added 1 commit 2024-02-21 03:57:40 +01:00
Interpolation On: interpolate if the radius attribute exists, or skip interpolation.

Interpolation Off: Check if the radius attribute does not exist with '''lookup_or_add_for_write_span'''. initialize radius with
constant number
XDzZyq requested review from Hans Goudey 2024-02-21 04:07:42 +01:00
Author
Contributor

Now interpolate_radius will always be on which means the radius attribute will not be added if users are adding curves. But once users turn off the interpolation, the radius attribute will be added with default value.image

Now ```interpolate_radius``` will always be on which means the radius attribute will not be added if users are adding curves. But once users turn off the interpolation, the radius attribute will be added with default value.![image](/attachments/35485a10-be74-4a54-90f3-8fec832f966c)
Member

Only blender organization members with write access can start builds. See documentation for details.

Only blender organization members with write access can start builds. See [documentation](https://projects.blender.org/infrastructure/blender-bot/src/branch/main/README.md) for details.
Hans Goudey requested changes 2024-02-27 15:57:15 +01:00
Hans Goudey left a comment
Member

Thanks! This works nicely in some simple tests. Just a few comments inline.

Thanks! This works nicely in some simple tests. Just a few comments inline.
@ -239,0 +245,4 @@
bke::SpanAttributeWriter radius_attr =
curves.attributes_for_write().lookup_or_add_for_write_span<float>("radius",
bke::AttrDomain::Point);
threading::parallel_for(IndexRange(added_curves_num), 256, [&](const IndexRange range) {
Member

This can be replaced with radius_attr.span.slice(new_points_range).fill(radius);

This can be replaced with `radius_attr.span.slice(new_points_range).fill(radius);`
Zyq-XDz marked this conversation as resolved
@ -239,0 +287,4 @@
continue;
}
radii_cu.slice(points).fill(0);
Member

0 -> 0.0f

`0` -> `0.0f`
Zyq-XDz marked this conversation as resolved
@ -239,0 +292,4 @@
for (const NeighborCurve &neighbor : neighbors) {
const int neighbor_curve_i = neighbor.index;
const IndexRange neighbor_points = points_by_curve[neighbor_curve_i];
const float3 &neighbor_root_cu = positions_cu[neighbor_points[0]];
Member

neighbor_root_cu is unused

`neighbor_root_cu` is unused
Zyq-XDz marked this conversation as resolved
@ -657,2 +657,2 @@
BRUSH_CURVES_SCULPT_FLAG_INTERPOLATE_SHAPE = (1 << 3),
BRUSH_CURVES_SCULPT_FLAG_INTERPOLATE_POINT_COUNT = (1 << 4),
BRUSH_CURVES_SCULPT_FLAG_INTERPOLATE_RADIUS = (1 << 3),
BRUSH_CURVES_SCULPT_FLAG_INTERPOLATE_SHAPE = (1 << 4),
Member

Best not to change these values, since they're stored in files. You can just add the new one at the end

Best not to change these values, since they're stored in files. You can just add the new one at the end
Zyq-XDz marked this conversation as resolved
XDzZyq added 1 commit 2024-02-27 17:21:41 +01:00
XDzZyq requested review from Hans Goudey 2024-02-27 17:22:56 +01:00
Hans Goudey reviewed 2024-02-27 17:26:30 +01:00
@ -239,0 +240,4 @@
const IndexRange new_points_range,
const float radius)
{
const OffsetIndices points_by_curve = curves.points_by_curve();
Member

points_by_curve is unused

`points_by_curve` is unused
Zyq-XDz marked this conversation as resolved
@ -239,0 +260,4 @@
curves.attributes_for_write().lookup_for_write_span<float>("radius");
if (!radius_attr) {
radius_attr.finish();
Member

No need to call finish() in this case :)

No need to call `finish()` in this case :)
Zyq-XDz marked this conversation as resolved
XDzZyq added 1 commit 2024-02-27 17:33:32 +01:00
Hans Goudey merged commit 999dfce736 into main 2024-02-27 18:22:16 +01:00
First-time contributor

I'm bit late to party, but I can see the 'fallback_curve_radius' is by default set to 0.
This mean that first new strands will be added with radius attribute of 0? I would imagine, initializing radius to 1 would be way more convenient for users. @Zyq-XDz
I mean the main issue in #117101 - was, that if there was no strands and (or) no radius attribute, the new strand gets initialized with radius 1.

I'm bit late to party, but I can see the 'fallback_curve_radius' is by default set to 0. This mean that first new strands will be added with radius attribute of 0? I would imagine, initializing radius to 1 would be way more convenient for users. @Zyq-XDz I mean the main issue in https://projects.blender.org/blender/blender/issues/117101 - was, that if there was no strands and (or) no radius attribute, the new strand gets initialized with radius 1.
Member

Thanks, committed the missing versioning here: d6f76c0889

Thanks, committed the missing versioning here: d6f76c08892cc776f776c8e62d36017bfe9493e6
Author
Contributor

Hi @JoseConseco, thanks for your idea,

This mean that first new strands will be added with radius attribute of 0.

To prevent extra memory usage, if adius attribute does not exits at beginning, adding curves will not add this new attribute.

but I can see the fallback_curve_radius is by default set to 0.

If the radius attribute exists (or interpolate_radius is off), the radius will be initialized with custom radius, and its default value is 0.01m not 0. The default value is 0.01 instead of 1 is because the unit of radius is meter. I believe it will be better to initialize with a plausible value. 😊image

Hi @JoseConseco, thanks for your idea, > This mean that first new strands will be added with radius attribute of 0. To prevent extra memory usage, if adius attribute does not exits at beginning, adding curves will not add this new attribute. > but I can see the ```fallback_curve_radius``` is by default set to 0. If the radius attribute exists (or ```interpolate_radius``` is off), the radius will be initialized with custom radius, and its default value is ```0.01m``` not 0. The default value is 0.01 instead of 1 is because the unit of radius is meter. I believe it will be better to initialize with a plausible value. 😊![image](/attachments/d02bccfa-794a-46c1-b167-b283c59c200d)
XDzZyq deleted branch custom-hair-radius 2024-04-02 10:22:35 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 Assignees
5 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#118339
No description provided.