Details

Reviewers

Julien Kaspar (JulienKaspar)
Sergey Sharybin (sergey)
Campbell Barton (campbellbarton)

Commits

rB438bd823714a: Sculpt: Pose Brush option to affect loose parts

Summary

This option allows posing meshes with different disconnected elements
using the Pose Brush.

This is achieved by doing the following:

Creating an ID per vertex that stores the loose part of that vertex.
By using those IDs, one fake topology connection is created per vertex to the nearest vertex in a different ID. The maximum distance to create that connection is determined by the "Max Element Distance" property. These fake connectivity neighbors are used in the Sculpt API functions iterators, so all the algorithms of the Pose Brush can run without modifications as if everything was part of the same mesh.

Diff Detail

Repository

rB Blender

Branch

sculpt-fake-neighbors (branched from master)

Build Status

Buildable 8500
Build 8500: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) created this revision.Mar 31 2020, 12:28 AM

Harbormaster completed remote builds in B7371: Diff 23278.Mar 31 2020, 12:28 AM

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Mar 31 2020, 12:31 AM

Pablo Dobarro (pablodp606) added a reviewer: Julien Kaspar (JulienKaspar).

Pablo Dobarro (pablodp606) added a project: BF Blender (2.90).

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)

Jun Mizutani (jmztn) awarded a token.Mar 31 2020, 10:39 AM

Kenn Nyström (Frozen_Death_Knight) awarded a token.Mar 31 2020, 11:00 AM

Julien Kaspar (JulienKaspar) awarded a token.Mar 31 2020, 11:59 AM

Pablo Dobarro (pablodp606) mentioned this in D7471: Sculpt: Vertex Info management.Apr 19 2020, 10:43 PM

Rebase

Harbormaster completed remote builds in B8436: Diff 25644.Jun 8 2020, 6:56 PM

Pablo Dobarro (pablodp606) mentioned this in T63745: Sculpt, Paint, Texture Module.Jun 8 2020, 7:53 PM

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Jun 9 2020, 11:27 PM

Redesign how fake neighbors work

Now it should be much faster, take less memory and it should be usable from other tools.
Also, the pose brush preview now works taking into account the disconnected parts

Harbormaster completed remote builds in B8480: Diff 25721.Jun 10 2020, 12:35 AM

Remove unused includes

Harbormaster completed remote builds in B8482: Diff 25723.Jun 10 2020, 12:37 AM

Pablo Dobarro (pablodp606) added a reviewer: Sergey Sharybin (sergey).Jun 10 2020, 2:13 AM

Nabil Adel (mywa880) awarded a token.Jun 10 2020, 4:57 AM

Quite cool. To keep stream of cool ideas: imagine this but with separate object in multi-sculpt mode ;)

The vertex distance based approach sounds a bit weak, but might be fine for real-world sculpt modes. Curious whats' Julien's experience using this tool is.

From the code perspective, see the inlined comment, but also:

"Topology ID" is too vague. I'm guess from the brief explanation and digging through non-commented code is that this is "island" in regular terms. In a mathematical terms its called "connected component".
Implication from the previous point: meaning, behavior, and implementation details deserved more commenting. Think of a future you coming back to the code, or another developer coming into the area: help them to understand what's what without them needing to scroll through the code and guess what is going on.
Since you mention there is possibility to support more brushes for this feature, don't make DNA/RNA pose-centric. Make it more generic.
The minimal distance seems to be defaulting to 0. While there is no good default which will work for all models, 0 is bad to the degree that it's guaranteed to not do anything unless you tweak the setting (which hurts discoverability of setting is actually doing).
It seems that the distance is in the world space. Don't think this is very useful or convenient. Tweaking settings for a specific character you're sculpting on sounds like an unnecessary and avoidable: you can expose this distance as relative to the object's bounding box. This way, for example, same presets will give good results when sculpting on both Spring and Alpha.

source/blender/blenkernel/BKE_paint.h
395	/* Fake neighbors. Used to ... */
397	Is it requested maximal distance or maximal distance for which the neighburhood is calculated for ?
398–399	For both of them: Indexed by <FOO>, points to <BAR>. To make it clear meaning of index and elements. Also taking bigger picture into account and something we've discussed prior to 2.82: the `SculptSession` is to be split in a way that it contains properly named (and typed) structures. For example, here you can have: /* Fake neighbours are used to allow sculpting on mesh with disconnected islands. .... Based on topology ID ... Topology ID is ... . / struct SculptFakeNeighbours { / Max distance used to calculate neighbourhood information. / float current_max_distance; / Indexed by vertex index. / int vertex_topology_id; }; struct SculptSession { ... SculptFakeNeighbours fake_neighbours; ... }
source/blender/editors/sculpt_paint/sculpt.c
7908–7909	Use anonymous enumerator.

This revision now requires changes to proceed.Jun 10 2020, 10:19 AM

Review Update

Harbormaster completed remote builds in B8500: Diff 25776.Jun 11 2020, 1:25 AM

Making this work across multiple objects would be ideal. In theory, if the sculpt API is implemented to expose the data from multiple meshes as a single mesh it should work. I don't know how much work this would take, but if you think that can be done in a reasonable amount of time we should consider it when refactoring the SculptSession data for 2.90
I renamed the topology ID to connected components and it is now stored in the SculptVertexInfo struct. This way it can be reused and shared from other tools, avoiding the initialization lag. I did an attempt to convert everything to this system in D7471, but it probably makes more sense to start making the change step by step as it is needed
I tried to make the distance a factor of the bounding box volume and the brush radius, but I think it makes it more unpredictable. The tool depends on how far disconnected parts are and how is the topology distributed and not that much in the size of the model, so depending on the area you are working on you need to adjust the value anyway (like going from posing the fingers to posing the entire arm). Probably leaving it like this for now is better as people are used to merge by distance, and this is a similar concept, but we can revisit this decision after seeing how it works with this T77738

I don't know how much work this would take, but if you think that can be done in a reasonable amount of time we should consider it when refactoring the SculptSession data for 2.90

Think it is quite involved change, which isn't so much a stopper for refactoring underlying foundations. Wouldn't prioritize this. Would make it easier to untangle all the sculpt session/undo early on. After that we can re-iterate to commit to a bigger involved change which will be related on multi-object sculpt.

I tried to make the distance a factor of the bounding box volume and the brush radius, but I think it makes it more unpredictable.

This is interesting. Can you make it so Julien can play around with this patch? (only if buildbot machine didn't decide to die :()

As for the patch itself, there is some last bit of feedback. Otherwise code side seems fine.
But I'd like to have feedback from Julien whether world-space setting works fine for him as well.

release/scripts/startup/bl_ui/properties_paint_common.py
629	The convention is to use `use_` prefix for boolean properties.Exceptions are `show`, `hide` and such. Not sure `use_affect_disconnected` is a good English though. Please check with Bastien or Campbell.
630	`max_disconnected_distance` maybe? To make it clear it is related to the `{use}_affect_disconnected`.
source/blender/editors/sculpt_paint/sculpt.c
7910	I would add some simplified general flow example. Something like: /* The first neighbors first need to be ensured to be initialized. * After that tools which needs fake neighbors functionality need to * temporarily enable it: * * void my_awesome_sculpt_tool() { * SCULPT_fake_neighbors_ensure(sd, object, brush->max_element_distance); * SCULPT_fake_neighbors_enable(ob); * * ... Logic of the tool ... * SCULPT_fake_neighbors_disable(ob); * } * * Such approach allows to keep all the connectivity information ready for reuse * (withouy having lag prior to every stroke), but also makes it so the affect * is localized to a specific brushes and tools only. */

Julian Perez (julperado) awarded a token.Jun 12 2020, 4:48 AM

Review Update

Harbormaster completed remote builds in B8625: Diff 26078.Jun 18 2020, 7:44 PM

I renamed the property to use_affect_disconnected. @Campbell Barton (campbellbarton) @Bastien Montagne (mont29) Do you think that naming is ok? Maybe just use_disconnected could also work

HEYPictures (HEYPictures) awarded a token.Jun 19 2020, 10:00 AM

HEYPictures (HEYPictures) rescinded a token.

Other than RNA naming seems fine.

P.S. Maybe use blender-chat to get more instant feedback?

This revision is now accepted and ready to land.Jun 19 2020, 11:13 AM

serge (rpserge) awarded a token.Jun 19 2020, 1:24 PM

I've tested the feature for a bit and it seems to work fine most of the time. With clothing pieces that are close to each other this makes it far easier to pose characters.
The only odd thing I kept noticing is that the pivot point becomes increasingly unpredictable with a higher "Max Element Distance".

I'll keep experimenting with it but luckily when it fails I can fall back on the Transform Tools to do the task instead but slower.

Campbell Barton (campbellbarton) added inline comments.Jun 23 2020, 5:24 AM

source/blender/makesrna/intern/rna_brush.c
2315	`_min/_max` are normally a suffix.
2618	As we have `use_proportional_connected` elsewhere, why not match this naming? `use_{pose}_connected` and flip the boolean with `RNA_def_property_boolean_negative_sdna`. `pose` could be something else, or just `use_connected` if this is intended to be implemented for the general cases. Then we can call this the same name as we do for proportional editing "Connected Only" in the UI.