Page MenuHome
Differential D14769

Curves: Port set type node to new data-block
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Apr 27 2022, 12:50 AM.

Details

Reviewers
Jacques Lucke (JacquesLucke)
Commits
rB9e393fc2f125: Curves: Port set type node to new data-block
Summary

This commit ports the "Set Spline Type" node to the new curves data-block.
Performance should be improved in similar ways to the other refactors from
the conversion task (T95443). The refactor has a few other explicit goals as well:

  • Don't count on initialization of attribute arrays when they are first allocated
  • Avoid copying the entire data-block when possible
  • Make decisions about which attributes to copy when changing curves more obvious
  • Use higher-level methods to copy data between curve points
  • Optimize for the common cases of single types and full selections
  • Process selected curves of the same types in the same loop

NURBS conversion changes are written by Piotr Makal (@Piotr Makal (pmakal)), thanks!

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 27 2022, 12:50 AM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) mentioned this in T95443: Refactor curve nodes to use new data structure.
Hans Goudey (HooglyBoogly) updated this revision to Diff 50998.Apr 29 2022, 4:05 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) added a subscriber: Jacques Lucke (JacquesLucke).
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Apr 29 2022, 8:37 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 51148.May 4 2022, 4:06 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Cleanup, fixes, merge some changes into master

There is still an issue with NURBS to Bezier conversion

Jacques Lucke (JacquesLucke) requested changes to this revision.May 4 2022, 4:46 PM

General structure seems fine, but there is still needs some more work.

source/blender/blenkernel/BKE_curves.hh
448

same size -> have the same number of control points

source/blender/blenkernel/intern/curves_geometry.cc
1316

Store name in new variable. That can make debugging simpler.

1353

Looks like this might not be taking src_curve_selection into account properly. Maybe iterate through src_curve_selection.slice(range) instead.

1369

Also doesn't use curve_selection correctly.

source/blender/blenloader/intern/versioning_300.c
2766

Incomplete comment.

source/blender/nodes/geometry/nodes/node_geo_curve_spline_type.cc
54–98

src.index_range()

55

Add a comment for why this is doing what it's doing.

216–220

Comment style.
The inline keyword does not ensure that this function is inlined. Not sure how inlining this would help with virtual arrays, it most likely does not.

223

Some comment explaining the code below would be useful.

286

Add a comment saying that in this case it doesn't matter what the source type is. Also the curves input is not used anymore.

317

Why are curve attributes ignored here?

333–337

Split this function up or add some high level comments within the function which say what happens where.

379

Converting catmull rom to bezier doesn't seem to work correctly for me currently. I'd expect the conversion to not change the shape of the curve.

catmull_rom_to_bezier.blend826 KBDownload

630

Yeah, probably fine to not have this. Also doesn't seem critical enough to justify using AllSpanOrSingle.

This revision now requires changes to proceed.May 4 2022, 4:46 PM
Hans Goudey (HooglyBoogly) mentioned this in D14877: Curves: Interpolate point count in add brush.May 6 2022, 4:24 PM
Hans Goudey (HooglyBoogly) mentioned this in D14870: Curves: Expose resample operator in sculpt mode.May 10 2022, 9:42 AM
Hans Goudey (HooglyBoogly) updated this revision to Diff 51347.May 10 2022, 5:43 PM

Further refactoring and cleanup

Hans Goudey (HooglyBoogly) planned changes to this revision.May 10 2022, 5:43 PM

This still needs the proper Catmull Rom to Bezier conversion which is still on my other computer. Also the NURBS to Bezier conversion is slightly broken unfortunately.

Hans Goudey (HooglyBoogly) added a parent revision: D14877: Curves: Interpolate point count in add brush.May 10 2022, 5:44 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 51423.May 12 2022, 12:14 PM

Merge master

Hans Goudey (HooglyBoogly) planned changes to this revision.May 12 2022, 12:14 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 51515.May 16 2022, 9:48 AM
  • Fixes
  • Proper Catmull Rom -> Bezier support
Hans Goudey (HooglyBoogly) planned changes to this revision.May 16 2022, 9:48 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)May 16 2022, 11:05 AM
AgentA1cr added a subscriber: AgentA1cr.May 16 2022, 9:06 PM
Hans Goudey (HooglyBoogly) marked 14 inline comments as done.May 19 2022, 6:16 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/nodes/geometry/nodes/node_geo_curve_spline_type.cc
223

Yes, that would be nice, I find it confusing myself though, the logic came from D13546.

Hans Goudey (HooglyBoogly) updated this revision to Diff 51628.May 19 2022, 6:18 PM
Hans Goudey (HooglyBoogly) marked an inline comment as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Lots of cleanup, all the logic is much closer. Review comments corrected. I'm still noticing issues with some conversions though.

Hans Goudey (HooglyBoogly) planned changes to this revision.May 19 2022, 6:18 PM
Piotr Makal (pmakal) added a subscriber: Piotr Makal (pmakal).May 20 2022, 8:40 PM
Piotr Makal (pmakal) commandeered this revision.Jun 1 2022, 9:37 PM
Piotr Makal (pmakal) added a reviewer: Hans Goudey (HooglyBoogly).

Hey guys, with a permission from @Hans Goudey (HooglyBoogly) I want to jump in and tackle NURBS to Bezier conversion.

Piotr Makal (pmakal) updated this revision to Diff 52135.Jun 1 2022, 9:46 PM
Piotr Makal (pmakal) edited the summary of this revision. (Show Details)

This update fixes NURBS to Bezier conversion. Bringing back behavior provided with the D13546 patch. Bare in mind that this fix/conversion still doesn't address the changes to Bezier knots mode introduced with D13891. This is not a trivial thing to handle and unfortunately currently I don't have enough time to look at it. For such cases conversion will happen but will not be ideal.

Piotr Makal (pmakal) added inline comments.Jun 1 2022, 10:00 PM
source/blender/nodes/geometry/nodes/node_geo_curve_spline_type.cc
318–323

This code was suggested by @Hans Goudey (HooglyBoogly), so nurbs_weight attribute will not forced assigned to converted curve. While it works, it also makes position attribute to be omitted, although I haven't seen any side-effects of this.

419

While this line is not strictly necessary, because lines 306-311 make nurbs_weight attribute to have zero value either way, by adding it I wanted to underline my intentions.

Hans Goudey (HooglyBoogly) commandeered this revision.Jun 4 2022, 1:08 PM
Hans Goudey (HooglyBoogly) edited reviewers, added: Piotr Makal (pmakal); removed: Hans Goudey (HooglyBoogly).

Thanks a lot for the updates @Piotr Makal (pmakal), the NURBS to Bezier conversion looks good now, and passes the tests!

Hans Goudey (HooglyBoogly) updated this revision to Diff 52259.Jun 4 2022, 1:10 PM
Hans Goudey (HooglyBoogly) retitled this revision from Curves: Port set type node to new data-block (WIP) to Curves: Port set type node to new data-block.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) removed a reviewer: Piotr Makal (pmakal).
Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.
  • Merge master
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jun 4 2022, 1:11 PM
Jacques Lucke (JacquesLucke) resigned from this revision.Jun 7 2022, 12:59 PM

That doesn't seem to work as expected yet:

source/blender/blenloader/intern/versioning_300.c
3025

That code may have to be moved down.

source/blender/nodes/geometry/nodes/node_geo_curve_spline_type.cc
56

point size -> number of points

74

A comment would be good. Also use 1.0f instead of 1.0.

Hans Goudey (HooglyBoogly) mentioned this in rB7974d2bff6f3: CustomData: Add function to free a named layer.Jun 7 2022, 6:01 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 52370.Jun 8 2022, 1:07 PM
Hans Goudey (HooglyBoogly) marked 2 inline comments as done.

Come cleanups/improvements to conversion to NURBS

This revision was not accepted when it landed; it landed in state Needs Review.Jun 8 2022, 3:38 PM
Closed by commit rB9e393fc2f125: Curves: Port set type node to new data-block (authored by Hans Goudey (HooglyBoogly)). · Explain Why
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rB9e393fc2f125: Curves: Port set type node to new data-block.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
2 lines
7 lines
58 lines
intern/
13 lines
84 lines
blenlib/
2 lines
blenloader/
intern/
26 lines
makesdna/
6 lines
makesrna/
2 lines
intern/
8 lines
10 lines
nodes/
geometry/
nodes/
745 lines

Diff 51515

View Options

source/blender/blenkernel/BKE_blender_version.h

Loading...
View Options

source/blender/blenkernel/BKE_curves.hh

Loading...
View Options

source/blender/blenkernel/BKE_curves_utils.hh

Loading...
View Options

source/blender/blenkernel/intern/curves_geometry.cc

Loading...
View Options

source/blender/blenkernel/intern/curves_utils.cc

Loading...
View Options

source/blender/blenlib/BLI_index_mask.hh

Loading...
View Options

source/blender/blenloader/intern/versioning_300.c

Loading...
View Options

source/blender/makesdna/DNA_node_types.h

Loading...
View Options

source/blender/makesrna/RNA_enum_items.h

Loading...
View Options

source/blender/makesrna/intern/rna_curves.c

Loading...
View Options

source/blender/makesrna/intern/rna_nodetree.c

Loading...
View Options

source/blender/nodes/geometry/nodes/node_geo_curve_spline_type.cc

Loading...