Fix T46553: FCurve Modifier parameter is changed only with time change
Needs ReviewPublic

Authored by Kévin Dietrich (kevindietrich) on Jul 22 2016, 12:33 AM.

Details

Summary

UI buttons were missing some update callback, which is copied from the RNA
update callback.

Diff Detail

Repository
rB Blender
Branch
fix_t46553
Build Status
Buildable 38
Build 38: arc lint + arc unit
Kévin Dietrich (kevindietrich) retitled this revision from to Fix T46553: FCurve Modifier parameter is changed only with time change.Jul 22 2016, 12:33 AM
Kévin Dietrich (kevindietrich) updated this object.
  • Use UI_but_funN_set to pass callback data to the but func.
Joshua Leung (aligorith) requested changes to this revision.Jul 23 2016, 4:03 PM

Currently you've got this set for some of the buttons but not others. Why is it that only those ones get set? Are there any others which may benefit? (e.g. do the other RNA update callbacks already do something similar, in cases where this sort of thing may be useful?)

source/blender/editors/animation/fmodifier_ui.c
117

It would be better to have the check outside the function call here

125

Standard convention is to mark the arg in the function signature instead of doing it here. This way is a lot harder to follow.

So for example:

static void update_fmodifier_cb(bContext *C, void *custom_data, void *UNUSED(args2))
128

It'd be great to have just a small (1-liner) comment noting where this data may be useful or what it is for.

655

Although not strictly necessary anymore, I'd still prefer that as much as possible (*) we try to keep defines at the start of blocks instead of scattered all throughout the code.

(*) Of course, that is assuming that keeping them at the start of the block(s) won't cause us to have to jump through hoops of fire to make that happen.

But in this case, it's forseeable that some of the other buttons may at some point need something similar

This revision now requires changes to proceed.Jul 23 2016, 4:03 PM
Kévin Dietrich (kevindietrich) marked 3 inline comments as done.Jul 24 2016, 2:32 AM

I just made a thorough check of all the buttons in the various modifiers and made sure that have their update callbacks set, using the RNA ones as reference. Most buttons have their callbacks set in the RNA already, the ones missing are the points from the Envelope modifier and the polynomial coefficients of the Generator modifier, since those buttons are dynamically generated. Also the delete modifier button was missing the update.

Will update the patch shortly.

source/blender/editors/animation/fmodifier_ui.c
117

How do you see that happening? Use separate callbacks, store the recalc flag in the UpdateModifierCb struct added below?

  • Some changes from review, make sure all buttons have callbacks set.
source/blender/editors/animation/fmodifier_ui.c
117

I meant, instead of doing:

DAG_id_tag_update(id, (GS(id->name) == ID_OB) ? OB_RECALC_OB : OB_RECALC_DATA);

do this instead:

if (GS(id->name) == ID_OB)
    DAG_id_tag_update(id, OB_RECALC_OB);
else
    DAG_id_tag_update(id, OB_RECALC_DATA);
source/blender/editors/animation/fmodifier_ui.c
117

Alrighty then :)

  • Use if/else instead of ternary operator for clarity.