Fix T90782: Add UV Pack option to specify margin as a fraction.
ClosedPublic
Actions

Authored by Chris Blackbourn (chrisbblend) on Oct 1 2022, 7:11 AM.

Details

Reviewers

Maniphest Tasks

T90782: UV: unwrap. Margin size is not related to any useful measure
T85978: Smart UV island margin value changed since C rewrite

Commits

rBc2256bf7f714: Fix T90782: add uv pack option to specify margin as a fraction

Summary

A refactor of the margin calculation of UV packing, in anticipation of multiple packing methods soon becoming available.

Three margin scaling options are now available:

"Add", Simple method, just add the margin. This is the default margin scale from Blender 2.8 and earlier. [0]
"Scaled", Use scale of existing UVs to multiply margin, the default from Blender 3.3+
"Fraction", a new (slow) method to precisely specify a fraction of the UV unit square for margin. [1]

The "fraction" code path implements a novel combined search / secant root finding method which exploits domain knowledge to accelerate convergence while remaining robust against bad input.

[0]: Resolves T85978
[1]: Resolves T90782

Diff Detail

Repository: rB Blender

Event Timeline

Chris Blackbourn (chrisbblend) requested review of this revision.Oct 1 2022, 7:11 AM

Chris Blackbourn (chrisbblend) created this revision.

Chris Blackbourn (chrisbblend) mentioned this in D16058: Fix T85978: Add option to use old margin formula for UV Smart Project..

Chris Blackbourn (chrisbblend) mentioned this in T90782: UV: unwrap. Margin size is not related to any useful measure.Oct 1 2022, 7:26 AM

This adds a new method ED_PACK_MARGIN_FRACTION which seems to be new and behavior, the patch description should explain why this was added and the advantages it has over existing methods.

source/blender/editors/include/ED_uvedit.h
343–345	Prefer `ED_UVPACK_` prefix (there can be other kinds of packing in Blender).
346	Prefer to follow type naming from `UVPackIsland_Params`, `eUVPackIsland_MarginMethod`.
source/blender/editors/uvedit/uvedit_islands.cc
549	This may as well return `BoxPack *box_array` instead of taking it as an argument.
553–573	I find these checks for different packing methods difficult to parse, if practical - splitting this into a switch statement could make the intended behavior clearer - just suggestion if it can be made to work nicely.
source/blender/editors/uvedit/uvedit_unwrap_ops.c
2448	Can be const.

This revision now requires changes to proceed.Oct 4 2022, 7:03 AM

Updated enum names. Incorporated feedback. Improved comments.

Testing this patch a bit and while the code seems acceptable, it would be good to have an example showing how the new method ED_PACK_MARGIN_FRACTION behaves well in cases the other methods don't.

From some quick tests I didn't seem much difference but it's likely I wasn't testing cases where the simple methods perform poorly.

Testing this, with the margin set high for ED_UVPACK_MARGIN_FRACTION source/blender/editors/uvedit/uvedit_islands.cc:529:49: runtime error: division by zero

The UV's are then set to -nan.

Looking into it further I can see there is a limit beyond which can't be usefully applied, in that case it could clamp - ideally.

source/blender/editors/uvedit/uvedit_islands.cc
586–588	Add `break;` statements (even though they're unreachable, causes compile warning with GCC). /src/blender/source/blender/editors/uvedit/uvedit_islands.cc: In function ‘BoxPack* pack_islands_params(const blender::Vector<FaceIsland>&, const UVPackIsland_Params&, float)’: /src/blender/source/blender/blenlib/BLI_assert.h:57:40: warning: this statement may fall through [-Wimplicit-fallthrough=] 57 \| _BLI_ASSERT_ABORT(), \ \| ^ /src/blender/source/blender/blenlib/BLI_assert.h:96:5: note: in expansion of macro ‘BLI_assert_msg’ 96 \| BLI_assert_msg(0, "This line of code is marked to be unreachable."); \ \| ^~~~~~~~~~~~~~ /src/blender/source/blender/editors/uvedit/uvedit_islands.cc:586:7: note: in expansion of macro ‘BLI_assert_unreachable’ 586 \| BLI_assert_unreachable(); /* Handled above. */ \| ^~~~~~~~~~~~~~~~~~~~~~ /src/blender/source/blender/editors/uvedit/uvedit_islands.cc:587:5: note: here 587 \| default: \| ^~~~~~~ [100%: -0] Linking CXX executable bin/blender
source/blender/editors/uvedit/uvedit_unwrap_ops.c
1155–1165	The descriptions seems not all that helpful, that some methods are simple/complex fast/slow doesn't explain what they do. It would be good to describe the final output from a user perspective.
1159	Scale margin by average bounding-box length (fast) ?

Chris Blackbourn (chrisbblend) edited the summary of this revision. (Show Details)Oct 5 2022, 8:32 AM

Chris Blackbourn (chrisbblend) edited the summary of this revision. (Show Details)

Chris Blackbourn (chrisbblend) added a task: T90782: UV: unwrap. Margin size is not related to any useful measure.

Vyacheslav (hitrpr) awarded a token.Oct 5 2022, 10:26 AM

Improve strings / enums to be more user facing.

Fix / tune fraction method with large margin

Chris Blackbourn (chrisbblend) edited the summary of this revision. (Show Details)Oct 8 2022, 6:52 AM

Accepting with minor requests.

source/blender/editors/include/ED_uvedit.h
354–355	Prefer shared prefix for naming. e.g. eUVPackIsland_MarginScale margin_method; float margin; Or... eUVPackIsland_MarginScale island_margin_method; float island_margin;
source/blender/editors/uvedit/uvedit_islands.cc
570	Include short note on why it's slow.
source/blender/editors/uvedit/uvedit_unwrap_ops.c
1146	picky prefer comma before `};`, it means minor changes to naming or making `const` wont re-indent the whole block.
2122–2132	`margin_scale` reads too much like a multiplier. Also, renaming the existing "margin" will cause problems for any scripts that call this operator. While we're not considering every change to operator properties an API breakage, it's not so nice if scripts that call this operator break between releases. Suggest to call `margin_method` (same for `UVPackIsland_Params.margin_scale`).