Page MenuHome

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.



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

Diff Detail

rB Blender
Build Status
Buildable 1152
Build 1152: arc lint + arc unit

Event Timeline

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.
Kévin Dietrich (kevindietrich) updated this revision to Diff 7102.
  • Use UI_but_funN_set to pass callback data to the but func.
Joshua Leung (aligorith) requested changes to this revision.

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?)


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


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))

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


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.


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

Kévin Dietrich (kevindietrich) updated this revision to Diff 7107.
  • Some changes from review, make sure all buttons have callbacks set.

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);
    DAG_id_tag_update(id, OB_RECALC_DATA);

Alrighty then :)

Kévin Dietrich (kevindietrich) updated this revision to Diff 7108.
  • Use if/else instead of ternary operator for clarity.

This name makes it look like you're referring to a typedef'd function poiinter and not a struct. I'd suggest: FModifierUpdateData or even tFModUpdateData

This revision now requires changes to proceed.Jun 6 2017, 6:19 AM
  • Merge branch 'master' into arcpatch-D2118
  • Rename UpdateModifierCb -> FModifierUpdateData.


Great to see you picking this up again :) Let's try and wrap this thing up!

From the looks of things, there are just a few minor tweaks that will make this a bit easier to understand.


Since the "custom_data" parameter represents the ID block, it would be better to name it "id_arg" to make its purpose clearer


It would be better to name the "arg" parameter "id" or "id_arg", to make it clearer what you're supposed to pass here


It took me a little while to figure out why you were duplicating this here. I guess we can leave this as-is though, as there aren't really any nicer solutions that come to mind :)

  • Merge branch 'master' into arcpatch-D2118
  • Rename parameters to id_arg.