UV: Update ui for pack islands operator #108253

Chris Blackbourn · 2023-05-25T05:08:34+02:00

Chris Blackbourn commented

2023-05-25 05:08:34 +02:00

No response

_No response_

Chris Blackbourn added 1 commit 2023-05-25 05:08:46 +02:00

40bb54fc5f UV: Update ui for pack islands operator

Chris Blackbourn commented

2023-05-26 04:59:35 +02:00

Before:

After 40bb54fc5f

Before: ![image](/attachments/955fdcb7-3856-46bc-8018-076a53d468c3) After 40bb54fc5f ![image](/attachments/a87cc8e9-f435-4e78-aabe-788e1c4ee909)

image.png

63 KiB

image.png

57 KiB

Brecht Van Lommel requested review from Pablo Vazquez 2023-05-30 18:45:09 +02:00

Brecht Van Lommel reviewed 2023-05-30 18:56:29 +02:00

source/blender/editors/uvedit/uvedit_unwrap_ops.cc

					
				@ -1609,2 +1618,3 @@

				     "Locked Rotation and Scale",

				     "Pinned islands will translate only"},

				    {ED_UVPACK_PIN_LOCK_ALL, "LOCKED", 0, "Locked position", "Pinned islands are locked in place"},

				    {ED_UVPACK_PIN_LOCK_ALL, "LOCKED", 0, "Locked Position", "Pinned islands are locked in place"},

Brecht Van Lommel commented

2023-05-30 18:56:29 +02:00

Follow convention for tense:

Ignored -> Ignore
Locked Scale -> Lock Scale
...

Make identifier match label:

SCALE -> LOCK_SCALE
ROTATION -> LOCK_ROTATION
...

This seems like it would rotate and scale in place, seems more clear like:

Locked Position -> Lock in Place

Follow convention for tense: ``` Ignored -> Ignore Locked Scale -> Lock Scale ... ``` Make identifier match label: ``` SCALE -> LOCK_SCALE ROTATION -> LOCK_ROTATION ... ``` This seems like it would rotate and scale in place, seems more clear like: ``` Locked Position -> Lock in Place ```

Chris_Blackbourn marked this conversation as resolved

source/blender/editors/uvedit/uvedit_unwrap_ops.cc Outdated

					
				@ -1669,10 +1712,11 @@ void UV_OT_pack_islands(wmOperatorType *ot)

				               "");

Brecht Van Lommel commented

2023-05-30 18:51:22 +02:00

Merge Overlapped -> Merge Overlapping

Chris_Blackbourn marked this conversation as resolved

source/blender/editors/uvedit/uvedit_unwrap_ops.cc Outdated

					
				@ -1669,10 +1712,11 @@ void UV_OT_pack_islands(wmOperatorType *ot)

				               "");

				  RNA_def_float_factor(

				      ot->srna, "margin", 0.001f, 0.0f, 1.0f, "Margin", "Space between islands", 0.0f, 1.0f);

				  RNA_def_boolean(ot->srna, "pin", false, "Pin", "Pinned islands are handled differently");

Brecht Van Lommel commented

2023-05-30 18:52:10 +02:00

I don't understand the purpose of this boolean, "handled differently" doesn't explain what leaving this disabled does.

I suggest to remove this boolean, and instead to rename the Default method to Pack, and then have options in this order:

Pack
Lock in Place
Lock Rotation
Lock Scale
Lock Rotation and Scale
Ignore

I don't understand the purpose of this boolean, "handled differently" doesn't explain what leaving this disabled does. I suggest to remove this boolean, and instead to rename the Default method to Pack, and then have options in this order: ``` Pack Lock in Place Lock Rotation Lock Scale Lock Rotation and Scale Ignore ```

Chris_Blackbourn marked this conversation as resolved

Brecht Van Lommel requested changes 2023-05-30 18:59:01 +02:00

Brecht Van Lommel left a comment

This is a nice improvement, only the pinning is still confusing to me.

Brecht Van Lommel commented

2023-05-30 19:07:01 +02:00

This could also use some separators (uiItemS(layout)) to more easily scan the options. I suggest:

Shape Method
Scale
Rotate
Rotation Method

Margin Method
Margin

Pinned Islands

Merge Overlapping

Pack to

This could also use some separators (`uiItemS(layout)`) to more easily scan the options. I suggest: ``` Shape Method Scale Rotate Rotation Method Margin Method Margin Pinned Islands Merge Overlapping Pack to ```

Chris Blackbourn added 1 commit 2023-05-31 00:05:04 +02:00

b3cb89e637 Updated from feedback.

Chris Blackbourn commented

2023-05-31 00:06:00 +02:00

After b3cb89e637

After https://projects.blender.org/blender/blender/commit/b3cb89e63783b00dbe908caf9835bb220bbd7745 ![image](/attachments/1bb5f3ef-dbbe-4621-ab0c-ff3a54aedc1d) ![image](/attachments/3edc6952-d6a7-4646-858a-3c5a6ff21510)

image.png

63 KiB

image.png

77 KiB

Brecht Van Lommel approved these changes 2023-05-31 01:08:17 +02:00

Brecht Van Lommel left a comment

Nitpicking:

I would put Ignore last in the list since to me it's different than the other options, so does not make much sense to be between them.
Seeing it now, maybe the separators between Pinned Islands, Merge Overlapping, Pack to are too much, could be removed again.

Nitpicking: * I would put Ignore last in the list since to me it's different than the other options, so does not make much sense to be between them. * Seeing it now, maybe the separators between Pinned Islands, Merge Overlapping, Pack to are too much, could be removed again.

Chris Blackbourn commented

2023-05-31 01:57:59 +02:00

Will squash and commit tomorrow so we can hear from @pablovazquez ?

![image](/attachments/aa69958f-06e8-48a0-a63a-b9d82c140d5f) ![image](/attachments/816a55b6-b090-4967-ac83-326a0c5bc08f) Will squash and commit tomorrow so we can hear from @pablovazquez ?

image.png

62 KiB

image.png

75 KiB

Chris Blackbourn added 1 commit 2023-05-31 01:58:52 +02:00

8bd546797d Updated from feedback.

Brecht Van Lommel approved these changes 2023-05-31 09:41:52 +02:00

Chris Blackbourn referenced this issue from a commit

2023-06-01 01:45:13 +02:00

UV: Update ui for the uv pack islands operator

Chris Blackbourn merged commit f87f119c2f into blender-v3.6-release

2023-06-01 01:45:14 +02:00