Page MenuHome

Fix T71200: Build curve geometry in one piece
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Sat, Jan 11, 8:41 PM.
Subscribers
None
Tokens
"Like" token, awarded by EAW."Like" token, awarded by erickblender."Like" token, awarded by amonpaike.

Details

Summary

Currently a curve's beveled geometry is built with duplicate geometry along the seams between the "front," "back," etc, sections of the curve. This builds them in one piece, resulting in smooth geometry.

The code is cleaned up a bit too, so it should be clearer what's actually going on there.

Other than the duplicate geometry, the vertex positions are the same as before.

A simple test file I used to check equality with the old method:

Diff Detail

Repository
rB Blender

Event Timeline

Thanks for the patch.
To simplify the review it would be nice to separate non-functional changes (cleanup) from functional changes into different patches.

Probably the cleanup will be accepted quickly.

Hans Goudey (HooglyBoogly) planned changes to this revision.Sun, Jan 12, 4:34 PM

Thanks for the patch.
To simplify the review it would be nice to separate non-functional changes (cleanup) from functional changes into different patches.
Probably the cleanup will be accepted quickly.

Alright, sure! I guess I can't sneak some comments in with the patch :P

I'll update this to be just functional changes and then submit a patch with comments and changed variable names after this lands.

Remove non-functional changes.

More cleanup removed

Germano Cavalcante (mano-wii) requested changes to this revision.EditedSun, Jan 12, 9:14 PM

These warnings came up when I tried

source/blender/blenkernel/intern/curve.c
1813
Warning	C4700	uninitialized local variable 'dl' used
1823
Warning	C6001	Using uninitialized memory 'dl'.
1842
Warning	C4701	potentially uninitialized local variable 'angle' used
This revision now requires changes to proceed.Sun, Jan 12, 9:14 PM
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.

Solve compiler warnings

These warnings came up when I tried

Oops! I didn't get warning but I probably should have.

Thanks for looking at this.

Campbell Barton (campbellbarton) requested changes to this revision.Mon, Jan 13, 4:06 PM

Minor change requested, otherwise works well & LGTM.

source/blender/blenkernel/intern/curve.c
1843

Changes to parenthesis here (and elsewhere) lower the precision of operations (so the multiplication is done on floats instead of doubles), although it's unlikely to cause issues, prefer to keep this as is since it's not the purpose of this patch to make these kinds of changes.

This revision now requires changes to proceed.Mon, Jan 13, 4:06 PM
Hans Goudey (HooglyBoogly) marked an inline comment as done.

Improved precision of multiplication, merged master

Germano Cavalcante (mano-wii) added inline comments.
source/blender/blenkernel/intern/curve.c
1838

These bitwise operations are a bit confusing.
For example, in this case, the CU_BACK flag does not need to be tested again.

In my opinion, this makes it easier to understand:

if (cu->flag & CU_BACK || !(cu->flag & CU_FRONT)) {

But I may be missing something. This is not very important.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jan 13, 6:30 PM
This revision was automatically updated to reflect the committed changes.

This revision was not accepted when it landed; it landed in state Needs Review.

Oh, sorry if I jumped the gun committing this @Campbell Barton (campbellbarton). It includes your suggested change to the parentheses.

Hans Goudey (HooglyBoogly) marked an inline comment as done.Mon, Jan 13, 6:34 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/curve.c
1838

I agree they are confusing, your suggestion of not checking flags that were already checked cleaned it up a bit.