GSoC 2016 - Bezier Curves Improvements
Needs ReviewPublic

Authored by João Araújo (genio84) on Jan 4 2017, 10:07 AM.

Details

Summary

For GSoC 2016 I added some new tools to work with Bezier Curves.

This commit implements the following features

  • Extend tool
  • Trim tool
  • Offset Tool
  • Chamfer Tool
  • Fillet Tool
  • Improved curve extrusion

Every piece of feedback is welcome!

Windows test build:
http://graphicall.org/1186

Mac test build:
http://graphicall.org/1185

Proposal:
https://wiki.blender.org/index.php/User:Genio84/gsoc2016/proposal

Reports:
https://wiki.blender.org/index.php/User:Genio84/gsoc2016/reports

End User Documentation:
https://wiki.blender.org/index.php/User:Genio84/gsoc2016/final_report/

Working Branch Used for Development:
https://developer.blender.org/diffusion/B/browse/gsoc2016-improved_extrusion/

Diff Detail

João Araújo (genio84) retitled this revision from to GSoC 2016 - Bezier Curves Improvements.Jan 4 2017, 10:07 AM
João Araújo (genio84) updated this object.
João Araújo (genio84) set the repository for this revision to rB Blender.
João Araújo (genio84) updated this object.

Just some initial review from the usage level (without going into the code).

Bevel

Not really happy about two extra options for such a little thing here. Can at least get rid of revered direction and allow negative values for the extrude?

Extend

Extend tool works totally fuzzy when having multiple points selected. For example, could not reproduce the example with "circle". When there are more than one point selected the tool ruins unselected poitns coordinates as well.

Batch Extend

Don't think it's a good name and tool. Usually all the tools are operating on the all selection, so implicitly they are all Batch. Not sure why this should be an exception.

The behavior you're achieving with multiple selection and Extend tool sounds more like a separate tool actually since it does something different.

Trim

Works in simple cases, fails when intersection point is exactly on the spline.

Offset

Is it supposed to keep constant distance of the new spline from the existing one?

Chamfer

Not sure the name is really good, it's totally different from similar tool for meshes.

Couldn't get it to work either, it just ruins the whole spline.

Fillet

Shouldn't it be merged with Chamfer tool and have a slider about how much "smoothness" you want?

In any case, also couldn't make it to work, it just ruins the whole spline.

Thank you very much for the throughout review, Sergey!

Just some initial review from the usage level (without going into the code).

Bevel

Not really happy about two extra options for such a little thing here. Can at least get rid of revered direction and allow negative values for the extrude?

Extend

Extend tool works totally fuzzy when having multiple points selected. For example, could not reproduce the example with "circle". When there are more than one point selected the tool ruins unselected poitns coordinates as well.

Extend tool is only supposed to work up to two points selected. You should have gotten a warning when you tried to select more than that. Could you send me the file where you failed to reproduce the example?

Batch Extend

Don't think it's a good name and tool. Usually all the tools are operating on the all selection, so implicitly they are all Batch. Not sure why this should be an exception.

When you have two points selected, you can either extend them independently of each other, or you can extend them up to their intersection. The extend tool extends up to the intersection, while batch extend extends independently. Perhaps renaming "Batch Extend" to "Extend" and "Extend" to "Intersect" would be better?

The behavior you're achieving with multiple selection and Extend tool sounds more like a separate tool actually since it does something different.

As I have said that should not have even run.

Trim

Works in simple cases, fails when intersection point is exactly on the spline.

Could not reproduce. Could you send me an example file?

Offset

Is it supposed to keep constant distance of the new spline from the existing one?

Unfortunately not. Only constant distance between control points.

Chamfer

Not sure the name is really good, it's totally different from similar tool for meshes.

Now I am curious! I did not know there was a mesh chamfer. Tried to look for it, but could not find it. I imported the terminology from CAD software, where the Chamfer tool does what I implemented.

Couldn't get it to work either, it just ruins the whole spline.

Example please?

Fillet

Shouldn't it be merged with Chamfer tool and have a slider about how much "smoothness" you want?

By definition they are different operators. Engineers/Architects use one or the other, intermediate stages are useless.

In any case, also couldn't make it to work, it just ruins the whole spline.

Example?

Once again thank you very much for the review! Will be waiting for your response :)

As architect I'm used to fillet/chamfer as separate tools too, but in Blender bevel tool gives really great artistic freedom. Having "profile" option in combined fillet/chamfer would reassemble blender bevel and is really nice addition despite of not being used in cad software.

In D2445#57872, @Sergey Sharybin (sergey) wrote:
Not really happy about two extra options for such a little thing here. Can at least get rid of revered direction and allow negative values for the extrude?

That would seem more useful and flexible indeed

Perhaps renaming "Batch Extend" to "Extend" and "Extend" to "Intersect" would be better?

Those names do sound better

In D2445#57872, @Sergey Sharybin (sergey) wrote:
Works in simple cases, fails when intersection point is exactly on the spline.

Couldn't get it to work either, it just ruins the whole spline.

Some operators are mostly mean to run on 2D curves and some times fail or yield unexpected results on 3D curves. Could it be that you were running them on the later?
If that is the case maybe restrict operators to running in 2D curves exclusively with a warning pop-up similar to the one implemented for some of the other operators?

Combining the Fillet and Chamfer into one operator with a profile slider would also be a nice option. It may be less "cad-like" but it would probably integrate better with Blender workflow and other similar tools like the Mesh Bevel

Hi @João Araújo (genio84) as promised, here is a review of the operators from a user perspective (just realized that I noted everything down but never actually posted it)

Extrude
Very useful addition!
Would eliminate the two checkboxes though: Imho this should be combobox -> Bidirectional Extrude | Unidirectional Extrude upwards | Unidirectional Extrude downwards

Extend
The limitation on the XY plane is cumbersome...this did work sometimes in my tests, sometimes not. I assume it's because it is limited to 2D operations, but you will get countless bug reports saying "it does not work" when this is committed as it is.

Redo Operator: Extrapolate properties should be always true. It's the expected behaviour

Trim
Immediate crashes on my side, no matter if I select one or two points (just added a circle and a spline that crosses it [in the same object] - so I could not test it.

Offset
Super useful! Should have an option to equalize the offset on every point of the spline. In my tests, the offset geometry was oftentimes not consistently spaced in relation to the original spline.

Chamfer
This didn't work on closed splines - which I expected it to... why is that restricted to open splines?

Best regards,
Thomas

In D2445#57872, @Sergey Sharybin (sergey) wrote:
Not really happy about two extra options for such a little thing here. Can at least get rid of revered direction and allow negative values for the extrude?

That would seem more useful and flexible indeed

I overlooked that comment. Believe me when I say I tried to do it! But for some reason the input would not accept negative values! I'll give it another shot.

Combining the Fillet and Chamfer into one operator with a profile slider would also be a nice option. It may be less "cad-like" but it would probably integrate better with Blender workflow and other similar tools like the Mesh Bevel

Could you please provide an example of what this would look like?

Hi @João Araújo (genio84) as promised, here is a review of the operators from a user perspective (just realized that I noted everything down but never actually posted it)

:-)

Extend
The limitation on the XY plane is cumbersome...this did work sometimes in my tests, sometimes not. I assume it's because it is limited to 2D operations, but you will get countless bug reports saying "it does not work" when this is committed as it is.

Redo Operator: Extrapolate properties should be always true. It's the expected behaviour

Are you saying that the operator should also work on the XZ and YZ planes, or it should work in general 3D space? In the later case, it is pretty close to impossible. You could argue it should use the view plane, but at least from my experience with Blender that can also lead to unexpected results. This would not probably be a problem if User Coordinate Spaces existed.

Trim
Immediate crashes on my side, no matter if I select one or two points (just added a circle and a spline that crosses it [in the same object] - so I could not test it.

I will need more info to squash that one. Could you run Blender under a debugger of some sorts so you can catch the state of Blender when it crashes?

Offset
Super useful! Should have an option to equalize the offset on every point of the spline. In my tests, the offset geometry was oftentimes not consistently spaced in relation to the original spline.

From what I understand, that requires re-implementing the whole operator using a different offsetting algorithm. I picked the algorithm considering the trade-off "time required to implement" vs "accuracy". 100% accuracy is impossible to achieve with Bezier curves. The algorithm I used gave nice visual results.

Chamfer
This didn't work on closed splines - which I expected it to... why is that restricted to open splines?

Yes, I expected the same..... Could you please send me a file where it failed?

Thank you very much for all your comments!

Small comment, also it looks like there are several comments that should be on a new line rather than having a really long line.

release/scripts/startup/bl_ui/space_view3d.py
2769–2780

Why is the commented out?

In D2445#58166, @João Araújo (genio84) wrote:
I overlooked that comment. Believe me when I say I tried to do it! But for some reason the input would not accept negative values! I'll give it another shot.

! In D2445#58159, @Thomas Beck (plasmasolutions) wrote:
Would eliminate the two checkboxes though: Imho this should be combobox -> Bidirectional Extrude | Unidirectional Extrude upwards | Unidirectional Extrude downwards

Maybe plasmasolutions suggestion would be more reasonable, a combobox would be more visually tidy and an acceptable solution

In D2445#58166, @João Araújo (genio84) wrote:
Could you please provide an example of what this would look like?

This is how the Curve Bevel Addon currently works.

Here is some more information for the issues i've mentioned.

Trim tool

File:

Steps: With this file run Trim tool (i do it from spacebar menu) and the error will appear: Cyclic splines needs at least two intersections

Chamfer tool

File:


Steps: Call Chamfer tool from spacebar menu.
This is result i've got:

This is something which might be involved here (and might be a real bug in master branch):

1​==1837== Source and destination overlap in memcpy(0x17316258, 0x17316210, 144)
2​==1837== at 0x72F65F3: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
3​==1837== by 0xF87134: ED_curve_beztcpy (editcurve.c:6215)
4​==1837== by 0xF92723: curve_chamfer_invoke (editcurve.c:8452)
5​==1837== by 0xC718C7: wm_operator_invoke (wm_event_system.c:1099)
6​==1837== by 0xC720F0: wm_operator_call_internal (wm_event_system.c:1330)
7​==1837== by 0xC721A3: WM_operator_name_call_ptr (wm_event_system.c:1342)
8​==1837== by 0x102CB00: operator_call_cb (interface_templates.c:3300)
9​==1837== by 0xFE4615: ui_apply_but_funcs_after (interface_handlers.c:790)
10​==1837== by 0xFFDBEC: ui_popup_handler (interface_handlers.c:10137)
11​==1837== by 0xC7008C: wm_handler_ui_call (wm_event_system.c:451)
12​==1837== by 0xC73B9D: wm_handlers_do_intern (wm_event_system.c:2058)
13​==1837== by 0xC73E39: wm_handlers_do (wm_event_system.c:2142)
14​==1837==

Fillet tool

File:


Steps: Call Fillet tool from spacebar menu.
This is result i've got:

I've got similar memcpy issue here, so might be same root problem as Chamfer.

Rest of discussion would need closer attention which i'll do a bit later :)

release/scripts/startup/bl_ui/space_view3d.py
2769–2780

My original plan was for those tools to be callable from the Curve Specials menu. However, dphantom pointed out that operators called that way did not run modal. I left those lines as comments to remind me where they should go, but they will either be deleted or uncommented soon. I still need to discuss that with users.

This is how the Curve Bevel Addon currently works.

I see. The problem with that solution is that the approximation to a "real fillet" (best approach to a circle) can only be as good as the artists' eye. Chamfers become trivial though, but I'm guessing I will no longer be able to allow you to control the angle.

Here is some more information for the issues i've mentioned.

Trim tool

File:

Steps: With this file run Trim tool (i do it from spacebar menu) and the error will appear: Cyclic splines needs at least two intersections

I can confirm this one. Will take a look and get back to you :)

Chamfer tool

File:


Steps: Call Chamfer tool from spacebar menu.

You attached the same file as in the trim case :)

Fillet tool

File:


Steps: Call Fillet tool from spacebar menu.
This is result i've got:

I've got similar memcpy issue here, so might be same root problem as Chamfer.

I didn't get any unexpected behavior. Worked perfectly on my computer, with no memcpy issues. I updated my repo to latest master and still got no problems.

Ooops. You can use fillet file to reproduce chamfer issue actually.

As of the issue, we have to replace memcpy with memmove anyway i think and then things starts to kind of work but the result is still not really correct. So try with memmove i guess? Also, what's your OS?

Ooops. You can use fillet file to reproduce chamfer issue actually.

Same thing. Ran fine on my computer.

As of the issue, we have to replace memcpy with memmove anyway i think and then things starts to kind of work but the result is still not really correct. So try with memmove i guess? Also, what's your OS?

I use Mac OS X 10.9.5. I'll edit all memcpy calls in "ED_curve_beztcpy" to memmove's and see if it still works.

I see. The problem with that solution is that the approximation to a "real fillet" (best approach to a circle) can only be as good as the artists' eye. Chamfers become trivial though, but I'm guessing I will no longer be able to allow you to control the angle.

In that addon a bevel factor 1 corresponds to a "perfect circular bevel" or as close as bezier curve permits, and like mesh bevel a factor of 0 corresponds to a "flat bevel" or a chamfer.

Given that the current chamfering tool provides additional functionality like asymmetry/angle control I'd leave it as is, or maybe rename "Fillet" to "Bevel".
If the bevel could have an additional "factor" slider that would be great, but I guess additional features should not be a priority right now.

Steps: With this file run Trim tool (i do it from spacebar menu) and the error will appear: Cyclic splines needs at least two intersections

Makes sense, one can't cut a close curve without two specific intersections, otherwise its unclear where to place the gap.

In your example case I got the same result, I am guessing it is cause by perfectly overlapping vertex. Both vertex from the "open curve" are perfectly overlapping both the closed curve and two of its vertex which probably causes detection or precision problems.

This is a common problem in most software, even in tools and CAD software other than Blender. If one moves the open curve vertex slightly outwards it works as expected.

Hey everyone. As I said in the mailing list, I have been extremely busy lately, and didn't have the time to work on this.
Here's what I can tell you:

  1. Improved extrusion - You suggested allowing negative values for the extrude value, or using a combo box. Regarding the negative values, I blame my failure on these lines of code
On file "rna_curve.c"

RNA_def_property_ui_range(prop, 0, 100.0, 0.1, 3);
RNA_def_property_range(prop, 0.0, FLT_MAX);
On file "rna_curve_gen.c"

void Curve_extrude_set(PointerRNA *ptr, float value)
{
	Curve *data = (Curve *)(ptr->data);
	data->ext1 = CLAMPIS(value, 0.0f, FLT_MAX);
}

I tried changing this to

void Curve_extrude_set(PointerRNA *ptr, float value)
{
	Curve *data = (Curve *)(ptr->data);
	if (data->flag & CU_UNI_EXTRUDE) {
		data->ext1 = CLAMPIS(value, FLT_MIN, FLT_MAX);
	}
	else {
		data->ext1 = CLAMPIS(value, 0.0f, FLT_MAX);
	}
}

But I would guess the "gen" in "rna_curve_gen.c" means "generated", which would explain why after compiling my changes simply disappeared. I am much more fond of the negative values solution, so if someone could clarify this I would be grateful.

  1. General bugs - I was unable to reproduce them, with the exception of the one with the trim tool.
  1. Bug with the trim tool - It's not really a bug. In my computer it "runs" the operator, but has no noticeable effect. Furthermore, I can't think of a single use case where an artist would need to trim a spline which is exactly on top of the spline they want to trim to, because that's what the trim tool does.
  1. I have renamed "Extend" to "Intersect" and "Batch Extend" to "Extend".
  1. I edited the memcpy call in "ED_curve_beztcpy" to memmove. Noticed no difference.

I am not sure of what else I can do.

  1. I have renamed "Extend" to "Intersect" and "Batch Extend" to "Extend".

Cool makes sense, and seems tidier now! I'm no coder, can't really comment on the rest, so lets see what the other devs say about it.
Thanks for working on it, and Happy Easter to whoever celebrates it :)

@João Araújo (genio84) Can you update the diff with your latest fixes for review? The version here is from january.

@João Araújo (genio84) Please split the diff per tool. It will be hard to get a quality review for all of them before 2.79. It will allow to commit some of them independently.

@João Araújo (genio84) Please split the diff per tool. It will be hard to get a quality review for all of them before 2.79. It will allow to commit some of them independently.

The only way I am seeing of doing that is by creating a new branch, copying and pasting the code which refers to each tool to that branch, and make individual diffs. Please tell me there is a better way.

@João Araújo (genio84) Please split the diff per tool. It will be hard to get a quality review for all of them before 2.79. It will allow to commit some of them independently.

The only way I am seeing of doing that is by creating a new branch, copying and pasting the code which refers to each tool to that branch, and make individual diffs. Please tell me there is a better way.

That is probably the easiest, if your commit history has commits mixed for different tools. Otherwise cherry-picking into new branches could work.

You can also use a mix. As Jesterking said, you can cherry-pick all commits related to one tool only. Then you only have a few commits related to several tools that can be copy-pasted by hand.

Note that instead of manually cherry-picking, you can also call "git rebase -i master" to rebase your work on top of master. It will create a file containing all the commit hashes and comments. You can then reorder and squash those commits by tool category, then save the file (line starting with p will pick (like cherry-pick), s will squash with previous commit, f will fix previous commit). Both s and f will create one commit out of many. See https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History.