Details

Reviewers

Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)

Commits

rBf674976edd88: Curve: Remove 'CU_2D' flag used for nurbs

Summary

The CU_2D flag is set for nurbs, so a Curve can have 2D nurbs mixed with 3D.

But the UI does not allow this mixing. It updates all nurbs to 2D or 3D when set.

This patch proposes to remove this specific flag for nurbs.

This may break old files, since 2D curves with mixed 3D are now set as 3D.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D10738 (branched from master)

Build Status

Buildable 13656
Build 13656: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Mar 16 2021, 8:15 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B13549: Diff 35273.Mar 16 2021, 8:15 PM

+1 for not mixing 2D and 3D curves in the same data, they're really not the same at all and combining them is just confusing.

Edit: Not mixing them would also allow making a greater distinction between the two data types in the future. For example, there could be a "2D boolean" that only worked on 2D curve data.

Not sure why I was added as reviewer... Would rather get @Sergey Sharybin (sergey)'s view on this topic first.

The code and UI should indeed be in a consistent state.

Don't think old files breakages will be that much of a problem: from my understanding this is ~2.4 code, which then got changed in 2.5 where the interface was only exposing curve-ID-level setting.

The code itself needs more work, to make it more clear. General notes:

There are some not-so-obvious functions. Add a comment about when and which function is to be used.
Some of the inlined check needs more explanation as they look inversed. Maybe with some more clear API it can be solved naturally (maybe update_nurbs_after_dimension_change?)

I wouldn't consider myself as a blocking reviewer. Those reviews anyone from the module can do and currently I have very limited time for such work.

source/blender/blenkernel/BKE_curve.h
89–90	What's the difference between those? When they are to be used, and which one?
source/blender/blenkernel/intern/curve.c
2709–2711	Such change degrades readability of code: you are replacing something which has clear semantic naming with a confusing inlined check.
source/blender/editors/space_view3d/view3d_buttons.c
1034–1037	`(cu->flag & CU_3D) == 0` means the curve is 2D, so why the code does 3D-to-2D?

Germano Cavalcante (mano-wii) added inline comments.Mar 22 2021, 3:54 PM

source/blender/blenkernel/BKE_curve.h
89–90	This was a utility created to replace `BKE_curve_curve_dimension_update` with no change in behavior. Basically it is a `BKE_curve_dimension_update` that affects only 2D curves. But I see that it can be easily replaced.

Comment in order to compensate the readability degradation due to the removal of the CU_DO_TILT macro
Use macro to indicate when the curve is 2D (improves readability)
Replace BKE_curve_dimension_update_2d with BKE_curve_dimension_update with the previous CU_IS_2D(cu) check
Rename BKE_nurb_3d_to_2d to BKE_nurb_project_2d since the curve is marked as 2D even though it affects 3D space

Harbormaster completed remote builds in B13656: Diff 35473.Mar 22 2021, 3:55 PM

Fix wrong curve 2D check

Harbormaster completed remote builds in B13657: Diff 35475.Mar 22 2021, 4:08 PM

Fix another wrong curve 2D check

Harbormaster completed remote builds in B13658: Diff 35476.Mar 22 2021, 4:10 PM

Campbell Barton (campbellbarton) accepted this revision.Apr 1 2021, 1:58 PM

This revision is now accepted and ready to land.Apr 1 2021, 1:58 PM

Closed by commit rBf674976edd88: Curve: Remove 'CU_2D' flag used for nurbs (authored by Germano Cavalcante (mano-wii)). · Explain WhyApr 1 2021, 3:55 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBf674976edd88: Curve: Remove 'CU_2D' flag used for nurbs.

Fix T86440: Transforming 2D curves as if they were 3D
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 35473

source/blender/blenkernel/BKE_curve.h

source/blender/blenkernel/intern/curve.c

source/blender/blenkernel/intern/curve_convert.c

source/blender/blenkernel/intern/displist.c

source/blender/blenkernel/intern/font.c

source/blender/blenlib/intern/freetypefont.c

source/blender/blenloader/intern/versioning_250.c

source/blender/blenloader/intern/versioning_290.c

source/blender/editors/curve/curve_intern.h

source/blender/editors/curve/editcurve.c

source/blender/editors/curve/editcurve_add.c

source/blender/editors/curve/editcurve_paint.c

source/blender/editors/object/object_add.c

source/blender/editors/object/object_data_transform.c

source/blender/editors/space_view3d/view3d_buttons.c

source/blender/editors/transform/transform_convert_curve.c

source/blender/editors/util/ed_transverts.c

source/blender/makesdna/DNA_curve_types.h

source/blender/makesrna/intern/rna_curve.c

Fix T86440: Transforming 2D curves as if they were 3DClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 35473

source/blender/blenkernel/BKE_curve.h

source/blender/blenkernel/intern/curve.c

source/blender/blenkernel/intern/curve_convert.c

source/blender/blenkernel/intern/displist.c

source/blender/blenkernel/intern/font.c

source/blender/blenlib/intern/freetypefont.c

source/blender/blenloader/intern/versioning_250.c

source/blender/blenloader/intern/versioning_290.c

source/blender/editors/curve/curve_intern.h

source/blender/editors/curve/editcurve.c

source/blender/editors/curve/editcurve_add.c

source/blender/editors/curve/editcurve_paint.c

source/blender/editors/object/object_add.c

source/blender/editors/object/object_data_transform.c

source/blender/editors/space_view3d/view3d_buttons.c

source/blender/editors/transform/transform_convert_curve.c

source/blender/editors/util/ed_transverts.c

source/blender/makesdna/DNA_curve_types.h

source/blender/makesrna/intern/rna_curve.c

Fix T86440: Transforming 2D curves as if they were 3D
ClosedPublic
Actions

Revision Contents
Changeset List