Curves: Sculpt mode frame selected operator support #109924

Merged
Hans Goudey merged 17 commits from mod_moder/blender:tmp_frame_on_selection into main 2023-11-03 09:43:05 +01:00

Implementation of Frame to Selected operator for curves sculp.

Implementation of Frame to Selected operator for curves sculp.
Iliya Katushenock added the
Interest
Geometry Nodes
label 2023-07-10 17:09:39 +02:00
Iliya Katushenock added 1 commit 2023-07-10 17:09:50 +02:00
Iliya Katushenock added this to the Nodes & Physics project 2023-07-10 17:09:54 +02:00
Hans Goudey requested changes 2023-07-10 17:45:10 +02:00
Hans Goudey left a comment
Member

Do the other modes use the evaluated geometry rather than the original geometry?

Also, it might be better to use the deformed positions in GeometryComponentEditData combined with the selection on the original geometry. That's probably more reliable. But maybe I'm missing something.

Do the other modes use the evaluated geometry rather than the original geometry? Also, it might be better to use the deformed positions in `GeometryComponentEditData` combined with the selection on the original geometry. That's probably more reliable. But maybe I'm missing something.
@ -45,0 +73,4 @@
return GVArray::ForSingle(*default_value.type(), domain_size, default_value.get());
}
static std::optional<Bounds<float3>> selection_bounds_for_curves(const bke::CurvesGeometry &curves)
Member

I'd suggest a couple changes:

  • Add a version of the bounds function that takes an index mask argument
  • Use retrieve_selected_points and IndexMask::from_bools for other geometry types
I'd suggest a couple changes: - Add a version of the bounds function that takes an index mask argument - Use `retrieve_selected_points` and `IndexMask::from_bools` for other geometry types
Author
Member

The main reason for splitting logic for different domain is that we can do simpler selection transformations:

  • For curves:
    • If curve domain: IndexMask::ForOffsetInfides(curves.points_by_curve(), curves_mask);
    • If points domain: IndexMask::from_bools(boolean_selection);
  • For mesh:
    • If points domain: IndexMask::from_bools(boolean_selection);
      • If if enough size: mesh.bounds_cache;
    • If other domain: IndexMask::from_bools(boolean_selection);
  • For points:
    • IndexMask::from_bools(boolean_selection);
      • If if enough size: points.runtime.bounds_cache;
The main reason for splitting logic for different domain is that we can do simpler selection transformations: - For curves: - If curve domain: `IndexMask::ForOffsetInfides(curves.points_by_curve(), curves_mask);` - If points domain: `IndexMask::from_bools(boolean_selection);` - For mesh: - If points domain: `IndexMask::from_bools(boolean_selection);` - If if enough size: `mesh.bounds_cache`; - If other domain: `IndexMask::from_bools(boolean_selection);` - For points: - `IndexMask::from_bools(boolean_selection);` - If if enough size: `points.runtime.bounds_cache;`
Member

For this feature, I think I'd rather avoid the complexity and use the attribute API for the domain and type conversions when necessary. If the performance is a problem, it can be improved later.

For this feature, I think I'd rather avoid the complexity and use the attribute API for the domain and type conversions when necessary. If the performance is a problem, it can be improved later.
mod_moder marked this conversation as resolved
@ -45,0 +107,4 @@
bounds_list.append(*bounds);
}
}
if (geometry.has_mesh()) {
Member

if (const Mesh *mesh = geometry.get_mesh_for_read()) {

`if (const Mesh *mesh = geometry.get_mesh_for_read()) {`
mod_moder marked this conversation as resolved
@ -45,0 +116,4 @@
}
if (geometry.has_pointcloud()) {
const PointCloud &points = *geometry.get_pointcloud_for_read();
const std::optional<Bounds<float3>> bounds = selection_bounds_for_points(points);
Member

The variable declaration and if statement can be combined into one line here too

The variable declaration and if statement can be combined into one line here too
mod_moder marked this conversation as resolved
Iliya Katushenock added 1 commit 2023-07-10 19:48:06 +02:00
Author
Member

Do the other modes use the evaluated geometry rather than the original geometry?

Here I observe some unsequence (maybe just a bug, lagacy or undefined, for now I'll just consider this how it works):

  • Generated and armature modifiers are taken into account for this in edit mode.
  • The array modifier ignored in edit mode.
  • In object mode, evaluated geometry and grease pencil are used.
  • The geometry from the preview node is ignored.
  • The legacy grease pencil completely ignores modifiers in all modes.

Also, it might be better to use the deformed positions in GeometryComponentEditData combined with the selection on the original geometry.

Not sure why (for what):

  • We don't see the original selected points because we're working with a evaluated geometry (unless we see both versions at the same time).
  • We don't want to have a reverse version of the transform (to reflect the brush correctly).
> Do the other modes use the evaluated geometry rather than the original geometry? Here I observe some unsequence (maybe just a bug, lagacy or undefined, for now I'll just consider this how it works): - Generated and armature modifiers are taken into account for this in edit mode. - The array modifier ignored in edit mode. - In object mode, evaluated geometry and grease pencil are used. - The geometry from the preview node is ignored. - The legacy grease pencil completely ignores modifiers in all modes. > Also, it might be better to use the deformed positions in GeometryComponentEditData combined with the selection on the original geometry. Not sure why (for what): - We don't see the original selected points because we're working with a evaluated geometry (unless we see both versions at the same time). - We don't want to have a reverse version of the transform (to reflect the brush correctly).
Iliya Katushenock added 2 commits 2023-07-14 20:05:18 +02:00
Iliya Katushenock added 1 commit 2023-07-14 21:10:30 +02:00
Iliya Katushenock added 1 commit 2023-07-14 21:15:36 +02:00
Iliya Katushenock changed title from WIP: Frame Selected operator for curves sculpting to Frame Selected operator for curves sculpting 2023-07-14 21:18:46 +02:00
Iliya Katushenock requested review from Hans Goudey 2023-07-14 21:18:56 +02:00
Hans Goudey requested changes 2023-07-14 21:29:43 +02:00
Hans Goudey left a comment
Member

Thanks, it's getting there! Can be simpler still though.

  • Use curves::retrieve_selected_points to retrieve the selection index mask
  • Move the selection_bounds function to ED_curves.h, since it's specific to curves now
Thanks, it's getting there! Can be simpler still though. - Use `curves::retrieve_selected_points` to retrieve the selection index mask - Move the `selection_bounds` function to `ED_curves.h`, since it's specific to curves now
Iliya Katushenock requested review from Hans Goudey 2023-07-14 21:47:31 +02:00
Iliya Katushenock added 2 commits 2023-07-14 21:47:40 +02:00
Hans Goudey reviewed 2023-07-16 05:05:38 +02:00
@ -65,0 +80,4 @@
init,
[&](const IndexRange range, const Bounds<T> &init) {
Bounds<T> result = init;
mask.slice(range).foreach_index_optimized<int>(
Member

Can't assume the number of elements is less than 2 billion here in this generic header-- so int64_t here

Can't assume the number of elements is less than 2 billion here in this generic header-- so `int64_t` here
mod_moder marked this conversation as resolved
@ -25,0 +28,4 @@
std::optional<Bounds<float3>> selection_bounds(
const Curves *curves_id, const bke::crazyspace::GeometryDeformation deformation)
{
if (curves_id == nullptr) {
Member

Such a function shouldn't really have a null check.

Such a function shouldn't really have a null check.
mod_moder marked this conversation as resolved
@ -1378,0 +1386,4 @@
using namespace blender::bke::crazyspace;
const Object &ob_orig = *DEG_get_original_object(ob_eval);
const GeometryDeformation deformation = get_evaluated_curves_deformation(ob_eval, ob_orig);
const Curves *curves = static_cast<const Curves *>(ob_eval->data);
Member

I thought we decided to retrieve the selection from the original curves. Since they have the same number of points, that should amount to the same thing, but rely on interpolation of selection to the evaluated curves.

I thought we decided to retrieve the selection from the original curves. Since they have the same number of points, that should amount to the same thing, but rely on interpolation of selection to the evaluated curves.
mod_moder marked this conversation as resolved
Hans Goudey changed title from Frame Selected operator for curves sculpting to Curves: Sculpt mode frame selected operator support 2023-07-16 05:06:23 +02:00
Iliya Katushenock added 1 commit 2023-07-16 15:22:53 +02:00
Iliya Katushenock added 1 commit 2023-07-16 15:24:11 +02:00
Member

Sorry for the delay. If you merge main into this branch, I'll take a look again-- I think I was close to accepting this before.

Sorry for the delay. If you merge main into this branch, I'll take a look again-- I think I was close to accepting this before.
Iliya Katushenock added 3 commits 2023-08-22 03:40:32 +02:00
Hans Goudey reviewed 2023-08-23 15:40:39 +02:00
@ -23,6 +26,15 @@
namespace blender::ed::curves {
std::optional<Bounds<float3>> selection_bounds(
Member

Since this is only really tangentially related to selection, and it's probably not used elsewhere, it might be better if this function was inlined into the view selected function. It's only 4 lines anyway.

Since this is only really tangentially related to selection, and it's probably not used elsewhere, it might be better if this function was inlined into the view selected function. It's only 4 lines anyway.
Author
Member

One reason would be to be able to reuse this for new grease pencil.
Other one is just size of already exist view selected function...

One reason would be to be able to reuse this for new grease pencil. Other one is just size of already exist view selected function...
Member

I think these operations (retrieve_selected_points, bounds::min_max) are high level enough that combining them won't be that helpful for that case. For GP the arguments would be different anyway.

I think these operations (`retrieve_selected_points`, `bounds::min_max`) are high level enough that combining them won't be that helpful for that case. For GP the arguments would be different anyway.
mod_moder marked this conversation as resolved
Iliya Katushenock added 3 commits 2023-09-19 21:12:17 +02:00
Hans Goudey approved these changes 2023-09-20 19:18:21 +02:00
Hans Goudey left a comment
Member

I'll test this before committing, but the code looks good to me now, thanks.

I'll test this before committing, but the code looks good to me now, thanks.
Hans Goudey added 1 commit 2023-11-03 09:41:56 +01:00
Hans Goudey merged commit 3d971feb36 into main 2023-11-03 09:43:05 +01:00
Iliya Katushenock deleted branch tmp_frame_on_selection 2023-11-03 19:04:34 +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 Assignees
2 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#109924
No description provided.