Page MenuHome
Differential D6562

Fix T71200: Build curve geometry in one piece
ClosedPublic
Actions

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

Details

Reviewers
Campbell Barton (campbellbarton)
Germano Cavalcante (mano-wii)
Maniphest Tasks
T71200: Beveled curves geometry contains duplicate vertices
Commits
rBe34d3e32dda7: Fix T71200: Build curve geometry in one piece
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:

Curve_Extrude_Bevel_Test.blend685 KBDownload

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) created this revision.Jan 11 2020, 8:41 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) added a task: T71200: Beveled curves geometry contains duplicate vertices.
noki paike (amonpaike) awarded a token.Jan 12 2020, 9:02 AM
Erick Tukuniata (erickblender) awarded a token.Jan 12 2020, 9:32 AM
Evan Wilson (EAW) awarded a token.Jan 12 2020, 3:15 PM
Germano Cavalcante (mano-wii) added a comment.Jan 12 2020, 4:12 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.

Hans Goudey (HooglyBoogly) planned changes to this revision.Jan 12 2020, 4:34 PM
In D6562#154281, @Germano Cavalcante (mano-wii) wrote:

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.

Hans Goudey (HooglyBoogly) updated this revision to Diff 20734.Jan 12 2020, 4:40 PM

Remove non-functional changes.

Hans Goudey (HooglyBoogly) updated this revision to Diff 20736.EditedJan 12 2020, 4:42 PM

More cleanup removed

Germano Cavalcante (mano-wii) requested changes to this revision.EditedJan 12 2020, 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.Jan 12 2020, 9:14 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 20752.Jan 13 2020, 3:52 PM
Hans Goudey (HooglyBoogly) marked 3 inline comments as done.

Solve compiler warnings

Hans Goudey (HooglyBoogly) added a comment.Jan 13 2020, 3:54 PM
In D6562#154325, @Germano Cavalcante (mano-wii) wrote:

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.Jan 13 2020, 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.Jan 13 2020, 4:06 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 20757.Jan 13 2020, 4:42 PM
Hans Goudey (HooglyBoogly) marked an inline comment as done.

Improved precision of multiplication, merged master

Germano Cavalcante (mano-wii) accepted this revision.Jan 13 2020, 6:15 PM
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.Jan 13 2020, 6:30 PM
Closed by commit rBe34d3e32dda7: Fix T71200: Build curve geometry in one piece (authored by Hans Goudey (HooglyBoogly)). · Explain Why
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rBe34d3e32dda7: Fix T71200: Build curve geometry in one piece.
Hans Goudey (HooglyBoogly) added a comment.Jan 13 2020, 6:32 PM

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.Jan 13 2020, 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.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
137 lines

Diff 20760

View Options

source/blender/blenkernel/intern/curve.c

Loading...