Page MenuHome

Implementation of UV unwrapping with SLIM as disdussed on T48036.
Needs RevisionPublic

Authored by Aurel Gruber (AurelGruber) on Feb 27 2017, 2:04 PM.

Details

Summary

implementation for T48036

commits:

Category: UV Unwrapping SLIM Algorithm Integration

Added SLIM Subfolder

Category: UV Unwrapping SLIM Algorithm Integration

added subfolder SLIM to CMakeFile of /intern

Category: UV Unwrapping SLIM Algorithm Integration

integrating SLIM including data gathering and transfer from Blender to SLIM

This commit is huge, because I copied over the code from a different repository. Not commit-by-commit.

The Algorithm can be invoked either by choosing SLIM from the dropdown in the unwrapping settings or by
hitting ctrl. + m in the uv editor for relaxation. Tried adding it to the menu the same way as minimizing stretch is there but failed.

Category: UV Unwrapping SLIM Algorithm Integration

preserving vertex ids and gathering weights

Category: UV Unwrapping SLIM Algorithm Integration

adding more members to phandle and creating param_begin

Category: UV Unwrapping SLIM Algorithm Integration

fixing bug that causes weight-per-vertex mapping to be wrong

Category: UV Unwrapping SLIM Algorithm Integration

adjustments to the UI parameters

Category: UV Unwrapping SLIM Algorithm Integration

adding weightinfluence

Category: UV Unwrapping SLIM Algorithm Integration

slim interactive exec now only for changing parameters

Category: UV Unwrapping SLIM Algorithm Integration

taking care of memory leaks

Category: UV Unwrapping SLIM Algorithm Integration

adding relative scale, reflection mode and Vertex group input

Category: UV Unwrapping SLIM Algorithm Integration

correcting wrong comment on SLIM phases

Category: UV Unwrapping SLIM Algorithm Integration

Adding SLIM code by means of git read-tree

Category: UV Unwrapping SLIM Algorithm Integration

freeing matrix_transfer properly

Category: UV Unwrapping SLIM Algorithm Integration

adding (unsupported-) eigen files

Diff Detail

Repository
rB Blender
Branch
uv_unwrapping_slim_algorithm
Build Status
Buildable 562
Build 562: arc lint + arc unit

Event Timeline

Aurel Gruber (AurelGruber) retitled this revision from to Implementation of UV unwrapping with SLIM as disdussed on T48036..
Aurel Gruber (AurelGruber) updated this object.

Comments from an initial test and read of the code.

  • intern/SLIM should be renamed to intern/slim for consistency. Also the files in this folder could use lower case and underscores to make them all consistent.
  • Modules in intern/ normal put the public header file in the top level folder, and other files in intern/slim/intern.
  • The GPL license header is normally included in every Blender source file, with contributors listed like Contributor(s): Aurel Gruber
  • Code changes should not be marked with your name, for that we can use git diff and blame.
  • Compiler warnings should be fixed:
  • There are some code style inconsistencies, like using ){ instead of ) {, not putting { on a new line for functions, mixel of camelCase and underscores for variable and struct members names, .. . It's ok to follow a different style in modules if needed, but in source/blender it should be consistent.
  • The old minimize stretch code could be removed entirely and replaced with the new SLIM method. No need to have two separate operators I think.
  • For ABF and LSCM it automatically packs the islands, but for SLIM they just seem to end up outside the 0..1 bounds and on top of each other. Tested by unwrapping the monkey mesh.
  • Minimize Stretch SLIM blending doesn't seem to work. Do we really need an iterations settings there, instead of just leaving it to 0? The weight influence also does nothing I think, it's not clear to me why it's there without the property to choose a vertex group.
intern/SLIM/src/matrix_transfer.h
50 ↗(On Diff #8321)

Can we name this something like SLIMMatrixTransfer? Since this in the module API.

This file should also use extern "C" { if it's going to be included from C. Not strictly needed now probably, but just to be sure.

intern/SLIM/src/slim_C_interface.h
19–23 ↗(On Diff #8321)

To make it clear which functions call into this module, and to avoid naming conflicts, please prefix these function names with SLIM_.

This file could perhaps also be renamed to SLIM_capi.h. We don't really have one naming convention for such module headers, but might as well follow one that's already in use.

source/blender/bmesh/bmesh_class.h
103 ↗(On Diff #8321)

We want to keep Mesh memory usage as low as possible, and should not add such members for specific algorithms.

Maybe BM_elem_index_get(l->v) already works? In fact that's already being passed as the key, do we really need both?

source/blender/editors/uvedit/uvedit_parametrizer.c
4685

Debug code like this should be removed.

source/blender/editors/uvedit/uvedit_parametrizer.h
84–97

So far this API in uvedit_parametrizer.h has stayed decoupled from Blender object or mesh data structures. Is it really needed to add so many API functions here?

Ideally we could keep a clean separation, with some generic param_* API functions, and not exposing the SLIM interface outside uvedit_parametrizer.c.

129–135

It's not clear to me why this functions are in this header, they can be static functions in uvedit_parametrizer.c? Also no need for function prototypes if they are defined in the right order.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
88–89

We already have MAX2 and MIN2 defines for this.

577

Such local functions should be defined static, though this specific one might as well be inlined I guess.

1019–1022

Property names should not include _slim.

1247

Remove this debug print.

1337–1340

This seems like it could be a boolean instead of an enum.

1367

Same here, no need to use slim_ or SLIM in the property names. If some parameters only work for SLIM, we can change the UI to only show them when that method is selected.

See e.g. image_open_draw for how to add such a custom UI draw method.

1369–1370

Is 1 a good default? From a quick test it seems I usually need more to get a good result. -10 as a minimum also seems wrong, should be 1 I guess?

1374–1376

Any reason you're not using RNA_def_string?

With a custom UI it would be possible also to show the Vertex Group icon and a dropdown menu, though the code for that might be a bit tricky in this case.

1378

The min/max of -10000 to 10000 seems wrong?

I would rename this to vertex_group_factor and "Factor", for consistency with other places in Blender.

Removed double comment.

Aurel Gruber (AurelGruber) edited edge metadata.

Corrections as discussed on D2530 + new uv_initializers (MVC + Harmonic) + allowing to use minimize stretch to affect only selected vertices

  • Merge branch 'uv_unwrapping_slim_algorithm' of git.blender.org:blender into uv_unwrapping_slim_algorithm
  • UV Unwrapping SLIM: adding harmonic and mvc to uvinitializer
  • UV Unwrapping SLIM: refactoring UVInitializer
  • UV Unwrapping SLIM: renaming intern/SLIM to intern/slim
  • UV Unwrapping SLIM: renaming files and moving headers
  • UV Unwrapping SLIM: renaming another file
  • UV Unwrapping SLIM: adding GPL header comment
  • UV Unwrapping SLIM: removing thesis marker-comments from code
  • UV Unwrapping SLIM: respecting source/blender style conventions
  • UV Unwrapping SLIM: removing old minimize stretch operator
  • UV Unwrapping SLIM: further refactoring according to discussion on D2530
  • UV Unwrapping SLIM: adding slim_matrix_transfer.h
  • UV Unwrapping SLIM: renaming src to intern
  • UV Unwrapping SLIM: renaming slim_c_interface to slim_capi and renaming contained functions
  • UV Unwrapping SLIM: removing unnecessary ui parameters from minimize_stretch_operator
  • UV Unwrapping SLIM: minimize_stretch now only affects selected vertices. RESPECTS user defined pins!
  • UV Unwrapping SLIM: space added after if
  • UV Unwrapping SLIM: removing remainder of old minimize stretch
  • UV Unwrapping SLIM: reordering functino definitions to remove prototypes and cleaning header files
  • UV Unwrapping SLIM: small bugfixes and putting char array and not just pointer into toolsettings for vertexgroupname

Hey Brecht

Thank you for the review!
I have not yet implemented all of your suggestions, namely I still have:

  • fix compiler warnings
  • remove slim_ from property names and use custom ui draw method
  • renaming some ui parameters

Some thoughts on minimize_stretch & vertex groups:

I think that the parameters "Vertex Group", "Vertex Group Factor" and "relative Scale" should be per-object-settings. this way, unwrapping, live-unwrapping (working on that) and minimize-stretch could always use the correct settings for any object. I think that given the possibility to choose a vertex group, having a relaxation algorithm that does not respect the vertex group, can in many situations be annoying. Don't you think? There could still be an option to disable the use of the vertex group.
Regarding the minimize-stretch blend factor: I think it works correctly. On my macbook scrolling doesn't trigger the WHEEL_DOWN case, but with a mouse it does. How did you test it? I had the same issue with the old minimize stretch operator btw.

Also, why is this comment (line 1383-1386 uvedit_unwrap_ops) linked to this comment? I didn't intend for that.

source/blender/editors/uvedit/uvedit_unwrap_ops.c
1337–1340

The idea is to later add automatic - which, given the pinned vertices, should evaluate automatically whether the user intends to flip the map or not. Is it ok to leave it like this for now or should I write a comment above it or switch to boolean for now?

UV Unwrapping SLIM: Added Live Unwrapping - corrections based on D2530: removing slim_ prefix from ui parameters, using custom ui draw, taking care of compiler warnings

  • Merge branch 'slim' into 'master'
  • Merge branch 'live_unwrap'

Hey,

With the latest commit, live unwrapping is now implemented. It should be noted that SLIM doesn't perform tremendously well here for larger geometries. SLIM requires to build and then solve a minimisation problem in every iteration. We try to precompute as much as possible, but it can still get a bit slow at times. Unwrapping suzane is not a problem. After 2 subdivs it gets quite slow. I'll see if there is something I can do.

Also, I believe I took care of all the things @Brecht Van Lommel (brecht) mentioned.

I'm grateful for any further inputs.

Hey,

With the latest commit, live unwrapping is now implemented. It should be noted that SLIM doesn't perform tremendously well here for larger geometries. SLIM requires to build and then solve a minimisation problem in every iteration. We try to precompute as much as possible, but it can still get a bit slow at times. Unwrapping suzane is not a problem. After 2 subdivs it gets quite slow. I'll see if there is something I can do.

Also, I believe I took care of all the things @Brecht Van Lommel (brecht) mentioned.

I'm grateful for any further inputs.

Some more issues found in testing:

  • The minimize stretch is missing ED_area_headerprint() to visually indicate that we are interactive mode and to inform the users how to use it. It seems to be working, but on an already unwrapped result it wasn't clear the operator was running at all. The exec() implementation and iterations and blend properties are also needed to execute the operator in non-interactive mode.
  • It's not clear to me when relative scale other than 1.0 is useful, it seems to make the unwraps worse? When would you use this?
  • For vertex groups I suggest to set a default vertex group name, like "unwrap_weights", and use that as the convention for naming the vertex group. Then it doesn't need to be remember per object (actually mesh if anything). In general I'd be hesitant to add such properties permanently to meshes, there aren't really any other tools that work the same way.
  • Vertex group weights don't work for subdivision surfaces, the weights is always built from the editmesh. Probably the better approach is to add a weight parameter to param_face_add and add them the weights to PVert, rather than adding them afterwards.
  • Unwrapping a mix of pinned and non-pinned charts places charts in seemingly random positions and scales.
  • Live unwrap doesn't seem to be working for me? Only the pins move, the rest of the chart doesn't follow along.

I thought I might have been looking at the wrong code, but the uv_unwrapping_slim_algorithm branch and this diff seem to contain the same code.

The code still seems too spread out between uvedit_unwrap_ops.c and uvedit_parametrizer.c, it's difficult to follow the logic for me. I think I can refactor that code behind a simple param_slim_* API so it's almost all in uvedit_parametrizer.c. If you don't mind I can commit that refactor and some fixes in the branch directly when I have some time this weekend.

Brecht Van Lommel (brecht) requested changes to this revision.Mar 23 2017, 4:59 AM
This revision now requires changes to proceed.Mar 23 2017, 4:59 AM
Aurel Gruber (AurelGruber) edited edge metadata.

UV Unwrapping SLIM - improving live unwrap, implementing more efficient convex-border-construction, adding global scale invariance in the presence of pins

Thank you for the testing, feedback and everything.

I improved live unwrap, it works well for me. it had a few issues before and sometimes got stuck. does it work better with this version?
I also added global scale invariance which makes it much more predictable i think.

regarding your points, in order:

  • I will take a look at ED_area_headerprint(). I wasn't aware that that exists. I will also look at exec().
  • relative scale. Indeed, values other than 1.0 make the map worse. The idea behind it is this: If a user sets for example 2 pins. And then he drags one pin around. What's his intention? Does he want to make the entire map bigger or does he want to stretch it? If he want to scale the entire map, then relative scale should remain 1. but what if he wants to stretch? then, setting relative scale to < 1 does the trick. It's not very intuitive and a different mechanism should be used. Maybe do an initial unwrap and then have a parameter boolean "keep scale fixed" or so? I hope this is understandable.
  • I will fix the problem with the weights and subdivision surfaces
  • I will fix the problem with pinned and non-pnned charts
  • Live unwrap should work. I made pretty big update - maybe it works now?

If you could commit the refactor I wouldn't mind at all, but rather greatly apreciate that :) ! I'm not all that familiar with software architecture, you are also welcome to recommend me a read in that area.

best,
Aurel

I've pushed a bunch of changes to the uv_unwrapping_slim_algorithm branch:

  • Code refactoring to match code style
  • Transfer weights in construction, fix subsurf weights.
  • Avoid using scene toolsettings, this was old mechanism from before 2.5x.
  • Move SLIM integration behind into param_* API, reuse more LSCM code.
  • Fix issue with live unwrap and multiple pinned/non-pinned charts.

Live unwrap seems to be working in my tests, but I found some issues:

  • Live unwrap sometimes show flickering UVs moving into random places. It doesn't always happen, maybe an uninitialized variable somewhere.
  • Pinned vertices can move to a different position, both using regular and during live unwrap. I guess this is a result of the way the algorithm works, or is it a bug?
  • Live unwrap only works when moving the mouse, so you have jiggle the mouse to keep it solving. A timer could be used to keep it solving.

Hey Brecht,

Thank you! I'm looking forward to take a look at it, but probably not before Wednesday.

Have a nice week

Aurel

Hey,

The code is so much cleaner now, thank you! Must have taken quite some time, I appreciate it!

Regarding live unwrap, there are some issues.

The algorithm uses "soft constraints". This means, that by pinning a vertex, you express the desired location of that vertex, the algorithm is not guaranteed to respect it. This is especially important for live unwrap. So ideally, and i know that would be a bit too much work probably, the vertex you drag around in liveunwrap mode wouldn't be the actual vertex, but another point that indicates the desired location. Since that isn't so easy, I instead "abuse" the actual vertex as softconstraint. But the actual location of the vertex as the algorithm "outputs it" is not the location the dragged-around vertex has. Therefore I implemented it originally s.t. when I release the vertex, it snaps to whatever location the algorithm has reached. You now changed it to stay wherever it was dragged. This is problematic, because if the algorithm is invoked again, be it via minimize_stretch or live_unwrap, it has to re-initialize because flipped triangles are present in the map. From my perspective, it is now too confusing. Don't you think? Also, If the algorithm doesn't have to re-initialize, the user ideally would never notice pins moving - unless he chooses weird locations. I would therefore suggest that upon end of liveunwrap, the vertex snaps back to wherever it got through the means of SLIM.

One other thing i noticed: It is no longer possible to drag around unpinned vertices when in live unwrap mode.

I can take a look at the flickering vertex, noticed it too.

A timer would indeed be nice! How would that best be implemented? Such that ED_uvedit_live_unwrap_begin starts the iterations and the timer and ED_uvedit_live_unwrap_resolve would simply update the positions? Is there something in blender that already uses such a timer to cheat off of?

thanks

Aurel

  • SLIM: match code style.
  • SLIM: move code around, no functional changes.
  • SLIM: use operator history instead of toolsettings for remembering settings.
  • SLIM: transfer weights in construction, fix subsurf case.
  • SLIM: move most SLIM integration behind param_* API, reuse more code.
  • SLIM: reuse LSCM logic for pinning, to fix issue with multiple charts.
  • SLIM: In live Unwrap mode unpinned vertices can now be moved. Also, a single pin also invokes SLIM. That also avoids crashing minimize_stretch when invoked on chart with one pin.
  • SLIM: disallowing pins to move to places that the SLIM algorithm can't handle. Also, this simplifiey the way we transfer uv coords back to native part.

Hey,

I took care of the mentioned problems. Key differences:

  • when using live unwrap with SLIM, pins can no longer be dragged "ahead of the algorithm". This means that it's more robust and predictable. If we drag the vertex to a location it can't reach, it simply doesn't reach that position, or only very slowly.
  • when in live unwrap mode with SLIM, unpinned vertices can now be moved in the same way as in ABF live unwrap.

Haven't tackled the timer thing yet

Thanks

Aurel

This revision now requires changes to proceed.May 18 2017, 5:26 PM

Hey Guys,

Sorry for neglecting this a little.

I did not yet have time to add the timer to the live unwrap. Apart form that, what else has to be taken care of? Is the following accurate, if not, what else?

  • Add timer to live-unwrap
  • Do some testing / bugfinxing
  • release test-build on graphicall
  • improve upon feedback / bug reports
  • done

@Hassan Shoob (Shoob)
In order to use it you have to either checkout the branch uv_unwrapping_slim_algorithm or apply this diff as a patch.

building:

  • you need to build the cmake option WITH_CXX_11
  • in order for that to succeed on mac osx you need to build Not with the libraries you find at:

https://svn.blender.org/svnroot/bf-blender/trunk/lib/darwin-9.x.universal

but with the ones you find here:

https://svn.blender.org/svnroot/bf-blender/trunk/lib/darwin/

  • It should work on linux and windows, but apparently someone had issues on linux:

https://blenderartists.org/forum/showthread.php?420309-Build-Blender-from-source-(first-time)/page2

Any news about this patch? the build works well, only have some crashes, but appear stable.