UV: Pack to closest/active UDIM
ClosedPublic
Actions

Authored by Siddhartha Jejurkar (sidd017) on Sep 28 2021, 8:44 PM.

Details

Reviewers

Campbell Barton (campbellbarton)

Maniphest Tasks

T78397: Pack to the same UDIM

Summary

Implements T78397
Extends the functionality of pack islands operator to allow packing UVs
to either the closest or active UDIM tile.
This provides 2 new options for packing UVs :

Closest UDIM: Selected UVs will be packed to the UDIM tile they were placed on. If not present on a valid UDIM tile, the UVs will be packed to the closest UDIM in UV space
Active UDIM: Selected UVs will be packed to the active UDIM image tile In case, no image is present in the UV editor, then UVs will be packed to the tile on the UDIM grid where the 2D cursor is located.

Diff Detail

Repository

rB Blender

Branch

T (branched from master)

Build Status

Buildable 17404
Build 17404: arc lint + arc unit

Event Timeline

Siddhartha Jejurkar (sidd017) requested review of this revision.Sep 28 2021, 8:44 PM

Siddhartha Jejurkar (sidd017) created this revision.

Harbormaster completed remote builds in B17399: Diff 42594.Sep 28 2021, 8:44 PM

Siddhartha Jejurkar (sidd017) added a task: T78397: Pack to the same UDIM.Sep 28 2021, 8:45 PM

Siddhartha Jejurkar (sidd017) added a reviewer: Campbell Barton (campbellbarton).Sep 28 2021, 8:48 PM

Siddhartha Jejurkar (sidd017) added a project: Restricted Project.

Siddhartha Jejurkar (sidd017) added a subscriber: Daniel Bystedt (dbystedt).

Minor cleanup.

Quiet clang-tidy warnings.
Prefer snake case (packTo -> pack_to)
Use full sentences.

Harbormaster completed remote builds in B17404: Diff 42606.Sep 29 2021, 1:41 AM

Generally seems fine, there are a few things that aren't clear to me:

Why is scene->toolsettings->target_udim needed? A user may be unwrapping multiple meshes - so storing a UDIM per scene doesn't seem correct. Blender supports storing values between operator execution, so re-running a pack with the same UDIM should work.

Further, storing a UDIM in the scene seems unnecessary when the image has an active-UDIM the user can adjust before running pack.

Suggest these changes: P2452

Uses the images active UDIM instead of storing it in the scene.
Supports packing to the active-UDIM.

NOTE: unless there is a compelling reason to keep SPECIFIED_UDIM it might even be best to replace this with ACTIVE_UDIM since the user can select a UDIM and pack into that instead of having to enter a number value.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
1132–1150	If the only purpose of this function is to conditionally hide a property, `wmOperatorType.poll_property` can be used instead of the draw functions.

Campbell Barton (campbellbarton) added inline comments.Sep 29 2021, 3:52 AM

source/blender/makesdna/DNA_scene_defaults.h
340	This would need to be version patched so users existing files won't have this value set to zero, although I'm not sure if this should be kept (see previous reply).

Campbell Barton (campbellbarton) added inline comments.Sep 29 2021, 3:54 AM

source/blender/makesdna/DNA_scene_defaults.h
340	Correction: This would need to be version otherwise users existing files will have this value set to zero. Although I'm not sure if this should be kept (see previous reply).

Changes based on review
Use 2D cursor for UDIM grid when using ACTIVE_UDIM option
Remove SPECIFIED_UDIM option
Refactor code and minor cleanup
Remove unnecesary DNA variable

Harbormaster completed remote builds in B17407: Diff 42614.Sep 29 2021, 6:54 AM

Campbell Barton (campbellbarton) added inline comments.Sep 29 2021, 7:09 AM

source/blender/editors/uvedit/uvedit_islands.c
241	`lie_on` isn't common terminology for Blender, suggest `isect` e.g.: `uv_coord_isect_udim(...)`
source/blender/editors/uvedit/uvedit_unwrap_ops.c
1011	Prefer common prefix to easily group related values: eg `PACK_UDIM_SRC_ACTIVE`, `PACK_UDIM_SRC_CLOSEST`
1081–1101	This block can be moved within the: `else if (pack_to == ACTIVE_UDIM) { ... }` check.
1173	Suggest to name `udim_source`.
1175–1183	This can be removed.