Forcefield F-curve modifier changes don't reset frames in memory #80121

New Issue

Luc Delgrange · 2020-08-26T03:11:46+02:00

Luc Delgrange commented

2020-08-26 03:11:46 +02:00

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: Radeon (TM) RX 470 Graphics ATI Technologies Inc. 4.5.14736 Core Profile Context 20.8.1 27.20.2001.13001

Blender Version
Broken: version: 2.91.0 Alpha, branch: master, commit date: 2020-08-25 19:28, hash: 396d39c6b9
Worked: (newest version of Blender that worked as expected)

Short description of error
When doing an simulation (tested it on cloth and rigid body sim), adjusting a F-curve modifier of a forcefield doesn't reset the frame in memory.

Exact steps for others to reproduce the error
Set up a basic simulation with and object and a forcefield. Add a F-curve modifier to the intensity of the forcefield. Change a setting in the modifier: the frames in memory don't reset.frames_in_memory.blend

**System Information** Operating system: Windows-10-10.0.18362-SP0 64 Bits Graphics card: Radeon (TM) RX 470 Graphics ATI Technologies Inc. 4.5.14736 Core Profile Context 20.8.1 27.20.2001.13001 **Blender Version** Broken: version: 2.91.0 Alpha, branch: master, commit date: 2020-08-25 19:28, hash: `396d39c6b9` Worked: (newest version of Blender that worked as expected) **Short description of error** When doing an simulation (tested it on cloth and rigid body sim), adjusting a F-curve modifier of a forcefield doesn't reset the frame in memory. **Exact steps for others to reproduce the error** Set up a basic simulation with and object and a forcefield. Add a F-curve modifier to the intensity of the forcefield. Change a setting in the modifier: the frames in memory don't reset.[frames_in_memory.blend](https://archive.blender.org/developer/F8811141/frames_in_memory.blend)

Luc Delgrange commented

2020-08-26 03:11:46 +02:00

Added subscriber: @ldelgrange

Philipp Oeser commented

2020-08-26 10:34:25 +02:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2020-08-26 10:34:25 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Philipp Oeser commented

2020-08-26 10:34:25 +02:00

Can confirm, checking...

Philipp Oeser self-assigned this 2020-09-01 14:48:02 +02:00

Sybren A. Stüvel commented

2020-09-24 12:35:38 +02:00

Added subscriber: @dr.sybren

Sybren A. Stüvel commented

2020-09-24 12:35:38 +02:00

@lichtwerk are you still working on this?

Philipp Oeser commented

2020-09-24 12:59:19 +02:00

In #80121#1022320, @dr.sybren wrote:
@lichtwerk are you still working on this?

Yes

> In #80121#1022320, @dr.sybren wrote: > @lichtwerk are you still working on this? Yes

Philipp Oeser commented

2020-09-25 15:44:25 +02:00

So have been looking at things - and things seem to be of a bit more general nature.

The animation system does not call RNA update functions IIRC, usually the UI does though.
So if you would tweak something associated with the FCurve the FCurve Modifier is on directly in the UI, its RNA update function gets called.
In the case of this report we are talking about a Forcefield Strength (so we need rna_FieldSettings_update to be called -- this takes care of doing the specific updates needed ID_RECALC_TRANSFORM / ID_RECALC_PSYS_RESET)

tweaking this on its field in the Properties Editor would call it directly
tweaking keyframes in an FCurve would call it (via ANIM_animdata_update --> ANIM_list_elem_update --> RNA_property_update_main (on fcu->rna_path)
adding or removing FCurve Modifiers would do it via ANIM_animdata_update as well
BUT: the UI for the FCurve Modifiers has its own RNA, the properties associated with the FCurve will not get their RNA update functions called (this is what we need to change)

What updates happen with the UI for the FCurve Modifiers?

the corresponding RNA update function for the FCurve Modifier is called (mostly just rna_FModifier_update), again: not the corresponding property the FCurve is coupled with
rna_FModifier_update only does very general update tagging ( rna_tag_animation_update -- which now tags the Action ID_RECALC_ANIMATION, which also sounds pretty useless? )
- note: in rna_FModifier_update we have access to the Action (not the owning ID!), we could get the FCurves's rna_path, but for proper RNA updates, we would need the owning object ID instead?
depending on the Type of the FCurve Modifier, this will also jump through hoops and call deg_update [which sends NC_ANIMATION | ND_KEYFRAME | NA_EDITED notifiers, as well as tagging the Action ID_RECALC_ANIMATION -- but these are also not enough here]
- here again, the Action is tagged ID_RECALC_ANIMATION, which also sounds pretty useless? same as for the rna_FModifier_update...

So, I think what we should be after when tweaking the FModifiers UI is:

call proper RNA updates on the underlying properties coupled with that FCurve, (I think we would need the owning object, not the Action, and since an Action can be on multiple objects / IDs, this could get hairy)
clean up the whole FModifier UI and get rid of this deg_update workaround (already have parts of this done locally here)

Stay tuned...

So have been looking at things - and things seem to be of a bit more general nature. The animation system does not call RNA update functions IIRC, usually the UI does though. So if you would tweak something associated with the FCurve the FCurve Modifier is on **directly** in the UI, its RNA update function gets called. In the case of this report we are talking about a Forcefield Strength (so we need `rna_FieldSettings_update` to be called -- this takes care of doing the specific updates needed `ID_RECALC_TRANSFORM` / `ID_RECALC_PSYS_RESET`) - tweaking this on its field in the Properties Editor would call it directly - tweaking keyframes in an FCurve would call it (via `ANIM_animdata_update` --> `ANIM_list_elem_update` --> `RNA_property_update_main` (on fcu->rna_path) - adding or removing FCurve Modifiers would do it via `ANIM_animdata_update` as well - **BUT**: the UI for the FCurve Modifiers has its own RNA, the properties associated with the FCurve will **not** get their RNA update functions called (this is what we need to change) What updates happen with the UI for the FCurve Modifiers? - the corresponding RNA update function **for the FCurve Modifier** is called (mostly just `rna_FModifier_update`), again: not the corresponding property the FCurve is coupled with - `rna_FModifier_update` only does very general update tagging ( `rna_tag_animation_update` -- which now tags the **Action** ID_RECALC_ANIMATION, which also sounds pretty useless? ) - note: in `rna_FModifier_update` we have access to the **Action** (not the owning ID!), we could get the FCurves's rna_path, but for proper RNA updates, we would need the owning object ID instead? - depending on the Type of the FCurve Modifier, this will also jump through hoops and call `deg_update` [which sends NC_ANIMATION | ND_KEYFRAME | NA_EDITED notifiers, as well as tagging the **Action** ID_RECALC_ANIMATION -- but these are also not enough here] - here again, the **Action** is tagged ID_RECALC_ANIMATION, which also sounds pretty useless? same as for the `rna_FModifier_update`... So, I *think* what we should be after when tweaking the FModifiers UI is: - call proper RNA updates on the underlying properties coupled with that FCurve, (I *think* we would need the owning object, not the Action, and since an Action can be on multiple objects / IDs, this could get hairy) - clean up the whole FModifier UI and get rid of this `deg_update` workaround (already have parts of this done locally here) Stay tuned...

Philipp Oeser commented

2020-09-25 17:12:43 +02:00

This seems to work nicely (and should enable us to get rid of lots of custom update code in FModifiers UI code)
P1658: #80121 WIP



diff --git a/source/blender/makesrna/intern/rna_fcurve.c b/source/blender/makesrna/intern/rna_fcurve.c
index ac4395f08c9..362765ecf85 100644
--- a/source/blender/makesrna/intern/rna_fcurve.c
+++ b/source/blender/makesrna/intern/rna_fcurve.c
@@ -742,7 +742,7 @@ static void rna_FModifier_blending_range(
   *max = fcm->efra - fcm->sfra;
 }
 
-static void rna_FModifier_update(Main *bmain, Scene *UNUSED(scene), PointerRNA *ptr)
+static void rna_FModifier_update(Main *bmain, Scene *scene, PointerRNA *ptr)
 {
   ID *id = ptr->owner_id;
   FModifier *fcm = (FModifier *)ptr->data;
@@ -751,6 +751,36 @@ static void rna_FModifier_update(Main *bmain, Scene *UNUSED(scene), PointerRNA *
     calchandles_fcurve(fcm->curve);
   }
 
+  /* Call FCurve's corresponding property's RNA update function. */
+  FCurve *fcu = fcm->curve;
+  if (fcu && fcu->rna_path) {
+    ListBase *lb;
+    ID *id_iter;
+    bAction *act = (bAction *)id;
+
+    /* Have to find all users of this action. */
+    FOREACH_MAIN_LISTBASE_BEGIN (bmain, lb) {
+      FOREACH_MAIN_LISTBASE_ID_BEGIN (lb, id_iter) {
+        AnimData *adt = BKE_animdata_from_id(id_iter);
+
+        if (ELEM(NULL, adt, adt->action)) {
+          continue;
+        }
+
+        if (adt->action == act) {
+          PointerRNA id_ptr, prop_ptr;
+          PropertyRNA *prop;
+          RNA_id_pointer_create(id_iter, &id_ptr);
+          if (RNA_path_resolve_property(&id_ptr, fcu->rna_path, &prop_ptr, &prop)) {
+            RNA_property_update_main(bmain, scene, &prop_ptr, prop);
+          }
+        }
+      }
+      FOREACH_MAIN_LISTBASE_ID_END;
+    }
+    FOREACH_MAIN_LISTBASE_END;
+  }
+
   rna_tag_animation_update(bmain, id, true);
 }

Will test further and submit after the weekend

This seems to work nicely (and should enable us to get rid of lots of custom update code in FModifiers UI code) [P1658: #80121 WIP](https://archive.blender.org/developer/P1658.txt) ``` diff --git a/source/blender/makesrna/intern/rna_fcurve.c b/source/blender/makesrna/intern/rna_fcurve.c index ac4395f08c9..362765ecf85 100644 --- a/source/blender/makesrna/intern/rna_fcurve.c +++ b/source/blender/makesrna/intern/rna_fcurve.c @@ -742,7 +742,7 @@ static void rna_FModifier_blending_range( *max = fcm->efra - fcm->sfra; } -static void rna_FModifier_update(Main *bmain, Scene *UNUSED(scene), PointerRNA *ptr) +static void rna_FModifier_update(Main *bmain, Scene *scene, PointerRNA *ptr) { ID *id = ptr->owner_id; FModifier *fcm = (FModifier *)ptr->data; @@ -751,6 +751,36 @@ static void rna_FModifier_update(Main *bmain, Scene *UNUSED(scene), PointerRNA * calchandles_fcurve(fcm->curve); } + /* Call FCurve's corresponding property's RNA update function. */ + FCurve *fcu = fcm->curve; + if (fcu && fcu->rna_path) { + ListBase *lb; + ID *id_iter; + bAction *act = (bAction *)id; + + /* Have to find all users of this action. */ + FOREACH_MAIN_LISTBASE_BEGIN (bmain, lb) { + FOREACH_MAIN_LISTBASE_ID_BEGIN (lb, id_iter) { + AnimData *adt = BKE_animdata_from_id(id_iter); + + if (ELEM(NULL, adt, adt->action)) { + continue; + } + + if (adt->action == act) { + PointerRNA id_ptr, prop_ptr; + PropertyRNA *prop; + RNA_id_pointer_create(id_iter, &id_ptr); + if (RNA_path_resolve_property(&id_ptr, fcu->rna_path, &prop_ptr, &prop)) { + RNA_property_update_main(bmain, scene, &prop_ptr, prop); + } + } + } + FOREACH_MAIN_LISTBASE_ID_END; + } + FOREACH_MAIN_LISTBASE_END; + } + rna_tag_animation_update(bmain, id, true); } ``` Will test further and submit after the weekend

Philipp Oeser commented

2020-09-25 17:31:50 +02:00

Added subscriber: @HooglyBoogly

Philipp Oeser commented

2020-09-25 17:31:50 +02:00

@HooglyBoogly : I'll tweak the whole FModifier UI further in this respect, just getting you onboard early: do you think there is anything speaking againts this approach?
(the looping over all IDs is a bit unfortunate, but I dont see a quick way to solve this nicely)

@HooglyBoogly : I'll tweak the whole FModifier UI further in this respect, just getting you onboard early: do you think there is anything speaking againts this approach? (the looping over all IDs is a bit unfortunate, but I dont see a quick way to solve this nicely)

Hans Goudey commented

2020-09-25 17:42:26 +02:00

@lichtwerk I assume you're referring to D7997?

I'll push the UI team to sign off on that patch. I've significantly cleaned up the UI code for FModifiers there, in many cases using proper RNA buttons which should connect to the RNA update functions.
There are still a few cases of deg_update left after that patch.

From a pretty quick look I'll say that iterating over the entire bmain database does seem like an expensive thing to do in an RNA update function, but maybe that's fine.

@lichtwerk I assume you're referring to [D7997](https://archive.blender.org/developer/D7997)? I'll push the UI team to sign off on that patch. I've significantly cleaned up the UI code for FModifiers there, in many cases using proper RNA buttons which should connect to the RNA update functions. There are still a few cases of `deg_update` left after that patch. From a pretty quick look I'll say that iterating over the entire bmain database does seem like an expensive thing to do in an RNA update function, but maybe that's fine.

Philipp Oeser commented

2020-09-25 18:02:44 +02:00

In #80121#1023368, @HooglyBoogly wrote:
@lichtwerk I assume you're referring to D7997?

I'll push the UI team to sign off on that patch. I've significantly cleaned up the UI code for FModifiers there, in many cases using proper RNA buttons which should connect to the RNA update functions.
There are still a few cases of deg_update left after that patch.

From a pretty quick look I'll say that iterating over the entire bmain database does seem like an expensive thing to do in an RNA update function, but maybe that's fine.

Ah, from quick glance D7997 is what I meant with further cleanup.
But also from quick glance it would seem that even with D7997 we dont get the RNA updates from the underlying FCurve's property?
(so e.g. with the file from this report, you still should get the cache cleared when tweaking values in the FModifier? -- will also check later...)

> In #80121#1023368, @HooglyBoogly wrote: > @lichtwerk I assume you're referring to [D7997](https://archive.blender.org/developer/D7997)? > > I'll push the UI team to sign off on that patch. I've significantly cleaned up the UI code for FModifiers there, in many cases using proper RNA buttons which should connect to the RNA update functions. > There are still a few cases of `deg_update` left after that patch. > > From a pretty quick look I'll say that iterating over the entire bmain database does seem like an expensive thing to do in an RNA update function, but maybe that's fine. Ah, from quick glance [D7997](https://archive.blender.org/developer/D7997) is what I meant with further cleanup. But also from quick glance it would seem that even with [D7997](https://archive.blender.org/developer/D7997) we dont get the RNA updates from the underlying FCurve's property? (so e.g. with the file from this report, you still should get the cache cleared when tweaking values in the FModifier? -- will also check later...)

Hans Goudey commented

2020-09-25 22:45:46 +02:00

Oh, you're right, and I think I get the problem now. Passing updates to a property / struct owner doesn't seem to be considered very much in RNA's design. I think RNAUpdateCb is used in interface_templates.c to get around this same problem.

In that case, I'm not sure why this is only broken for forcefields. Huh

Oh, you're right, and I think I get the problem now. Passing updates to a property / struct owner doesn't seem to be considered very much in RNA's design. I think `RNAUpdateCb` is used in `interface_templates.c` to get around this same problem. In that case, I'm not sure why this is only broken for forcefields. Huh

Philipp Oeser commented

2020-09-25 22:57:53 +02:00

I assume FModifiers are updating less than promised (aka "broken") for more than Forcefields, yes. Might be that DEG tagging is enough for many things, but not the same as proper RNA update functions.

Sybren A. Stüvel commented

2020-09-28 12:41:21 +02:00

which now tags the Action ID_RECALC_ANIMATION, which also sounds pretty useless

The Action depsgraph node contains an Animation component. By tagging it, the users of the action are also re-evaluated. Here is an example depsgraph of two cubes using the same action:

Might be that DEG tagging is enough for many things, but not the same as proper RNA update functions.

In that case I think that we should move code out of the RNA update functions. Looping over all datablocks just to find the users of an action shouldn't be necessary, the depsgraph already contains the required information (and if it doesn't, maybe that's a sign that it should be extended).

> which now tags the Action ID_RECALC_ANIMATION, which also sounds pretty useless The Action depsgraph node contains an Animation component. By tagging it, the users of the action are also re-evaluated. Here is an example depsgraph of two cubes using the same action: ![depsgraph.png](https://archive.blender.org/developer/F8931321/depsgraph.png) > Might be that DEG tagging is enough for many things, but not the same as proper RNA update functions. In that case I think that we should move code out of the RNA update functions. Looping over all datablocks just to find the users of an action shouldn't be necessary, the depsgraph already contains the required information (and if it doesn't, maybe that's a sign that it should be extended).

Philipp Oeser commented

2020-09-28 13:03:57 +02:00

OK, thx clearing that up, will have to dig a little deeper then

Philipp Oeser commented

2020-09-28 14:06:43 +02:00

Added subscriber: @brecht

Philipp Oeser commented

2020-09-28 14:06:43 +02:00

So the reason (on the depsgraph side) why rna_FieldSettings_update causes the pointcache to be cleared is tagging it ID_RECALC_TRANSFORM there.
If this is done, pointcache gets cleared during evaluation because of this relation in DEG_add_forcefield_relations:

DEG_add_object_pointcache_relation(handle, relation->ob, DEG_OB_COMP_TRANSFORM, name);

On the FModifier UI side, the Action is tagged ID_RECALC_ANIMATION.
If that is done, sure, the animation system will eventually call animsys_evaluate_fcurves, that in turn will set the RNA property, but it will not update the property.

This is what I meant by "The animation system does not call RNA update functions IIRC, usually the UI does though."
This is old, see e.g.

In blender/blender-addons#28286#103015, @brecht wrote:
Property update function are never called on e.g. frame change for animation, only when changing the property in the UI, editing the fcurve, etc. The update function is intended to be quick, and builtin properties usually will tag the datablock for update in the depsgraph, and then the actual update will be done delayed before the next redraw in object_handle_update.

Regarding this:

In that case I think that we should move code out of the RNA update functions

I am not sure about this. IIRC, it was even the case once that code in RNA update functions was called from the animation system, but changed for performance reasons.
There is just two levels of updating things. Moving the particular pointcache relation from DEG_OB_COMP_TRANSFORM to generic DEG_OB_COMP_ANIMATION in the depsgraph would probably just slow things down too much (tested this, but wasnt working at all)?
On the other hand, I'll have to do some more learning to see if DEG could be granular enough to handle forcefields better than just DEG_OB_COMP_TRANSFORM (or DEG_OB_COMP_GEOMETRY)

Still think that for tweaking stuff in the UI one could expect the RNA updates to be called, D9031: FModifers RNA update: call corresponding FCurve properties RNA update as well could actually work (dont know how to get around the looping though)

So the reason (on the depsgraph side) why `rna_FieldSettings_update` causes the pointcache to be cleared is tagging it `ID_RECALC_TRANSFORM` there. If this is done, pointcache gets cleared during evaluation because of this relation in `DEG_add_forcefield_relations`: ``` DEG_add_object_pointcache_relation(handle, relation->ob, DEG_OB_COMP_TRANSFORM, name); ``` On the FModifier UI side, the Action is tagged `ID_RECALC_ANIMATION`. If that is done, sure, the animation system will eventually call `animsys_evaluate_fcurves`, that in turn will **set** the RNA property, but it will not **update** the property. This is what I meant by "The animation system does not call RNA update functions IIRC, usually the UI does though." This is old, see e.g. > In blender/blender-addons#28286#103015, @brecht wrote: > Property update function are never called on e.g. frame change for animation, only when changing the property in the UI, editing the fcurve, etc. The update function is intended to be quick, and builtin properties usually will tag the datablock for update in the depsgraph, and then the actual update will be done delayed before the next redraw in object_handle_update. Regarding this: > In that case I think that we should move code out of the RNA update functions I am not sure about this. IIRC, it was even the case once that code in RNA update functions was called from the animation system, but changed for performance reasons. There is just two levels of updating things. Moving the particular pointcache relation from `DEG_OB_COMP_TRANSFORM` to generic `DEG_OB_COMP_ANIMATION` in the depsgraph would probably just slow things down too much (tested this, but wasnt working at all)? On the other hand, I'll have to do some more learning to see if DEG could be granular enough to handle forcefields better than just DEG_OB_COMP_TRANSFORM (or DEG_OB_COMP_GEOMETRY) Still think that for tweaking stuff in the UI one could expect the RNA updates to be called, [D9031: FModifers RNA update: call corresponding FCurve properties RNA update as well](https://archive.blender.org/developer/D9031) could actually work (dont know how to get around the looping though)

Philipp Oeser was unassigned by Sybren A. Stüvel

2020-09-28 17:12:17 +02:00

Sybren A. Stüvel self-assigned this 2020-09-28 17:12:17 +02:00

blender-admin commented

2020-09-28 17:13:40 +02:00

This issue was referenced by 48a0c931ee

This issue was referenced by 48a0c931eea84fb072ce0b958090dda4b27cabff

Sybren A. Stüvel commented

2020-09-28 17:25:55 +02:00

Changed status from 'Confirmed' to: 'Resolved'

Sybren A. Stüvel closed this issue

2020-09-28 17:25:55 +02:00

Sign in to join this conversation.

5 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#80121

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Forcefield F-curve modifier changes don't reset frames in memory #80121