Page MenuHome

Fix T93084: Area stretch overlay full red on large scale mesh
AbandonedPublic

Authored by Philipp Oeser (lichtwerk) on Aug 29 2022, 10:18 AM.

Details

Summary

The current logic goes as follows:
[1] get each faces 3d area
[2] get each faces uv area
[3] also keep track of the current total area (both 3d and uv)
[4] divide the uv area by the 3d area and store as a ratio
[5] push totals and this ratio to the GPU, and multiply by
(total_uv_area / total_3d_area)

Issue arises when face areas are really large combined with small UV
areas (report has a mesh ~1.5 km), then floating point point imprecission
may lead to ratios of 0.0f (which then messes up the overlay display).

Just flipping the logic to a multiplication in step [4] would solve the
issue for large scale meshes, but could have similar issues for very
small scale meshes.
Using doubles might solve this as well (havent tried this though, unsure
pushing these to the GPU would be an issue).
Using the uv_parametrizer might also solve this (afaict this also uses
doubles internally) but also havent checked this.

So the idea now is as follows:
[1] get each faces 3d area
[2] get each faces uv area
[3] also keep track of the current total area (both 3d and uv)
[4] get a fraction of each faces 3d area in relation the the total 3d
area (making this "independent" of mesh dimensions)
[5] get a fraction of each faces uv area in relation the the total uv
area
[6] divide those two to get the stretch directly on the CPU and only
push this to the GPU

This way we avoid running into floating point precission issues (these
very small values in the former ratios).

Doing it this way will also remove the need to update totals and push
these constants to the GPU in a couple of places (getting rid of quite a
bit of code).

NOTE: we do need to loop over faces twice on the CPU but in my benchmarks this only resulted in a very small overall performance hit. The overlay was ~3% slower when editing UVs on a 1million poly mesh. If this (tiny) performance hit is of concern, another thing to try would be pushing arrays for face area AND uvarea (alongside the totals) to the GPU and do the calculation in the shader (but havent tested performance on this yet)

Diff Detail

Repository
rB Blender
Branch
T93084 (branched from master)
Build Status
Buildable 23519
Build 23519: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Aug 29 2022, 10:18 AM
Philipp Oeser (lichtwerk) created this revision.
Brecht Van Lommel (brecht) requested changes to this revision.Aug 29 2022, 2:27 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/draw/intern/mesh_extractors/extract_mesh_vbo_edituv_stretch_area.cc
80

Remove mp++, it's wrong and unnecessary.

This code can also be deduplicated and shared with the bmesh case.

83

Since this previously went through the effort of being GPU accelerated, maybe worth optimizing it:

const float a = poly_area[mp_index] * tot_uv_area;
const float b = uv_area[mp_index] * tot_area;
const float min_ab = min_ff(a, b);
const float max_ab = max_ff(a, b);
const float stretch = (max_ab >= 0.0f) ? min_ab / max_ab: 0.0f;

Not sure why FLT_EPSILON was there before, but in general it's wrong to use it like that on absolute quantities instead of relative ones.

This revision now requires changes to proceed.Aug 29 2022, 2:27 PM

Making it GPU enabled was an effort to reduce unneeded data transfers. Looking at previous patches makes me wonder if it could it also be solved by uploading floats in stead of GPU_COMP_I16? Have you tried this in one of your attempts.

address review comments:

  • remove wrong incrementing mpoly pointer in second loop
  • avoid divisions on favor of multiplications (for some reason I see no performance difference here)
  • deduplicate code for mesh/bmesh
NOTE: needs to be "max_ab > 0.0f" (not "max_ab >= 0.0f"), right @Brecht Van Lommel (brecht)?

Making it GPU enabled was an effort to reduce unneeded data transfers. Looking at previous patches makes me wonder if it could it also be solved by uploading floats in stead of GPU_COMP_I16? Have you tried this in one of your attempts.

Pretty sure it was area_ratio_get (that uses floats) already that returned 0.0f, let me double-check...

Patch looks fine to me now.

I imagine using floats works as well, I have no strong opinion either way.

This revision is now accepted and ready to land.Aug 30 2022, 11:05 AM

Making it GPU enabled was an effort to reduce unneeded data transfers. Looking at previous patches makes me wonder if it could it also be solved by uploading floats in stead of GPU_COMP_I16? Have you tried this in one of your attempts.

Iirc, you have about 6 digits to rely on using floats?
In the example file from the report, we deal with stuff like this:

  • uvarea 0.00008498
  • area 942.78295898
  • ratio 0.00000009 (unreliable)

or

  • uvarea 0.06508513
  • area 722083.43750000
  • ratio 0.00000009 (unreliable)

So afaict, floats wont do the trick, right?

For floats you have not just the digits but also an exponent to shift where the digits are.

That means your digits may be x.xxxxx, but also 0.0000xxxxxx. So a ratio around 0.00000009 can actually be quite accurate.

Multiplication/division generally don't lose much precision unless your exponent runs out of bits.

OK, tried floats, seems to work :)

Posted as D15810: Fix T93084: Area stretch overlay full red on large scale mesh, sorry to waste your time but I thought to keep them separate because they are so different.
Mind checking on that one? (thx in advance).

Philipp Oeser (lichtwerk) planned changes to this revision.Aug 30 2022, 1:45 PM

Will put this on hold to see if D15810 is preferred.