Page MenuHome

Merge By Distance: Optimize algorithm to find duplicate polygons
ClosedPublic

Authored by Germano Cavalcante (mano-wii) on Fri, Jan 20, 8:04 PM.
Tags
None
Subscribers
None
Tokens
"Love" token, awarded by Draise."Love" token, awarded by Bit."Love" token, awarded by gilberto_rodrigues.

Details

Summary

In an attempt to replace BKE_mesh_merge_verts (see D16755) a loss in
performance of around 20% (worst case) was observed.

The most time-consuming operation in merge by distance is to find
duplicate faces (faces that are different but have the same vertices).

Therefore, some strategies were planned to optimize this algorithm:

  • Store the corner indices in an array thus avoiding multiple calls of weld_iter_loop_of_poly_next;
  • Create a map of polygons linked to edges instead of linked to vertices - this decreases the number of connections and reduces the calculation of the intersection of polygon indices.

There are other fields to optimize, like reusing the wpolys array
instead of creating a new array of corner offsets. And join some arrays
as members of the same struct to be used in the same buffer.
But for now, it is already a nice optimization. And the new
poly_find_doubles function can be reused in the future to create a
generic utility.

The result of the optimization varies greatly depending on the number
of polygons, the size of each polygon and the number of duplicates.
On average it was something around 2 times faster.

Worst case tested (old vs new): 0.1ms vs 0.3ms
Best case tested (old vs new): 10.0ms vs 3.2ms

Diff Detail

Repository
rB Blender

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Fri, Jan 20, 8:04 PM
Germano Cavalcante (mano-wii) created this revision.

I don't fully understand this (that's okay I think) but it looks like quite an improvement in readability/simplicity. I appreciate the use of basic types like int and the use of the OffsetIndices class.

One thing that might help me as a reader is finding a word besides "link" in variable names. I find it a but hard to understand what that means sometimes. Also offset instead of offs would be nice :)

source/blender/geometry/intern/mesh_merge_by_distance.cc
467

Since MVert isn't used anymore, maybe verts_num would make more sense. There's a task describing how the _num suffix is meant to be the standard for such variables but I can't find it right now :/

1258–1259

How about this? Seems much simpler, and doesn't have to deal with raw pointers

1316–1319

Without std::move these are probably resulting in copies (though maybe this whole function is inlined)

1374–1375

Looks like this could be done a bit more simply by using a helper OffsetIndices object above after the vector has been filled.

1380

Using a more standard for loop with just one changing variable would make these much more readable in my opinion. It's hard to track what's going on here.

Same with the for (; link_len_first--; polys_to_test++) { above.

This revision is now accepted and ready to land.Fri, Jan 20, 8:22 PM
Germano Cavalcante (mano-wii) marked 5 inline comments as done.
  • Rename _len to _num and offs to offset
  • Use std::move
  • Simplify loop to read offsets
  • Rework intersect algorith

Performance remained the same.

source/blender/geometry/intern/mesh_merge_by_distance.cc
1258–1259

Not sure if it would work. The (*buffer > index) condition returns false and serves to break the loop early.

But I submitted another solution which is easier to read.

source/blender/geometry/intern/mesh_merge_by_distance.cc
1258–1259

Why wouldn't that work? Any reasonable implementation of std::any_of would stop when a false condition was found.
Or are you referring to correctness rather than performance? The new intersect function seems more complicated than either of these.
It's also not clear what "intersecting" means in this context, or what the "buffers" refer to. There are probably more meaningful names?

I would really suggest avoiding falling back to raw pointers here when we have Span.

source/blender/geometry/intern/mesh_merge_by_distance.cc
1258–1259

Agh, I wrote that wrong. Looks like the >= should have been reversed or !std::all_of could be used instead. Still seems nice to use those logic building blocks IMO

  • Add comments describing intersection further.
  • Avoid raw pointers by using Span