Page MenuHome

Masking: Cut Splines
Needs ReviewPublic

Authored by Marcelo Mutzbauer (1xundoredo) on Oct 25 2015, 3:18 PM.

Details

Summary

With this patch, splines can be cut in such a way that the shape of the resulting spline is the same as the one of the original spline.

Usage:

In the Clip- or Image-Editor, hit K to start the tool. As you move your mouse, you can see a line from the initial to the current mouse position. When you left-click, the spline will be cut at the places where it intersected your line and the handles will be adjusted such that the mask you end up with has the same shape as the original one.

Diff Detail

Repository
rB Blender
Branch
cut_mask

Event Timeline

  • Fix crash with deformed point array
  • Smarter choice of handle types(doesn't just blindly use free)

Also selects changed handles now

Generally an interesting patch, first round of quick review:

  • Some inlined comment
  • Why it's done in mask_add.c and not in mask_edit.c ?

More review soon.

@Sebastian Koenig (sebastian_k), @Sean Kennedy (hype) mind having a look as well?

source/blender/blenkernel/intern/mask.c
99–100

Passing tot_points_array is quite specific to be in a general utility function. It is either:

  • Proper array (aligned in size with spline->tot_point) to be passed
  • tot_points_array is to be always passed as an actual number of points (instead of using spline->tot_point)
  • Or even move this to a specific file where such a behavior is needed
Marcelo Mutzbauer (1xundoredo) planned changes to this revision.Oct 27 2015, 1:41 PM
  • Why it's done in mask_add.c and not in mask_edit.c ?

Mainly because of the mask_spline_add_point_after_index function. It also kind of 'adds new points'

Another way to work around the problems with the size of the deformed array would be to make a copy of the spline, but that also definitely doesn't look like nice thing to do.

Noticed a typo. Will fix once I have access to a machine.

The result of the cut is good, though I wonder if being able to cut multiple segments at once is really all that useful here?

We already have the ability to Ctrl+LMB on a curve to add a new point, perhaps this code could be used as a better way to calculate the new spline handles.

Of course one doesn't exclude the other (we can have knife tool _and_ improved point insertion)

User feedback on this would be good.

Marcelo Mutzbauer (1xundoredo) edited edge metadata.

Forgot if check which caused some intersection to be sorted out although they shouldn't or the other way around

  • Assign masklay->act_spline to prevent crash
  • Fix wrong handling of vector handle types
  • Code commenting
source/blender/blenkernel/intern/mask.c
99–101

So you suggest not being able to pass a negative value to let it look up the size itself?

if (tot_points_array - 1]) {
source/blender/blenkernel/intern/mask.c
99–101

Whoops...

if (point == &points_array[tot_points_array - 1]) {
  • use eMaskWhichHandle
  • Always pass the actual size of points_array for

BKE_mask_spline_point_next_bezt

@Campbell Barton (campbellbarton) Re Ctrl-Click: The portion of the code that deals with handle placement is actually quite small and basic(maybe ~50 lines, including handle type choice?). It seems like it could be used for inserting a point, however only if the user actually directly clicks on the spline. This code is not capable of (at least appropriately) handling cases where the coords of the new point are not lying on the curve which is to be split.

Absolutely agree that artist feedback would be helpful.

@Sergey Sharybin (sergey) You are referring to mask_ops.c, right?

I would love to test this out. Is there a way to get a windows build (I don't compile)?

  • Automatically animate new points and adjust animation of neighbor

handles

Marcelo Mutzbauer (1xundoredo) marked an inline comment as done.Oct 31 2015, 8:04 PM

@Sean Kennedy (hype) Sorry, haven't tried to get Blender building on Windows yet.

source/blender/editors/mask/mask_add.c
1314

Maybe it should filter out ts which are father than FLT_EPSILON away.

Something along the lines of:

if (t[i] - t[0] < -FLT_EPSILON) {
    /* first half */
}
else if (t[0] - t[i] < -FLT_EPSILON) {
    /* second half */
}
  • Adjust feathering when cutting (Hopefully last update adding new

functionality)

@Sean Kennedy (hype) Here you are(this is a 64-bit build):

source/blender/editors/mask/mask_add.c
1314

*closer than

Also, this now moved to line 1040

This is probably a bit more readable:

if (ABS(t[0] - t[i]) < FLT_EPSILON) {
    /* pass */
}
else if (t[i] < t[0]) {
    /* first half */
}
else {
    /* second half */
}

@Campbell Barton (campbellbarton) Re Ctrl-Click: The portion of the code that deals with handle placement is actually quite small and basic(maybe ~50 lines, including handle type choice?). It seems like it could be used for inserting a point, however only if the user actually directly clicks on the spline. This code is not capable of (at least appropriately) handling cases where the coords of the new point are not lying on the curve which is to be split.
Absolutely agree that artist feedback would be helpful.

@Marcelo Mutzbauer (1xundoredo)
Yep, Ctrl+LMB click on the spline already adds a point on the curve, so there is some overlap with existing functionality.

This patch does 2 things AFAICS.

  • Improved handle placement when inserting a point on an existing curve.
  • Adds a modal knife tool.

My question to users: Is the knife tool the preferred way of adding detailed points to an existing mask?

Since this patch could be modified so Ctrl+LMB close to an existing spline would add a point on it (as it does now). just with the improved handle calculation.

Said different - It doesn't make much sense if users are using the knife tool, only because it gives better subdivisions - when Ctrl+LMB is a faster, more direct way to add single points. Probably users need to try the tool out before answering though :).

  • Better handles for Ctrl+click
source/blender/editors/mask/mask_add.c
436

Should be renamed to select_neighbor_handles

518

No need to assign point->uw[i].w

@Sean Kennedy (hype) Here you are(this is a 64-bit build):

This is actually pretty cool. The standard CTRLclick way is nice to add points and adjust the new handles, while this new way is great for adding in a point that you want to retain the current animation until you need to change it specifically.

On a quick test, it seemed to not work perfectly with a mask that was parented to a track. It added the new points in the position that the mask would have been if it wasn't parented.

  • Parenting
  • Fix stupid #if 1 instead of #if 0 which disabled the better adjustment of ctrl+lmb handles(Can still be removed easily btw, just as I did in the last update ;))
  • Change name of select_neighbors to select neighbor_handles
  • Get rid of uneccessary assignment of w

@Sean Kennedy (hype) Thank you for the report. Mask points are parented individually. The solution I found was to check whether both neighbor points have the same parent. If they do, the new point is also going to be parented to the same track/plane. If they don't the new point won't be parented at all. Does that look like a good solution?

Marcelo Mutzbauer (1xundoredo) marked 2 inline comments as done.Nov 3 2015, 10:00 AM

@Sean Kennedy (hype) Thank you for the report. Mask points are parented individually. The solution I found was to check whether both neighbor points have the same parent. If they do, the new point is also going to be parented to the same track/plane. If they don't the new point won't be parented at all. Does that look like a good solution?

That sounds like a great solution to me.

  • Fix some parenting bugs
  • Yet another parenting fix

@Marcelo Mutzbauer (1xundoredo): Would you be able to split this patch?

  • improvements to handle calculation (better point insertion for existing).
  • modal knife tool.

The first patch can likely go into master since it's an obvious improvement (just some final review & sanity check).
The knife tool is then a much smaller patch, and we can further check functionality with users too.

Parenting turned out to be even more 'problematic'. If both neighbors have the same parent at the same frame, there shouldn't be any problems.(Technically it has to be the initial position where the spline points has been parented, which is the case on the same frame)
If they don't - it just assumes parenting the newly created point is not possible(though parenting the same point track on different frames should be). If that is the case(whether it is because of different track points or because of different frames), it currently places the new point in respect to its neighbors' positions after the parenting(without being parented itself). This is done on every keyframe.

@Campbell Barton (campbellbarton) With this patch ctrl lmb would loose a lot of flexibility/not do what it's supposed to because it...

  • ...adds in a point with an 'aligned' handle type.
  • ...doesn't give you the capability to adjust the new handle which has been created for you(at least not directly).
  • ...changes the handle types of its neighbors to 'aligned'(or 'free' if they already are).

Splitting this patch should not be that hard to do(at least not in its current state). The only one thing that might look confusing is that the function that does the split is capable of doing up to 3 splits(3 intersections possible on a bezier segment)

  • Add accidentally removed line back in
  • Clean up mask_spline_split_spline_point
  • Check against FLT_EPSILON when preparing for the next recursive call
Marcelo Mutzbauer (1xundoredo) marked an inline comment as done.Nov 4 2015, 11:18 AM

@Campbell Barton (campbellbarton) What about only using the improved calculation in case both neighbors already are doubleside aligned/free and falling back to old behavior otherwise? If both neighbors are free it could then make the newly inserted point free, although that would not be necessary to achieve the same shape.

How should I split the patch? I could upload two raw diffs, however that doesn't sound convenient to review.

source/blender/makesdna/DNA_mask_types.h
108

This var doesn't seem to contain any flags(as the comment suggests) but just stores framenr of the Shape? Maybe someone could clarify here.

@Marcelo Mutzbauer (1xundoredo), seems reasonable

@Campbell Barton (campbellbarton) What about only using the improved calculation in case both neighbors already are doubleside aligned/free and falling back to old behavior otherwise? If both neighbors are free it could then make the newly inserted point free, although that would not be necessary to achieve the same shape.

Seems like a reasonable way to handle it, anyone else have a preference?

How should I split the patch? I could upload two raw diffs, however that doesn't sound convenient to review.

Theres no special feature on phabricator to do this. - would just be another differential.
It wouldn't need a lot of review, just final check that everythings ok.

  • Only use improved handle calculation when both neighbors are doubleside-aligned/free
  • Fix parenting bug
  • conventions: use u instead of t
  • cleanup split function
source/blender/editors/mask/mask_add.c
175

Does this also fix the todo above?

Version without knife(only improved calculation):

So I should close this revision and create new ones?

One problem remains though: Currently(in master) you are able to adjust the newly created handles manually by keeping your mouse button pressed and moving your cursor. With this patch this cannot be done anymore because it calculates the handles for you.

source/blender/editors/mask/mask_add.c
438

Should temporarily store undeformed points as this could fail with Plane Track parenting.
Same for function below.