Skip to content

lscm: fix order of UV after area term sign fix#1863

Merged
alecjacobson merged 1 commit intolibigl:mainfrom
factoryofthesun:patch-1
Oct 2, 2022
Merged

lscm: fix order of UV after area term sign fix#1863
alecjacobson merged 1 commit intolibigl:mainfrom
factoryofthesun:patch-1

Conversation

@factoryofthesun
Copy link
Copy Markdown
Contributor

The fix in #1853 causes the solution UVs to be flipped, so this is a simple fix to that.

See attached a parameterization of an example surface. Apologies in advance for no reproducible example code in C -- I've been working with the python bindings and whipped this up on short notice.

Checklist

screenshot_000000
igl_lscm_param2

  • All changes meet libigl style-guidelines.
  • Adds new .cpp file.
  • Adds corresponding unit test.
  • This is a minor change.

The fix in libigl#1853 causes the solution UVs to be flipped, so this is a simple fix to that.
@alecjacobson
Copy link
Copy Markdown
Contributor

lol, so the git blame shows that this line has been toggled before e4876bd

What's the real issue here? Isn't LSCM orientation determined by the boundary conditions? If so, why should this method flip the output? If my understanding is correct then I'm inclined to merge this PR (and revert back to the original code!), but it'd be nice to be sure that the intent.

@kenshi84
Copy link
Copy Markdown
Contributor

Isn't LSCM orientation determined by the boundary conditions?

I don't think fixing two boundary vertices will determine the orientation of triangles.

My guessed logical explanation is that by using the wrong sign (+ instead of -) of the area term, the wrong LSCM energy wanted to make all the triangles flipped (i.e. to make their areas negative).

@alecjacobson alecjacobson merged commit d6db1cf into libigl:main Oct 2, 2022
@factoryofthesun factoryofthesun deleted the patch-1 branch December 15, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants