Page MenuHome

Make curve decimation only take into account the selected curve points
ClosedPublic

Authored by Sebastian Parborg (zeddb) on Thu, Nov 21, 4:21 PM.

Details

Summary

Previously the decimation would take the whole curve into account when decimating and not just the selected part.

If this new method is OK I will also update BKE_curve_decimate_nurb (Bezier/Nurbs object decimate) to match this behavior.

Diff Detail

Repository
rB Blender

Event Timeline

Updated the patch so the redo panel pops up after each operation.

We will need to double check that any graph editor operation doesn't have any properties that should be hidden.

The decimate operator now also works as a modal operator.

Previously the decimation would take the whole curve into account when decimating and not just the selected part.

Probably a good idea to refer to D4841 in the description.

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

Add a comment that says what this does.

331

Naming is unclear. I'm guessing 'seg' is short for 'segment' (just write 'segment' in that case), but then it's still unclear what's the difference between a chain and a segment.

338

actual is a bad name, as it doesn't actually (ha! punny) has any meaning here. selected_len would be a better name.

340–341

What is the difference between 'curve start/end point' and 'actual curve start/end point'?

342–348

The term bezt_chain seems to change semantics here. Earlier it was the 'chain of selected keys', and now it seems to mean 'the keys that can be deleted + surrounding keys'. Or maybe not. It's unclear, in any case.

356

Shouldn't this be #DEFINEd or const short'ed somewhere?

source/blender/editors/interface/view2d.c
1939

Add a comment explaining what the function does.

source/blender/editors/space_graph/graph_edit.c
1339–1344

These comment don't add any information.

1346

Sentence.

1349

Sentence.

1352

Comment doesn't add any information.

1361

Add a comment explaining what the function does.

1375

Make a sentence, start with capital and end with period.

I'll stop here with the comments about this; apply it to all comments where appliccable.

1375

The function name says 'reset', this comment says 'restore', pick one and make it consistent.

1399

You can do if (dgo == NULL) return; here, and unindent the rest of the function.

1428

No need to create a new variable scene just to avoid dgo-> in this line.

1461

Why would you allocate, do this check, and immediately deallocate? Why not just do the check before the allocation?

1471

What is 'pop' in this case?

1474

Comment doesn't add anything.

1477

Comment doesn't add anything.

1480

Why?

1493

I only see a lot of allocation & copying. Where is the cleaning done? Also what is cleaning anyway? It thought this was a function to decimate curves?

1517

Comment doesn't add anything.

1535

Comment doesn't add anything.

1536–1537

It would be better to have this in decimate_exit() instead, as it's on-exit behaviour.

1541

yes well, that comment doesn't add anything either.

1554

Comment doesn't add anything.

1561

Comment doesn't add anything.

1564

Comment doesn't add anything.

1573

Comment doesn't add anything.

1585–1595

What is the purpose of this? I'm not familiar with the xxxNumInput() functions, so that could be it, but it seems a bit like taking a step forward and then a step back again.

1589

Then why is percentage a float?

1609–1622

I would put this in a function, and call that function instead of setting do_pose_update=true.

1612

Comment doesn't add anything. Especially when you would rename the function to decimate_draw_status_header.

source/blender/makesdna/DNA_screen_types.h
386

Don't describe where it is used, but describe what it means.

Sybren A. Stüvel (sybren) requested changes to this revision.Wed, Nov 27, 4:36 PM
This revision now requires changes to proceed.Wed, Nov 27, 4:36 PM
Sebastian Parborg (zeddb) marked 35 inline comments as done.

Updated with the latest feedback.

Updated with so you can now choose from two types of decimation modes.

I tried it out, but it doesn't seem to work for me.

Decimate (Ratio) seems to always select the left-most keyframes:

I imported a BVH, selected some keyframes in a single range, and chose Decimate (Ratio) from the menu. Here is the Blend file:

Decimate (Allowed Change) doesn't seem to do anything. It looks like it doesn't have the wave-in-thin-air-to-change-the-parameter behaviour, and an error of 500 doesn't do anything either.

Fixed the issues that we found.

Ok, the fix works, but it's still confusing. The patch description doesn't mention anything about the interpolation type changing, and neither do the tooltips. Would it be better if linearly interpolated keyframes are changed back to linear after the decimation is done?

release/scripts/startup/bl_ui/space_graph.py
294–301

Don't name things context if they are not a Context. Just name it operator_context.

297

why?

source/blender/editors/animation/keyframes_general.c
340–341

Just write 'Only remove the start/end point of the segment if they are not the start/end point of the curve'. That removes the use of the somewhat vague term 'actual'.

356

Still unclear why 12 is such a good middle ground.

434

Shouldn't this be "the current keyframe's interpolation type"? Or are there other aspects of the keyframe that are relevant as well?

436

Well yes, that's obvious from the fact that nothing is happening. Always include the why in comments.

439

This describes what it's doing, but not why. As this is something that's not trivial to understand, the motivation needs to be included in the source code. It should also be obvious to the user that this is what will happen.

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

Missing tooltip.

1751–1759

Is a soft-max of FLT_MAX really useful here? It makes dragging the slider hard as the values will most likely be too big.

Sybren A. Stüvel (sybren) requested changes to this revision.Tue, Dec 3, 1:51 PM
This revision now requires changes to proceed.Tue, Dec 3, 1:51 PM
release/scripts/startup/bl_ui/space_graph.py
297

Also: period.

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

Make a switch with two case statements.

Sebastian Parborg (zeddb) updated this revision to Diff 20046.EditedTue, Dec 3, 4:44 PM
Sebastian Parborg (zeddb) marked 11 inline comments as done.

Updated based on feedback

Ok, the fix works, but it's still confusing. The patch description doesn't mention anything about the interpolation type changing, and neither do the tooltips. Would it be better if linearly interpolated keyframes are changed back to linear after the decimation is done?

I discussed this with @Sebastian Parborg (zeddb) and given how the decimate algorithm works (fitting Bézier curves) this wouldn't produce good results.

@Sebastian Parborg (zeddb) there still is the issue of telling users that this change from linear to Bézier will happen. I still don't feel confident that this is what can naturally be expected from the words 'Decimate' and 'remove keyframes', so IMO it should be mentioned in a tooltip (AFAICS it's pretty much the only space available for this if we don't want to cram full sentences in the menu item label).

release/scripts/startup/bl_ui/space_graph.py
297

as we do not have a modal mode for it

Now that there is a reasonable soft limit, shouldn't both modes just have the same behaviour?

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

Now that I think of it (sorry for bringing this up this late), wouldn't a switch make more sense here? Then it's immediately clear that it's a selection on fcu->bezt[i].ipo only.

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

We discussed (face-to-face) squaring the error here in order to use the more common RMS metric, as this would also give a more linear 'feel' to the user when experimenting with different values.

As the code is now, remove_error_margin (so not squared) is assigned to error_sq_max (the name implying 'squared'), without squaring.

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

Updated with the latest comments.

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

Rename to prepare_for_decimate_keyframes (or just prepare_for_decimate or something along those lines), as it does more than just checking. Also update the comment for this.

338

Probably fine to just return true; here.

360

ditto

source/blender/editors/space_graph/graph_edit.c
1635–1658

Give the (currenty anonymous) enum a name, and use that here.

1636

Pass initial values to avoid 'may be used uninitialized in this function' warnings for those with GCC < 9 ;-)

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

Comment doesn't add much. Better to document why the error is squared.

LGTM after the comment change.

This revision is now accepted and ready to land.Wed, Dec 4, 3:49 PM