Page MenuHome

Sculpt: Pose Brush Affect Loose Parts property
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Mar 31 2020, 12:28 AM.
Tokens
"Love" token, awarded by Geronimooo."Party Time" token, awarded by ruthwikrao."Love" token, awarded by Bit."Like" token, awarded by TheCharacterhero."Love" token, awarded by Serva."The World Burns" token, awarded by CobraA."Like" token, awarded by cfnjrey."Party Time" token, awarded by rpserge."Love" token, awarded by julperado."100" token, awarded by mywa880."Love" token, awarded by JulienKaspar."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by jmztn.

Details

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

Event Timeline

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
  • 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

  • Remove unused includes
Sergey Sharybin (sergey) requested changes to this revision.Jun 10 2020, 10:19 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
399

/* Fake neighbors.

  • Used to ... */
401

Is it requested maximal distance or maximal distance for which the neighburhood is calculated for ?

402–403

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
7998–7999

Use anonymous enumerator.

This revision now requires changes to proceed.Jun 10 2020, 10:19 AM
Pablo Dobarro (pablodp606) marked 4 inline comments as done.
  • Review Update
  • 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
639

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.

640

max_disconnected_distance maybe? To make it clear it is related to the {use}_affect_disconnected.

source/blender/editors/sculpt_paint/sculpt.c
8000

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. */
Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review Update

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

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.Fri, Jun 19, 11:13 AM

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.

source/blender/makesrna/intern/rna_brush.c
2365

_min/_max are normally a suffix.

2668

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.

Pablo Dobarro (pablodp606) marked 2 inline comments as done.
  • Review Update