Sculpt: Disable sculpt brushes on invisible objects #117746

Merged
Brecht Van Lommel merged 8 commits from Sean-Kim/blender:112371-invisible-sculpt into blender-v4.1-release 2024-02-08 15:06:26 +01:00
Member

This PR makes the following changes to sculpt mode:

  • Brushes no longer affect invisible objects or objects that are entirely contained by hidden collections
  • The brush cursor no longer highlights vertices of invisible objects and the cursor circle remains view-aligned.

invisible_sculpt_v2.gif

Testing

  • Manual testing on attached .blend file

Limitations

  • No testing performed on complex / non-trivial .blend files, so I'm unsure if the performance of this is fine for actual usage.

Addresses #112371

This PR makes the following changes to sculpt mode: * Brushes no longer affect invisible objects or objects that are entirely contained by hidden collections * The brush cursor no longer highlights vertices of invisible objects and the cursor circle remains view-aligned. ![invisible_sculpt_v2.gif](/attachments/ad01e004-b7c9-438e-b888-b130622a1159) ## Testing * Manual testing on attached `.blend` file ## Limitations * No testing performed on complex / non-trivial .blend files, so I'm unsure if the performance of this is fine for actual usage. Addresses #112371
Sean Kim added 1 commit 2024-02-02 08:29:19 +01:00
Objects that are themselves marked as hidden in a viewport
or belonging to a container that is hidden will no longer
be able to be edited by sculpt brushes.

Ref \#112371
Sean Kim requested review from Julien Kaspar 2024-02-02 08:30:51 +01:00
Author
Member

Only adding Julien for now, since I'm unsure of the performance impacts of this change in actual usage; if it seems satisfactory I'll look for further reviewers afterwards.

Only adding Julien for now, since I'm unsure of the performance impacts of this change in actual usage; if it seems satisfactory I'll look for further reviewers afterwards.
Contributor

Hm, Juliens original proposal made sense, if object is hidden brushes shouldn't go over it, but accounting for ANY collection visibility is bit strange, because I might have object in separate collections for light linking, compositing, instances for geonodes and etc, which are hidden in the viewport, and I'll have to unhide them to sculpt on object.

Collections are very widely used in all parts of Blender besides just scene organization, and I think we should account for that. Some collections are made to be always hidden and it shouldn't get in the way.

Hm, Juliens original proposal made sense, if object is hidden brushes shouldn't go over it, but accounting for ANY collection visibility is bit strange, because I might have object in separate collections for light linking, compositing, instances for geonodes and etc, which are hidden in the viewport, and I'll have to unhide them to sculpt on object. Collections are very widely used in all parts of Blender besides just scene organization, and I think we should account for that. Some collections are made to be always hidden and it shouldn't get in the way.
Author
Member

@nickberckley I think I may have misunderstood a comment in the original issue then; I took

Yes. No operations should be allowed on hidden objects (and objects in hidden collections)

To mean that if an object was in any hidden collection at all that it should be filtered out, though I can see why that would be undesirable behavior from a workflow perspective. I'll wait for further clarifying comments and adjust the PR later when I wake up.

@nickberckley I think I may have misunderstood a comment in the original issue then; I took > Yes. No operations should be allowed on hidden objects (and objects in hidden collections) To mean that if an object was in any hidden collection at all that it should be filtered out, though I can see why that would be undesirable behavior from a workflow perspective. I'll wait for further clarifying comments and adjust the PR later when I wake up.
Member

@Sean-Kim I would rephrase it then as: "If the object or all collections that the object is linked to are hidden".

Any case where the object is no longer visible in the viewport and therefore should not be editable.

Is it possible to have the brush cursor no longer read the hidden geometry? So the cursor circles would always be view aligned and the target vertex would not be highlighted?

@Sean-Kim I would rephrase it then as: "If the object or **all** collections that the object is linked to are hidden". Any case where the object is no longer visible in the viewport and therefore should not be editable. Is it possible to have the brush cursor no longer read the hidden geometry? So the cursor circles would always be view aligned and the target vertex would not be highlighted?

There is generic code for checking effective object visibility in a viewport, it should not be needed to invent anything new for sculpt mode.

For other modes there are iterators like BKE_view_layer_bases_in_mode_iterator_next that loop over all visible object in the mode. Since sculpt mode does not yet support multi object editing, probably it should call BKE_base_is_visible same as that iterator does.

There is generic code for checking effective object visibility in a viewport, it should not be needed to invent anything new for sculpt mode. For other modes there are iterators like `BKE_view_layer_bases_in_mode_iterator_next` that loop over all visible object in the mode. Since sculpt mode does not yet support multi object editing, probably it should call `BKE_base_is_visible` same as that iterator does.
Sean Kim changed title from Sculpt: Disable sculpting on invisible objects to WIP: Sculpt: Disable sculpting on invisible objects 2024-02-02 23:04:56 +01:00
Author
Member

Marking this as WIP for now since there's a fair amount of feedback to address.

@JulienKaspar - Makes sense, I'll keep that in mind for how the feature is intended to work. I'll investigate the cursor circle and target vertex as well

@brecht - Thanks for the tip about the methods, completely missed them when searching for something appropriate to use. I saw the comment in the existing code about #BKE_object_is_visible_in_viewport not working for sculpt and assumed that was the canonical method to use in this situation.

Marking this as WIP for now since there's a fair amount of feedback to address. @JulienKaspar - Makes sense, I'll keep that in mind for how the feature is intended to work. I'll investigate the cursor circle and target vertex as well @brecht - Thanks for the tip about the methods, completely missed them when searching for something appropriate to use. I saw the comment in the existing code about `#BKE_object_is_visible_in_viewport` not working for sculpt and assumed that was the canonical method to use in this situation.
Sean Kim added 2 commits 2024-02-03 01:20:08 +01:00
Sean Kim changed title from WIP: Sculpt: Disable sculpting on invisible objects to Sculpt: Disable sculpting on invisible objects 2024-02-03 01:31:20 +01:00
Sean Kim added 1 commit 2024-02-03 01:39:13 +01:00
Author
Member

Ok, addressed changes so far and updated the description with a new GIF, I think the PR is now in a state where the functionality can be tested @JulienKaspar

Ok, addressed changes so far and updated the description with a new GIF, I think the PR is now in a state where the functionality can be tested @JulienKaspar
Julien Kaspar approved these changes 2024-02-05 10:28:42 +01:00
Julien Kaspar left a comment
Member

Works like intended now. So it's all good on the user side.

@brecht do you mind if I add you as a reviewer since you already took a look at the PR?

Works like intended now. So it's all good on the user side. @brecht do you mind if I add you as a reviewer since you already took a look at the PR?
Julien Kaspar requested review from Brecht Van Lommel 2024-02-05 10:28:51 +01:00
Brecht Van Lommel requested changes 2024-02-05 12:43:30 +01:00
@ -1413,0 +1422,4 @@
ViewLayer *view_layer = CTX_data_view_layer(C);
BKE_view_layer_synced_ensure(scene, view_layer);
const Base *base = BKE_view_layer_base_find(view_layer, ob);

Use CTX_data_active_base(C) which does the same thing.

Use `CTX_data_active_base(C)` which does the same thing.
Sean-Kim marked this conversation as resolved
@ -5636,0 +5639,4 @@
ViewLayer *view_layer = CTX_data_view_layer(C);
BKE_view_layer_synced_ensure(scene, view_layer);
const Base *base = BKE_view_layer_base_find(view_layer, ob);

Use CTX_data_active_base(C) which does the same thing.

Use `CTX_data_active_base(C)` which does the same thing.
Sean-Kim marked this conversation as resolved
Sean Kim added 1 commit 2024-02-05 23:23:57 +01:00
Sean Kim requested review from Brecht Van Lommel 2024-02-05 23:24:20 +01:00
Brecht Van Lommel requested changes 2024-02-06 10:25:28 +01:00
@ -1413,0 +1415,4 @@
/* Ideally, this code would go inside SCULPT_cursor_geometry_info_update when we perform a
* raycast check, however the method is used by many different editors across the entirety of
* sculpt. To mitigate potential unwanted changes of hiding the cursor, the logic is extracted
* here. */

I think this should just be in SCULPT_cursor_geometry_info_update, not sure why it shouldn't be.

With this only the cursor drawing is changed, but the drawing and actual behavior should be consistent.

I think this should just be in `SCULPT_cursor_geometry_info_update`, not sure why it shouldn't be. With this only the cursor drawing is changed, but the drawing and actual behavior should be consistent.
Author
Member

Admittedly, I cut the investigation of moving this into SCULPT_cursor_geometry_info_update rather short after seeing it referenced from the SCULPT_OT_sample_detail_size and SCULPT_OT_cloth_filter operators among others.

I was planning on adding the check here but the subsequent modification of some SculptSession variables (active_vertex and active_face_index / active_grid_index) made me hesitant to introduce the changes there out of fear of breaking other operators that may have been dependent on them.

I'll do some further investigation later today to see if it actually has any impact or if this change can be moved there, because I do agree that it's currently in a hacky spot.

Admittedly, I cut the investigation of moving this into `SCULPT_cursor_geometry_info_update` rather short after seeing it referenced from the `SCULPT_OT_sample_detail_size` and `SCULPT_OT_cloth_filter` operators among others. I was planning on adding the check [here](https://projects.blender.org/blender/blender/src/commit/289d7fa7d200d2fa008b9a4a848fdee06448efef/source/blender/editors/sculpt_paint/sculpt.cc#L4932-L4937) but the subsequent modification of some `SculptSession` variables (`active_vertex` and `active_face_index` / `active_grid_index`) made me hesitant to introduce the changes there out of fear of breaking other operators that may have been dependent on them. I'll do some further investigation later today to see if it actually has any impact or if this change can be moved there, because I do agree that it's currently in a hacky spot.
Author
Member

Made the change, manually tested the various operators that call into the method and didn't find any crashes for them when operating on invisible geometry, though they do still perform the corresponding action.

Made the change, manually tested the various operators that call into the method and didn't find any crashes for them when operating on invisible geometry, though they do still perform the corresponding action.
Sean-Kim marked this conversation as resolved
Sean Kim added 1 commit 2024-02-07 01:49:35 +01:00
Sean Kim changed title from Sculpt: Disable sculpting on invisible objects to Sculpt: Disable sculpt brushes on invisible objects 2024-02-07 01:51:56 +01:00
Author
Member

Edited the title of the PR slightly to reflect some findings after testing the most recent change, I'll put this update in #112371, but the summary of it is that the following operators currently still operate on hidden geometry. I think it makes more sense to address those in other PRs instead of potentially cluttering this up with a lot more changes:

  • SCULPT_OT_expand
  • SCULPT_OT_trim_box_gesture
  • SCULPT_OT_trim_lasso_gesture
  • SCULPT_OT_sample_detail_size
  • MESH_OT_face_set_extract
  • SCULPT_OT_mesh_filter
  • SCULPT_OT_cloth_filter
  • SCULPT_OT_color_filter
Edited the title of the PR slightly to reflect some findings after testing the most recent change, I'll put this update in #112371, but the summary of it is that the following operators currently still operate on hidden geometry. I think it makes more sense to address those in other PRs instead of potentially cluttering this up with a lot more changes: * `SCULPT_OT_expand` * `SCULPT_OT_trim_box_gesture` * `SCULPT_OT_trim_lasso_gesture` * `SCULPT_OT_sample_detail_size` * `MESH_OT_face_set_extract` * `SCULPT_OT_mesh_filter` * `SCULPT_OT_cloth_filter` * `SCULPT_OT_color_filter`
Brecht Van Lommel requested changes 2024-02-07 12:46:30 +01:00
@ -4932,1 +4933,3 @@
if (!srd.hit) {
const View3D *v3d = CTX_wm_view3d(C);
const Base *base = CTX_data_active_base(C);
if (!srd.hit || !BKE_base_is_visible(v3d, base)) {

I think this check should be done earlier, at if (!ss->pbvh || !vc.rv3d)?

No point doing a raycast on an invisible object.

I think this check should be done earlier, at `if (!ss->pbvh || !vc.rv3d)`? No point doing a raycast on an invisible object.
Author
Member

Oh true, will move this there.

Oh true, will move this there.
Sean-Kim marked this conversation as resolved
Brecht Van Lommel changed target branch from main to blender-v4.1-release 2024-02-07 12:48:36 +01:00
Sean Kim added 2 commits 2024-02-08 00:41:17 +01:00
Sean Kim requested review from Brecht Van Lommel 2024-02-08 00:42:05 +01:00
Brecht Van Lommel approved these changes 2024-02-08 00:59:43 +01:00
Brecht Van Lommel merged commit 4df5bcfea9 into blender-v4.1-release 2024-02-08 15:06:26 +01:00
Brecht Van Lommel deleted branch 112371-invisible-sculpt 2024-02-08 15:06:28 +01:00
Sign in to join this conversation.
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 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#117746
No description provided.