Fix crash happening in Cycles fcurve modifier

Summary:
Crash was happening because of fcurve modifier stack
used modifier's DNA to store temporary data.

Now made it so storage for such a thing is being
allocated locally per object update so multiple objects
which shares the same animation wouldn't run into
threading conflict anymore.

This storage might be a part of EvaluationContext,
but that'd mean passing this context all over in
object_where_is which will clutter API for now without
actual benefit for this.

Optimization notes: storage is only being allocated
if there're Cycles modifier in the stack, so there're
no extra allocations happening in all other cases.

To make code a bit less cluttered with this storage
passing all over the place added extra callbacks to
the FModifier storage which runs evaluation with the
given storage.

Reviewers: brecht, campbellbarton, aligorith

CC: plasmasolutions
Differential Revision: https://developer.blender.org/D147

diff --git a/source/blender/blenkernel/BKE_fcurve.h b/source/blender/blenkernel/BKE_fcurve.h
index c31dd7459111a771da93163492411b03693b6d3e..5ad378bc331ad79402e776f7d698ba78c16aa1a9 100644 (file)
--- a/source/blender/blenkernel/BKE_fcurve.h
+++ b/source/blender/blenkernel/BKE_fcurve.h
@@ -99,6 +99,8 @@ float driver_get_variable_value(struct ChannelDriver *driver, struct DriverVar *
 
 /* ************** F-Curve Modifiers *************** */
 
+typedef struct GHash FModifierStackStorage;
+
 /* F-Curve Modifier Type-Info (fmi):
  *  This struct provides function pointers for runtime, so that functions can be
  *  written more generally (with fewer/no special exceptions for various modifiers).
@@ -134,6 +136,10 @@ typedef struct FModifierTypeInfo {
        float (*evaluate_modifier_time)(struct FCurve *fcu, struct FModifier *fcm, float cvalue, float evaltime);
        /* evaluate the modifier for the given time and 'accumulated' value */
        void (*evaluate_modifier)(struct FCurve *fcu, struct FModifier *fcm, float *cvalue, float evaltime);
+
+       /* Same as above but for modifiers which requires storage */
+       float (*evaluate_modifier_time_storage)(FModifierStackStorage *storage, struct FCurve *fcu, struct FModifier *fcm, float cvalue, float evaltime);
+       void (*evaluate_modifier_storage)(FModifierStackStorage *storage, struct FCurve *fcu, struct FModifier *fcm, float *cvalue, float evaltime);
 } FModifierTypeInfo;
 
 /* Values which describe the behavior of a FModifier Type */
@@ -157,7 +163,10 @@ typedef enum eFMI_Requirement_Flags {
         */
        FMI_REQUIRES_NOTHING            = (1 << 1),
        /* refer to modifier instance */
-       FMI_REQUIRES_RUNTIME_CHECK      = (1 << 2)
+       FMI_REQUIRES_RUNTIME_CHECK      = (1 << 2),
+
+       /* Requires to store data shared between time and valua evaluation */
+       FMI_REQUIRES_STORAGE            = (1 << 3)
 } eFMI_Requirement_Flags;
 
 /* Function Prototypes for FModifierTypeInfo's */
@@ -177,8 +186,10 @@ void set_active_fmodifier(ListBase *modifiers, struct FModifier *fcm);
 
 short list_has_suitable_fmodifier(ListBase *modifiers, int mtype, short acttype);
 
-float evaluate_time_fmodifiers(ListBase *modifiers, struct FCurve *fcu, float cvalue, float evaltime);
-void evaluate_value_fmodifiers(ListBase *modifiers, struct FCurve *fcu, float *cvalue, float evaltime);
+FModifierStackStorage *evaluate_fmodifiers_storage_new(ListBase *modifiers);
+void evaluate_fmodifiers_storage_free(FModifierStackStorage *storage);
+float evaluate_time_fmodifiers(FModifierStackStorage *storage, ListBase *modifiers, struct FCurve *fcu, float cvalue, float evaltime);
+void evaluate_value_fmodifiers(FModifierStackStorage *storage, ListBase *modifiers, struct FCurve *fcu, float *cvalue, float evaltime);
 
 void fcurve_bake_modifiers(struct FCurve *fcu, int start, int end);
 

diff --git a/source/blender/blenkernel/intern/anim_sys.c b/source/blender/blenkernel/intern/anim_sys.c
index cb628db8cd2b5933de12951b56fc554eb99e4b8c..609932ae5e213b8492a1a2a1345b19288c0ffda1 100644 (file)
--- a/source/blender/blenkernel/intern/anim_sys.c
+++ b/source/blender/blenkernel/intern/anim_sys.c
@@ -1981,6 +1981,7 @@ static void nlaeval_fmodifiers_split_stacks(ListBase *list1, ListBase *list2)
 /* evaluate action-clip strip */
 static void nlastrip_evaluate_actionclip(PointerRNA *ptr, ListBase *channels, ListBase *modifiers, NlaEvalStrip *nes)
 {
+       FModifierStackStorage *storage;
        ListBase tmp_modifiers = {NULL, NULL};
        NlaStrip *strip = nes->strip;
        FCurve *fcu;
@@ -2001,7 +2002,8 @@ static void nlastrip_evaluate_actionclip(PointerRNA *ptr, ListBase *channels, Li
        nlaeval_fmodifiers_join_stacks(&tmp_modifiers, &strip->modifiers, modifiers);
        
        /* evaluate strip's modifiers which modify time to evaluate the base curves at */
-       evaltime = evaluate_time_fmodifiers(&tmp_modifiers, NULL, 0.0f, strip->strip_time);
+       storage = evaluate_fmodifiers_storage_new(&tmp_modifiers);
+       evaltime = evaluate_time_fmodifiers(storage, &tmp_modifiers, NULL, 0.0f, strip->strip_time);
        
        /* evaluate all the F-Curves in the action, saving the relevant pointers to data that will need to be used */
        for (fcu = strip->act->curves.first; fcu; fcu = fcu->next) {
@@ -2022,7 +2024,7 @@ static void nlastrip_evaluate_actionclip(PointerRNA *ptr, ListBase *channels, Li
                /* apply strip's F-Curve Modifiers on this value 
                 * NOTE: we apply the strip's original evaluation time not the modified one (as per standard F-Curve eval)
                 */
-               evaluate_value_fmodifiers(&tmp_modifiers, fcu, &value, strip->strip_time);
+               evaluate_value_fmodifiers(storage, &tmp_modifiers, fcu, &value, strip->strip_time);
                
                
                /* get an NLA evaluation channel to work with, and accumulate the evaluated value with the value(s)
@@ -2032,7 +2034,10 @@ static void nlastrip_evaluate_actionclip(PointerRNA *ptr, ListBase *channels, Li
                if (nec)
                        nlaevalchan_accumulate(nec, nes, value);
        }
-       
+
+       /* free temporary storage */
+       evaluate_fmodifiers_storage_free(storage);
+
        /* unlink this strip's modifiers from the parent's modifiers again */
        nlaeval_fmodifiers_split_stacks(&strip->modifiers, modifiers);
 }

diff --git a/source/blender/blenkernel/intern/fcurve.c b/source/blender/blenkernel/intern/fcurve.c
index 32098c67ca7fc5fe8900b01c148dea3283d66696..04765a74b77f2ec9728c9e51dd74380e57d89d91 100644 (file)
--- a/source/blender/blenkernel/intern/fcurve.c
+++ b/source/blender/blenkernel/intern/fcurve.c
@@ -2138,6 +2138,7 @@ static float fcurve_eval_samples(FCurve *fcu, FPoint *fpts, float evaltime)
  */
 float evaluate_fcurve(FCurve *fcu, float evaltime)
 {
+       FModifierStackStorage *storage;
        float cvalue = 0.0f;
        float devaltime;
        
@@ -2177,9 +2178,10 @@ float evaluate_fcurve(FCurve *fcu, float evaltime)
                        }
                }
        }
-       
+
        /* evaluate modifiers which modify time to evaluate the base curve at */
-       devaltime = evaluate_time_fmodifiers(&fcu->modifiers, fcu, cvalue, evaltime);
+       storage = evaluate_fmodifiers_storage_new(&fcu->modifiers);
+       devaltime = evaluate_time_fmodifiers(storage, &fcu->modifiers, fcu, cvalue, evaltime);
        
        /* evaluate curve-data 
         *      - 'devaltime' instead of 'evaltime', as this is the time that the last time-modifying 
@@ -2191,8 +2193,10 @@ float evaluate_fcurve(FCurve *fcu, float evaltime)
                cvalue = fcurve_eval_samples(fcu, fcu->fpt, devaltime);
        
        /* evaluate modifiers */
-       evaluate_value_fmodifiers(&fcu->modifiers, fcu, &cvalue, evaltime);
-       
+       evaluate_value_fmodifiers(storage, &fcu->modifiers, fcu, &cvalue, evaltime);
+
+       evaluate_fmodifiers_storage_free(storage);
+
        /* if curve can only have integral values, perform truncation (i.e. drop the decimal part)
         * here so that the curve can be sampled correctly
         */

diff --git a/source/blender/blenkernel/intern/fmodifier.c b/source/blender/blenkernel/intern/fmodifier.c
index f24edfea136974e627854a7d76a00b07239c996b..3436728254ba32c06258b916837285e119bd760d 100644 (file)
--- a/source/blender/blenkernel/intern/fmodifier.c
+++ b/source/blender/blenkernel/intern/fmodifier.c
@@ -42,6 +42,7 @@
 #include "BLF_translation.h"
 
 #include "BLI_blenlib.h"
+#include "BLI_ghash.h"
 #include "BLI_noise.h"
 #include "BLI_math.h" /* windows needs for M_PI */
 #include "BLI_utildefines.h"
@@ -51,6 +52,11 @@
 
 /* ******************************** F-Modifiers ********************************* */
 
+/* Forward declarations. */
+void fmodifiers_storage_put(FModifierStackStorage *storage, FModifier *fcm, void *data);
+void fmodifiers_storage_remove(FModifierStackStorage *storage, FModifier *fcm);
+void *fmodifiers_storage_get(FModifierStackStorage *storage, FModifier *fcm);
+
 /* Info ------------------------------- */
 
 /* F-Modifiers are modifiers which operate on F-Curves. However, they can also be defined
@@ -89,7 +95,9 @@ static FModifierTypeInfo FMI_MODNAME = {
        fcm_modname_new_data, /* new data */
        fcm_modname_verify, /* verify */
        fcm_modname_time, /* evaluate time */
-       fcm_modname_evaluate /* evaluate */
+       fcm_modname_evaluate, /* evaluate */
+       fcm_modname_time_storage, /* evaluate time with storage */
+       fcm_modname_evaluate_storage /* evaluate with storage */
 };
 #endif
 
@@ -240,7 +248,9 @@ static FModifierTypeInfo FMI_GENERATOR = {
        fcm_generator_new_data, /* new data */
        fcm_generator_verify, /* verify */
        NULL, /* evaluate time */
-       fcm_generator_evaluate /* evaluate */
+       fcm_generator_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* Built-In Function Generator F-Curve Modifier --------------------------- */
@@ -362,7 +372,9 @@ static FModifierTypeInfo FMI_FN_GENERATOR = {
        fcm_fn_generator_new_data, /* new data */
        NULL, /* verify */
        NULL, /* evaluate time */
-       fcm_fn_generator_evaluate /* evaluate */
+       fcm_fn_generator_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* Envelope F-Curve Modifier --------------------------- */
@@ -469,7 +481,9 @@ static FModifierTypeInfo FMI_ENVELOPE = {
        fcm_envelope_new_data, /* new data */
        fcm_envelope_verify, /* verify */
        NULL, /* evaluate time */
-       fcm_envelope_evaluate /* evaluate */
+       fcm_envelope_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* exported function for finding points */
@@ -585,7 +599,8 @@ static void fcm_cycles_new_data(void *mdata)
        data->before_mode = data->after_mode = FCM_EXTRAPOLATE_CYCLIC;
 }
 
-static float fcm_cycles_time(FCurve *fcu, FModifier *fcm, float UNUSED(cvalue), float evaltime)
+static float fcm_cycles_time(FModifierStackStorage *storage, FCurve *fcu, FModifier *fcm,
+                             float UNUSED(cvalue), float evaltime)
 {
        FMod_Cycles *data = (FMod_Cycles *)fcm->data;
        float prevkey[2], lastkey[2], cycyofs = 0.0f;
@@ -717,18 +732,21 @@ static float fcm_cycles_time(FCurve *fcu, FModifier *fcm, float UNUSED(cvalue),
                tFCMED_Cycles *edata;
                
                /* for now, this is just a float, but we could get more stuff... */
-               fcm->edata = edata = MEM_callocN(sizeof(tFCMED_Cycles), "tFCMED_Cycles");
+               edata = MEM_callocN(sizeof(tFCMED_Cycles), "tFCMED_Cycles");
                edata->cycyofs = cycyofs;
+
+               fmodifiers_storage_put(storage, fcm, edata);
        }
        
        /* return the new frame to evaluate */
        return evaltime;
 }
  
-static void fcm_cycles_evaluate(FCurve *UNUSED(fcu), FModifier *fcm, float *cvalue, float UNUSED(evaltime))
+static void fcm_cycles_evaluate(FModifierStackStorage *storage, FCurve *UNUSED(fcu),
+                                FModifier *fcm, float *cvalue, float UNUSED(evaltime))
 {
-       tFCMED_Cycles *edata = (tFCMED_Cycles *)fcm->edata;
-       
+       tFCMED_Cycles *edata = fmodifiers_storage_get(storage, fcm);
+
        /* use temp data */
        if (edata) {
                /* add cyclic offset - no need to check for now, otherwise the data wouldn't exist! */
@@ -736,7 +754,7 @@ static void fcm_cycles_evaluate(FCurve *UNUSED(fcu), FModifier *fcm, float *cval
                
                /* free temp data */
                MEM_freeN(edata);
-               fcm->edata = NULL;
+               fmodifiers_storage_remove(storage, fcm);
        }
 }
 
@@ -744,15 +762,17 @@ static FModifierTypeInfo FMI_CYCLES = {
        FMODIFIER_TYPE_CYCLES, /* type */
        sizeof(FMod_Cycles), /* size */
        FMI_TYPE_EXTRAPOLATION, /* action type */
-       FMI_REQUIRES_ORIGINAL_DATA, /* requirements */
+       FMI_REQUIRES_ORIGINAL_DATA | FMI_REQUIRES_STORAGE, /* requirements */
        N_("Cycles"), /* name */
        "FMod_Cycles", /* struct name */
        NULL, /* free data */
        NULL, /* copy data */
        fcm_cycles_new_data, /* new data */
        NULL /*fcm_cycles_verify*/, /* verify */
-       fcm_cycles_time, /* evaluate time */
-       fcm_cycles_evaluate /* evaluate */
+       NULL, /* evaluate time */
+       NULL, /* evaluate */
+       fcm_cycles_time, /* evaluate time with storage */
+       fcm_cycles_evaluate /* evaluate with storage */
 };
 
 /* Noise F-Curve Modifier  --------------------------- */
@@ -810,7 +830,9 @@ static FModifierTypeInfo FMI_NOISE = {
        fcm_noise_new_data, /* new data */
        NULL /*fcm_noise_verify*/, /* verify */
        NULL, /* evaluate time */
-       fcm_noise_evaluate /* evaluate */
+       fcm_noise_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* Filter F-Curve Modifier --------------------------- */
@@ -828,7 +850,9 @@ static FModifierTypeInfo FMI_FILTER = {
        NULL, /* new data */
        NULL /*fcm_filter_verify*/, /* verify */
        NULL, /* evlauate time */
-       fcm_filter_evaluate /* evaluate */
+       fcm_filter_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 #endif // XXX not yet implemented
 
@@ -884,7 +908,9 @@ static FModifierTypeInfo FMI_PYTHON = {
        fcm_python_new_data, /* new data */
        NULL /*fcm_python_verify*/, /* verify */
        NULL /*fcm_python_time*/, /* evaluate time */
-       fcm_python_evaluate /* evaluate */
+       fcm_python_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 
@@ -927,7 +953,9 @@ static FModifierTypeInfo FMI_LIMITS = {
        NULL, /* new data */
        NULL, /* verify */
        fcm_limits_time, /* evaluate time */
-       fcm_limits_evaluate /* evaluate */
+       fcm_limits_evaluate, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* Stepped F-Curve Modifier --------------------------- */
@@ -980,7 +1008,9 @@ static FModifierTypeInfo FMI_STEPPED = {
        fcm_stepped_new_data, /* new data */
        NULL, /* verify */
        fcm_stepped_time, /* evaluate time */
-       NULL /* evaluate */
+       NULL, /* evaluate */
+       NULL, /* evaluate time with storage */
+       NULL /* evaluate with storage */
 };
 
 /* F-Curve Modifier API --------------------------- */
@@ -1256,6 +1286,60 @@ short list_has_suitable_fmodifier(ListBase *modifiers, int mtype, short acttype)
 
 /* Evaluation API --------------------------- */
 
+FModifierStackStorage *evaluate_fmodifiers_storage_new(ListBase *modifiers)
+{
+       FModifier *fcm;
+
+       /* Sanity checks. */
+       if (ELEM(NULL, modifiers, modifiers->last)) {
+               return NULL;
+       }
+
+       for (fcm = modifiers->last; fcm; fcm = fcm->prev) {
+               FModifierTypeInfo *fmi = fmodifier_get_typeinfo(fcm);
+
+               if (fmi == NULL) {
+                       continue;
+               }
+
+               if (fmi->requires & FMI_REQUIRES_STORAGE) {
+                       return (FModifierStackStorage *) BLI_ghash_new(BLI_ghashutil_ptrhash,
+                                                                      BLI_ghashutil_ptrcmp,
+                                                                      "fmodifier stack temp storage");
+               }
+       }
+
+       return NULL;
+}
+
+void evaluate_fmodifiers_storage_free(FModifierStackStorage *storage)
+{
+       if (storage != NULL) {
+               BLI_ghash_free((GHash *) storage, NULL, NULL);
+       }
+}
+
+void fmodifiers_storage_put(FModifierStackStorage *storage, FModifier *fcm, void *data)
+{
+       BLI_assert(storage != NULL);
+
+       BLI_ghash_insert((GHash *) storage, fcm, data);
+}
+
+void fmodifiers_storage_remove(FModifierStackStorage *storage, FModifier *fcm)
+{
+       BLI_assert(storage != NULL);
+
+       BLI_ghash_remove((GHash *) storage, fcm, NULL, NULL);
+}
+
+void *fmodifiers_storage_get(FModifierStackStorage *storage, FModifier *fcm)
+{
+       BLI_assert(storage != NULL);
+
+       return BLI_ghash_lookup((GHash *) storage, fcm);
+}
+
 /* helper function - calculate influence of FModifier */
 static float eval_fmodifier_influence(FModifier *fcm, float evaltime)
 {
@@ -1306,7 +1390,8 @@ static float eval_fmodifier_influence(FModifier *fcm, float evaltime)
  *       so nevaltime gets set to whatever the last time-modifying modifier likes...
  *     - we start from the end of the stack, as only the last one matters for now
  */
-float evaluate_time_fmodifiers(ListBase *modifiers, FCurve *fcu, float cvalue, float evaltime)
+float evaluate_time_fmodifiers(FModifierStackStorage *storage, ListBase *modifiers,
+                               FCurve *fcu, float cvalue, float evaltime)
 {
        FModifier *fcm;
        
@@ -1337,10 +1422,17 @@ float evaluate_time_fmodifiers(ListBase *modifiers, FCurve *fcu, float cvalue, f
                    ((fcm->sfra <= evaltime) && (fcm->efra >= evaltime)) )
                {
                        /* only evaluate if there's a callback for this */
-                       if (fmi->evaluate_modifier_time) {
+                       if (fmi->evaluate_modifier_time || fmi->evaluate_modifier_time_storage) {
                                if ((fcm->flag & (FMODIFIER_FLAG_DISABLED | FMODIFIER_FLAG_MUTED)) == 0) {
                                        float influence = eval_fmodifier_influence(fcm, evaltime);
-                                       float nval = fmi->evaluate_modifier_time(fcu, fcm, cvalue, evaltime);
+                                       float nval;
+
+                                       if ((fmi->requires & FMI_REQUIRES_STORAGE) == 0) {
+                                               nval = fmi->evaluate_modifier_time(fcu, fcm, cvalue, evaltime);
+                                       }
+                                       else {
+                                               nval = fmi->evaluate_modifier_time_storage(storage, fcu, fcm, cvalue, evaltime);
+                                       }
                                        
                                        evaltime = interpf(nval, evaltime, influence);
                                }
@@ -1355,7 +1447,8 @@ float evaluate_time_fmodifiers(ListBase *modifiers, FCurve *fcu, float cvalue, f
 /* Evaluates the given set of F-Curve Modifiers using the given data
  * Should only be called after evaluate_time_fmodifiers() has been called...
  */
-void evaluate_value_fmodifiers(ListBase *modifiers, FCurve *fcu, float *cvalue, float evaltime)
+void evaluate_value_fmodifiers(FModifierStackStorage *storage, ListBase *modifiers,
+                               FCurve *fcu, float *cvalue, float evaltime)
 {
        FModifier *fcm;
        
@@ -1374,12 +1467,18 @@ void evaluate_value_fmodifiers(ListBase *modifiers, FCurve *fcu, float *cvalue,
                if ((fcm->flag & FMODIFIER_FLAG_RANGERESTRICT) == 0 ||
                    ((fcm->sfra <= evaltime) && (fcm->efra >= evaltime)) )
                {
-                       if (fmi->evaluate_modifier) {
+                       if (fmi->evaluate_modifier || fmi->evaluate_modifier_storage) {
                                if ((fcm->flag & (FMODIFIER_FLAG_DISABLED | FMODIFIER_FLAG_MUTED)) == 0) {
                                        float influence = eval_fmodifier_influence(fcm, evaltime);
                                        float nval = *cvalue;
-                                       
-                                       fmi->evaluate_modifier(fcu, fcm, &nval, evaltime);
+
+                                       if ((fmi->requires & FMI_REQUIRES_STORAGE) == 0) {
+                                               fmi->evaluate_modifier(fcu, fcm, &nval, evaltime);
+                                       }
+                                       else {
+                                               fmi->evaluate_modifier_storage(storage, fcu, fcm, &nval, evaltime);
+                                       }
+
                                        *cvalue = interpf(nval, *cvalue, influence);
                                }
                        }

diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index ed501d50b076ce2e8be5d99c9c31ed760e0fec71..81018df7a8884498b8c8866875c6d46fff2efec3 100644 (file)
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -2028,7 +2028,6 @@ static void direct_link_fmodifiers(FileData *fd, ListBase *list)
        for (fcm = list->first; fcm; fcm = fcm->next) {
                /* relink general data */
                fcm->data  = newdataadr(fd, fcm->data);
-               fcm->edata = NULL;
                
                /* do relinking of data for specific types */
                switch (fcm->type) {

diff --git a/source/blender/makesdna/DNA_anim_types.h b/source/blender/makesdna/DNA_anim_types.h
index 7a7e08138b047fc03d5906a5d7f9cb41b4a3e98a..6f5c9254a3881476ae4f137984bc99799b85c57a 100644 (file)
--- a/source/blender/makesdna/DNA_anim_types.h
+++ b/source/blender/makesdna/DNA_anim_types.h
@@ -53,7 +53,6 @@ typedef struct FModifier {
        struct FModifier *next, *prev;
        
        void *data;                     /* pointer to modifier data */
-       void *edata;            /* pointer to temporary data used during evaluation */
        
        char name[64];          /* user-defined description for the modifier */
        short type;                     /* type of f-curve modifier */