Page MenuHome

Data Transfer modifier's Max Distance field working strangely
Open, Confirmed, MediumPublic

Description

Using Blender 2.89b. EVGA GTX1070. Win10. All non-default add-ons disabled and new scene loaded.

Attached demonstration file contains a cube using a data transfer with max distance enabled to copy weights from a nearby plane, and an armature for that cube. Max Distance settings should put four verts in the domain of the modifier; as you can see, seven verts are affected.

Problem encountered in less abstract situations as well, this is just my simplest reduction of the problem in order to verify that it's an issue and make it plain to see and reproduce.

For reference, official documentation reads, "This allows to transfer a small sub-detailed mesh onto a more complete one (e.g. from a “hand” mesh towards a “full body” one)," which would be useful if it were working properly.

Event Timeline

Philipp Oeser (lichtwerk) triaged this task as Confirmed, Medium priority.

Confirming on first sight, having a closer look now...

Philipp Oeser (lichtwerk) claimed this task.

I'll claim for the time being and do some further investiagtion. If that doesnt succeed, I'll get @Bastien Montagne (mont29) on board to help me out...

Coming back to add that there's a reasonable workaround, which is to use a vertex weight proximity modifier to determine the distance from each vertex before the weight transfer, use a custom (constant) curve to modulate that vertex group, and then use it to modulate the data transfer. It's a bit more work for the end user, but perfectly reasonable, and it wouldn't seem unreasonable to just drop support for max distance from the data transfer.

Had a look at this again and here are some findings:

  • BKE_mesh_remap_calc_verts_from_mesh / mesh_remap_bvhtree_query_nearest is where things happen
  • in this loop, code keeps the previous "nearest" [but doesnt reset nearest.index to -1]
  • if BLI_bvhtree_find_nearest() is called with a "nearest" provided, code will not update anything on "nearest" (esp. not index, not dist_sq) if it fails to find something.
  • so the comment here is actually misleading I think, index is not neccessarily -1 if it fails to find something.
  • if we have previously found something [making nearest.index non-zero], and then for the next vertex dont find anything, we still have a non-zero index leading to false positives in the following code.

It actually works fine if the index is reset to -1 for each vert.
Could be like this (would have to be done for a couple of other loops as well):

1
2
3diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c
4index db158ca8fb2..c1dd62d2719 100644
5--- a/source/blender/blenkernel/intern/mesh_remap.c
6+++ b/source/blender/blenkernel/intern/mesh_remap.c
7@@ -526,9 +526,12 @@ void BKE_mesh_remap_calc_verts_from_mesh(const int mode,
8
9 if (mode == MREMAP_MODE_VERT_NEAREST) {
10 BKE_bvhtree_from_mesh_get(&treedata, me_src, BVHTREE_FROM_VERTS, 2);
11- nearest.index = -1;
12+ //nearest.index = -1;
13
14 for (i = 0; i < numverts_dst; i++) {
15+ /* resetting index here would work */
16+ nearest.index = -1;
17+
18 copy_v3_v3(tmp_co, verts_dst[i].co);
19
20 /* Convert the vertex to tree coordinates, if needed. */

Or more general like:
1
2
3diff --git a/source/blender/blenkernel/intern/mesh_remap.c b/source/blender/blenkernel/intern/mesh_remap.c
4index db158ca8fb2..fb4ce68fed3 100644
5--- a/source/blender/blenkernel/intern/mesh_remap.c
6+++ b/source/blender/blenkernel/intern/mesh_remap.c
7@@ -67,6 +67,7 @@ static bool mesh_remap_bvhtree_query_nearest(BVHTreeFromMesh *treedata,
8 nearest->dist_sq = max_dist_sq;
9 }
10 /* Compute and store result. If invalid (-1 index), keep FLT_MAX dist. */
11+ nearest->index = -1;
12 BLI_bvhtree_find_nearest(treedata->tree, co, nearest, treedata->nearest_callback, treedata);
13
14 if ((nearest->index != -1) && (nearest->dist_sq <= max_dist_sq)) {

@Alexander Gavrilov (angavrilov), @Bastien Montagne (mont29): what do you think?

@Philipp Oeser (lichtwerk) nice catch. Wouldn’t it be even simpler to always reset that nearest->index directly in BLI_bvhtree_find_nearest_ex()? Keeping previous nearest distance there if provided makes sense, but I think item index should be always reset, at least I cannot see a case where keeping previous value would be useful?

I went over usages of BLI_bvhtree_find_nearest / BLI_bvhtree_find_nearest_projected and here are some findings:

these are the ones that always set item index to -1 before calling, it looks like an index of -1 ALWAYS should be OK:

  • dynamic_paint_paint_mesh_cell_point_cb_ex (called in a loop, item index set to -1 prior every time)
  • BKE_bmbvh_find_vert_closest (called once, item index set to -1 prior)
  • BKE_bmbvh_find_face_closest (called once, item index set to -1 prior)
  • closest_point_on_surface (called once, item index set to -1 prior)
  • obstacles_from_mesh_task_cb (smoke code, called in a loop, item index set to -1 prior every time)
  • sample_mesh (smoke code, called once, item index set to -1 prior)
  • remap_hair_emitter (called in a loop, item index set to -1 prior every time)
  • rna_Object_closest_point_on_mesh (called once, item index set to -1 prior)
  • nearestVert (surface deform modifier, called once, item index set to -1 prior)
  • vert2geom_task_cb_ex (weight proximity modifier, called once, item index set to -1 prior every time)
  • py_bvhtree_find_nearest (called once, item index set to -1 prior)
  • find_nearest_points_test (called with NULL)

snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 ALWAYS would be OK? maybe @Germano Cavalcante (mano-wii) knows more?:

  • snapMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)
  • snapEditMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)

shrinkwrap: not 100% sure, but it looks like an index of -1 ALWAYS should be OK

  • shrinkwrap_calc_nearest_vertex_cb_ex (resets index to -1, but in ParallelRangeSettings, see shrinkwrap_calc_nearest_surface_point, not 100% sure...)
  • BKE_shrinkwrap_find_nearest_surface (does not reset index to -1, needs checking, called from shrinkwrap_get_tarmat [where index is set to -1] / shrinkwrap_calc_nearest_vertex_cb_ex [see above])

our candidate for this report:), looks like an index of -1 ALWAYS should be OK

  • mesh_remap_bvhtree_query_nearest (does not reset index to -1 atm., results in bug)

Will also subscribe @Brecht Van Lommel (brecht) as lead architect here, but if we want to have this in for RC1, propose to actually play safe and go with D5206?

Keeping the previous search result when doing multiple nearest searches is a very important heuristic optimization. Not doing that in shrinkwrap for instance will absolutely butcher performance.

This heuristic however obviously relies on the previous search result being a valid candidate for the new search (i.e. it can be reused as is if it's still the nearest point).

More specifically, BVH nearest search code by default doesn't bother to ensure an optimal search order through the tree, because using the previous search result to prime the search and prune the tree works out to be faster. When implementing the new more complicated shrinkwrap mode that can't use this heuristic, I had to add BLI_bvhtree_find_nearest_ex and BVH_NEAREST_OPTIMAL_ORDER to get good performance.

Perhaps BVH_NEAREST_OPTIMAL_ORDER should be the default behavior instead (and also ignore the previous result), while the heuristic behavior should be invoked through BVH_NEAREST_REUSE_RESULT or something like that. I did it the way it is currently to avoid changing old code all over the place.

@Alexander Gavrilov (angavrilov) : thx for the heads up, will check on this (might take a bit to go over all cases), for the "quick fix": D5206 should still be fine, right?

(...)
snapping: these are the ones that set item index to -1 before calling but then could enter multiple times (vert, edge, ...), not sure if an index of -1 ALWAYS would be OK? maybe @Germano Cavalcante (mano-wii) knows more?:

  • snapMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)
  • snapEditMesh (item index set to -1 prior, but multiple usages here which could all be entered?... not sure)

(...)

These are cases where changing the nearest.index to -1 between calls would result in problems.