Page MenuHome

UV_OT_align and UV_OT_weld
AbandonedPublic

Authored by Habib Gahbiche (zazizizou) on May 25 2018, 7:51 PM.

Details

Reviewers
None
Maniphest Tasks
T54645: Multi-Object-Mode: EditMesh UV Tools
Summary

Using one patch for both operators because they both use the same underlying function

uv_weld_align

Diff Detail

Repository
rB Blender
Branch
arcpatch-D3439_1 (branched from blender2.8)
Build Status
Buildable 2016
Build 2016: arc lint + arc unit

Event Timeline

Alan Troth (Al) added inline comments.
source/blender/editors/uvedit/uvedit_ops.c
1726–1728

You should only be calling updates and notifiers on an object if the object has changed in some way.

Notifiers and updates are now called only if UVs of object have changed.

I think mode "s" (straighten) does not behave correctly. Even in Blender 2.79 it sometimes gives wrong results: sometimes it collapses vertices to the same point, which technically means they all lie on the same line, but it's not the behaviour I expected, so we might need to change/fix its implementation.

@Alan Troth (Al): thanks for the comment. Do you think it's now better with the bool array?

Initializing the bool array was not necessary

@Alan Troth (Al): thanks for the comment. Do you think it's now better with the bool array?

Yes, that's what was needed. Just one comment,

source/blender/editors/uvedit/uvedit_ops.c
1522

Nothing in the object has been changed here so it's probably wrong to have a 'changed = true'. Most of the time any object with changed set to true here will later end up being changed so it may not hurt too much. (A rare case where it wouldn't change is when straighten is used when only one uv vertex is selected).

I think mode "s" (straighten) does not behave correctly. Even in Blender 2.79 it sometimes gives wrong results: sometimes it collapses vertices to the same point, which technically means they all lie on the same line, but it's not the behaviour I expected, so we might need to change/fix its implementation.

I also noticed this or a similar problem with straighten when I was having a look at how the uv operators worked. I selected the uvs to make two separate lines in two uv islands on a single object, when I used the straighten operator the uvs collapsed to two points. The problem seemed to be that when the code is tagging edges ( /* flush vertex tags to edges */ ), it tags edges a seen on the mesh and not edges as seen in the uv/image editor - to the code my two separate uv lines where actually one line with the start and end uvs visibly in the middle of the line, not the outside.

Habib Gahbiche (zazizizou) marked an inline comment as done.
Habib Gahbiche (zazizizou) marked an inline comment as done.

code style: spaces to tabs

updated according to previous comments from Dalai:

  • return OPERATOR_FINISHED only if change occurred (changed function return type)
  • use MEM_FreeN for array

Thanks for the patch, but I committed a different solution on rB994f438af3d62f70e1e2d5e79f8268cbd0483dfd.

The problems with this patch are:

  • Unnecessary high amount of for (object...) loops. We only need to loop twice at most. All the operations are done based on common center. Besides that all they need to care about is their own data.
  • The code changes "tool" based on the selected vertices direction (when tool is 't' or 'u'). You can't allow one object iteration to change the tool that will be used by another one.
  • Not a huge problem, but we can skip the for loop when no selected vertex (if syncing selection is true).
  • When the operator has many options is good to always return OPERATOR_FINISHED, otherwise going through the options may lead the operator to be cancelled preventing further tweaks.