Page MenuHome

Add curve decimate in the graph editor
ClosedPublic

Authored by Sebastian Parborg (zeddb) on May 10 2019, 5:56 PM.

Details

Diff Detail

Repository
rB Blender

Event Timeline

I didn't have time to implement it when I wrote this first draft, but I would like to add the same kind of logic as with "Relax pose".
IE when the operator is invoked, the cursor is grabbed and the location in the viewport determines the ratio of the decimation.
So the left side of the screen would be a ratio of 0 while the right would be a ratio of 1.

The 'ratio' parameter seems to influence the percentage of points to keep/remove. Is this really a useful metric? Wouldn't it be better to manipulate the allowed error instead?

source/blender/editors/animation/keyframes_general.c
336

Unused parameter ‘ac’.

341

I would always write full sentences, so "Check if there are any points." or something like that.

342

No need for parentheses around the equality checks.

source/blender/editors/space_graph/graph_edit.c
1330

Shouldn't this be done in a poll function?

1350

I don't quite understand what "the ratio keyframes" means.

1362

A ratio between 0 and FLT_MAX is weird. How can you remove more than 100% of the points?
It's also unclear what the ratio indicates. What would ratio=0.2 do? Remove 20% of the points? Or keep 20% of the points?

I've tried it with the default ratio, and it removed all the points except the first and last one, even though there were quite big jumps in the curve.

source/blender/editors/space_graph/graph_edit.c
1330

This is how it is done in the other exec functions in this file.
Should we change this?

Agree we could use a better metric here, especially if you're editing multiple curves at once a ratio is not useful.

source/blender/editors/space_graph/graph_edit.c
1330

The poll function already does it.

Strictly speaking, you could assume that this never returns 0. But still I think it's good practice to always check the return value of this function in case future refactoring breaks that assumption.

Agree we could use a better metric here, especially if you're editing multiple curves at once a ratio is not useful.

Sybren and I discussed this on thursday and came to the conclusion to ask the artists for input.
When I spoke to them, they wanted a ratio. So they could invoke the operator and move the mouse to the left (ratio=0) or the right (ratio=1).
This way they would have linear control of how much is decimated.

This is also how it works for the decimate operator for curve objects, it uses a ratio.

However, I do agree that also being able to use it with a error margin in addition to this would be nice.

If it was a ratio for all selected curves together that could make sense. If it's applied for each individual curve as it is now, I doubt that is what artists are expecting.

It's also not obvious to me that the ratio will be perceived as more linear compared to a good error metric.

The artists (Nacho) scenario only decimated one curve at the time. And it was also on baked key frames.

I don't think we will be able to have a measurement that that will work nicely on multiple curves at once.
Even if we normalize the error margin, the optimal value will not be the same for different curves.
This depends of course what the user want to get out of the decimation.

I don't know if a combined ratio would help in this sense.

For me at least, when I select multiple objects and run an operator on them, I expect the individual result to be the same as if I ran the operator on just one of the objects.

Well, I disagree about what the optimal behavior for multiple curves is here. But since this is a pain to implement over multiple curves I don't think this has to block the patch.

@Sebastian Parborg (zeddb) I'm fine with the functionality, but I think my inline remarks are still valid.

Sebastian Parborg (zeddb) marked 7 inline comments as done.Oct 3 2019, 8:05 PM

Sorry for the long delay. I waited to make any changes in case we decided to make bigger changes first.

Hope my changes is up to snuff.

source/blender/editors/space_graph/graph_edit.c
1350

I hope that the modified wording is clearer. I not we'll try to figure something out.

Sebastian Parborg (zeddb) marked an inline comment as done.
Sybren A. Stüvel (sybren) requested changes to this revision.Oct 15 2019, 1:04 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/animation/keyframes_general.c
345

You may want to add a comment here that explains that you don't want to decimate by using the error, but only by the number of remaining keyframes.

346

The error here is a bit unclear; it could mean "this is the target length of the keyframes in error", e.g. the ones that are removed, or it could be "this is the target length of the keyframes to keep, for whatever function deals with errory things", e.g. the ones to keep.

347

I wouldn't mind just passing BEZT_FLAG_TEMP_TAG directly to the BKE_curve_decimate_bezt_array() function. It's only used in one place anyway. The other variables at least clarify the numbers assigned to them, so they have added value, but passing a flag is IMO clear enough.

347

The above variables could be declared in the if-block they are used in.

source/blender/editors/space_graph/graph_edit.c
1350

LGTM

1358

This line should IMO just be removed, or add a reference to a Txxx/Dxxx and make it possible for others to help out and implement the missing feature.

1366

This still needs a description explaining what 'ratio' means. Right now it's unclear (it doesn't even say it's about the number of keyframes), and it's ambiguous whether this is the percentage to keep or the percentage to remove. Since the default value is 50%, you can't even tell which one it is from the result, as removing 50% is the same as retaining 50%.

1366

Using RNA_def_float_percentage() would make sense here, I think.

This revision now requires changes to proceed.Oct 15 2019, 1:04 PM
Sebastian Parborg (zeddb) marked 8 inline comments as done.

Updated the patched based on the given feedback.

I still feel that ratio is an ambiguous name, in both the C and RNA code. Why not name it keep_ratio or remove_ratio? That way it's immediately clear from the name.

source/blender/editors/animation/keyframes_general.c
347

The old_bezts variable is declared at the top of the function, whereas target_fcurve_verts and num are declared at this spot. This seems a bit strange to me, to have two declaration styles used intermixed in new code.

348

num is unnecessarily short and undescriptive; totvert or even fcu_totvert makes things a lot clearer.

The placement of this line is confusing; the code probably exists at all to make the for-loop below faster, but num is already declared here. Furthermore, the use of fcu->totvert in the if-statement below isn't using num even though it's closer to this line.

Either declare here and avoid fcu->totvert in the if, or just declare it directly above the for-loop before the fcu->totvert = 0 assignment.

355

Magic number; add a comment to explain why 12 is a good value here.

source/blender/editors/space_graph/graph_edit.c
1341

There is no need to add a comment above every line, saying what the line does.

Sebastian Parborg (zeddb) marked 4 inline comments as done.

Updated with the latest feedback

LGTM after those few notes.

source/blender/editors/space_graph/graph_edit.c
1365

As discussed face-to-face: change to 33.3333333333333333333333333333333333333333333333333333333333333333333% (so 1.0f/3.0f).

1368

Probably better to change the label to "Remove"; it's only shown in the redo panel, which already has "Decimate Keyframes" as title.

1369

As discussed face-to-face: ratio → percentage

This revision is now accepted and ready to land.Thu, Nov 21, 11:30 AM
This revision was automatically updated to reflect the committed changes.