Details

Reviewers

Summary

Hi there, the past days have been working on this to see if I can understand a little about blender. So, here it is my first try for #T45231 .

I have made some manual things like keeping the track of selected points in the bezier and then just unselecting them, if there is a better way or a call that I can make, then the patch size would go down in lines,

So it has some bugs, like when multiple and moving, some of the control points doesn't move only the center. I also don't know how to test the path for that is not BEZIER aka bp.

Is something like this what T45231 is about or I'm in the wrong path?

Diff Detail

Repository: rB Blender

Event Timeline

tyoc213 (tyoc213) updated this revision to Diff 4796.Aug 2 2015, 6:22 PM

tyoc213 (tyoc213) retitled this revision from to WIP T45231.

tyoc213 (tyoc213) updated this object.

tyoc213 (tyoc213) added a reviewer: Campbell Barton (campbellbarton).

tyoc213 (tyoc213) set the repository for this revision to rB Blender.

tyoc213 (tyoc213) added a project: Good First Issue.

General notes on code formatting:

first, please read the code style guidelines (mostly spaces around operators, else (if) { on new lines, full float format (i.e. 0.0f not 0)...)
function names should be in English, as well as comments and variable names ;)
there's no need to forward declare functions only used in the implementation file, just declare them as static, e.g. static void foo()
there should only be one line between functions or their declarations in header files, so remove all those extra lines ;) but add an extra line between variables declaration and the body of the function so it doesn't look like a wall of text.
variable declaration should happen before code (I think we use C89 or C90 standard)

Actual review of the functionality and implementation will be handled by someone else (Campbell most likely), but formatting the code according to style guidelines is a must have so please do so. (It seems to be on track with what the quick hack is about, though.)

source/blender/editors/curve/curve_intern.h
135	Extra line.
source/blender/editors/curve/curve_ops.c
143	Extra line.
294	Extra line.
source/blender/editors/curve/editcurve.c
6029	No need for forward declaration, just make it static.
6030	Opening brace on a new line, also function name should be in English.
6034	Dead code should be removed.
6044–6046	could use `copy_v3_v3(&selectedList[idx * 3], bezt->vec[1]);`
6079	Unused.
6122	Unused.
source/blender/editors/curve/editcurve_select.c
1732–1739	use `len_squared_v3v3(a, b)` from BLI_math
1752–1753	Either directly assign values to mval, e.g.: `const float mval_f[2] = { event->mval };` or move the assignment after the variable declarations.
1754–1760	Variable name should be in lower case, with underscore between words, e.g. `bezt_next`.
1769	You can use `MEM_callocN` and `MEM_freeN` for dynamic memory (de-)allocation.
1776	FLT_MAX?

This revision now requires changes to proceed.Aug 2 2015, 8:02 PM

Kévin Dietrich (kevindietrich) retitled this revision from WIP T45231 to Rip edge-move for curves..Aug 2 2015, 8:04 PM

Kévin Dietrich (kevindietrich) edited edge metadata.

Kévin Dietrich (kevindietrich) added a task: T45231: Rip edge-move for curves.

I did put Curve_OT_rip after subdivide becuase it uses subdivide call inside, or should I let it at the end like it is on editcurve.c?

And I was wondering why did you have so many statics... and if I should declare my functions inside the .h.

For name and parameter namming, I think it should be make more specific if name_of_this is for functions and parameters or also to local varibles, wich I see as camelized, for local variables it says straight and to the point but not in what format (it is a little ambiguos).

3.A I see "Variable name should be in lower case, with underscore between words, e.g. bezt_next." I thought it was camelized, but it is not explicit in the naming convention. Also it doesnt say nothing against namemyvar, example there is: obedit and bezt or bp.

BPoint stands for what exactly? bezier point? and BezTriple to bezier triple?

reversePointerCount = nurb->pntsu * nurb->pntsv; // ???: this is correct?

it doesn't say nothing against } else { ???

So I think now I follow the major parts of the convention, also deleted some "todos" (like the check if the checks inside `select_adjacent_mouse``` are OK)

I just found `if (equals_v3v3(&search_list[idx*3], point)) {``

and I wonder if I can do some like:

bezt->f1 = bezt->f2 = bezt->f3 &= ~SELECT;

just kidding, anyway, waiting for next review to see where this is going.

Downloaded the blender dev tools and corrected my range of lines, the count of the lint for my places is 0 now.

I checked the path of "path" so I fixed my errors there (was only testing with bezier).

The only real problem I see now is that when the point in a bezier is added for example select 1 point in between a bezier

pt1 > here (in the back arrow from a point works OK)
here < pt1 (in the forward arrow one handle is pinned at the end of operation)

I don't know why this is happening, so any insight would be helpfull.

From the raw diff looks like you are using spaces instead of tabs for most (all?) of this. Please switch to tabs :) You'll also likely want to set up you editor for tabs.

Kevin Mackay (yakca) added inline comments.Aug 3 2015, 10:50 PM

source/blender/editors/curve/curve_ops.c
171	Duplication of lines 160-162. In fact, is any of this change subset needed?
source/blender/editors/curve/editcurve.c
6207	On line 6146, you define selected_list and have selectedList here, and there is search_list elsewhere. New code should be consistent in style, and follow same case style of the file it is being added to.

As like Kévin, I've posted some clean up suggestions. I'll check back in here later in the week when I have a bit more time.

source/blender/editors/curve/editcurve.c
6297	Long variable name for what is effectively an index :)
6302	This is stylistic, but you could rewrite a lot of your while loops as more succinct for loops. E.g. for (i = 0, bezt = nurb->bezt; i < nurb->pntsu; ++i, ++bezt) {} IMO for is more readable.
6311	To answer q5, this is correct for handling NURBS curve and surfaces, if not using surfaces then the multiply is redundant. It is actually correct Bezier too, so you could move before this the if nurb->type check.

I will check your comments, haven't seen there where more... also I see now that there is a "done" and a "unused" thing I guess is to mark before update the diff?

In this web the comments are put last first??? or how is the order? also the tab is only for first indent isn't? I was following "New code should be consistent in style, and follow same case style of the file it is being added to"... but I did also read the guidelines for writing code (my "normal style" is more like the original if you see the first diff, I already changed it to be like in the developer guidelines... so what is?)

Corrected tabs, moved some initializations before curb check, let the names of variables as name_my_var (or should I change back to nameMyVar?).

For the next revision I will change the whiles(a--) to for if it is really needed (I'm between do things like say the file or like we say xD or the wiki docs say).

tyoc213 (tyoc213) marked an inline comment as done.Aug 4 2015, 4:11 AM

tyoc213 (tyoc213) added inline comments.

source/blender/editors/curve/curve_ops.c
171	Are you referring to the above lines or the below lines? or are you referring that I did take as example the lines in 160? or the whole change in itself? I don't understand?
source/blender/editors/curve/editcurve.c
6207	I will stick for the moment to name_here_this, because is the one linked, also because there was already a comment of kevindietrich Variable name should be in lower case, with underscore between words, e.g. bezt_next. So I changed all the camels there.
6302	I just did follow/copy how it was coded from the above operations/functions, for one that I was having problems I used a for, but changed the name from a to this.
6311	In my last commit (before read this) I deleted nurb->pntsu * nurb->pntsv; and only let `nurb->pntsu``` then I guess I should make it again that multiplication? Ir it is more that because both can use only pntsu I can make initialize this count before?? (will try this)

I was going to write a little more large post but only do this (without this modification):

Add a bezier curve, go edit, don't unselect the 2 vertices, then CTRL+Click, then extend selection by 1 (you have now all the 4 points selected) and subdivide, now move and you see now an added control point that doesn't move with the selection and also an extra vertex (quasi same behavior as this implementation, only that it only select automatically the new points and deselect the originals). So this (for me bad behavior) also happen on normal blender... so should I report it as error?(hope I explained that if you start selecting random points in a curve and subdivide and move and again, sooner or later, one handle will not move when grabbing after subdivide) or try to solve it some way?

So I will need to redo this task (well it will be almost the same, I only need to reorganize things in other way and only do it in one operator), not with a chain of 2 operators but only one and do one subdivision at a time (not all at the same time, will be something like a foreach with pair of points7subdivide then select all new points) for not have this extra 3rd cut..

And:

I think it is correct use f1 (control), f2 (center), f3(control) to turn on/off selection? but the next has any value to this code in some way or notification?

		WM_event_add_notifier(C, NC_GEOM | ND_SELECT, cu/*obedit->data*/);
		BKE_curve_nurb_vert_active_validate(cu/*obedit->data*/);

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:18 AM

Rohan Rathi (RohanRathi) removed a task: T45231: Rip edge-move for curves.Mar 21 2017, 4:46 PM

Rip edge-move for curves.
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 4796

source/blender/editors/curve/curve_intern.h

source/blender/editors/curve/curve_ops.c

source/blender/editors/curve/editcurve.c

source/blender/editors/curve/editcurve_select.c

Rip edge-move for curves.Needs ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 4796

source/blender/editors/curve/curve_intern.h

source/blender/editors/curve/curve_ops.c

source/blender/editors/curve/editcurve.c

source/blender/editors/curve/editcurve_select.c

Rip edge-move for curves.
Needs ReviewPublic
Actions

Revision Contents
Changeset List