[CRASH] [EDITOR] Animation Modifiers will crash the editor if you edit a curve that the modifier creates

Development Pipeline & Plugins
, ,
1

To specifically fix the moving keys issue, it’s possible to make the two changes

* FRichCurveEditorModel::SetKeyPositions - remove direct call to Owner->PostEditChangeProperty

* FRichCurveEditorModelNamed::OnModelHasChanged - add call to AnimSequence->PostEditChange(); when scope is finally closed

But other changes to the curve will still cause the crash, so a larger refactor is required.

2

Steps to Reproduce

  1. Create an animation modifier that adds a float curve named “footplant_l” and has Reapply Post Owner Change = true
  2. Add that modifier to an animation sequence and run it
  3. Edit the “footplant_l” curve, moving a key
  4. It will crash because it will remove the curve we are currently editing
3

Hi, sorry for the delay getting back to you on this one. I managed to get a similar repro to the one that you described. This one is going to be a bit tricky to get a solution for since it potentially affects quite a lot of code. I’m going to discuss it further with the dev team and I’ll follow up once we have a better idea of whether we’ll go down the route that you have or look at a different solution.

4

Okay, so the approach we’re looking at taking to fix this is different from yours. The dev team want to address the issue with removing curves specifically. To do that, we plan to defer any removal of a curve to the next tick of the curve editor, rather than removing the curve immediately (I can share a CL if you’re interested). This would mean that the curve editor can modify the curve, and then the modifier’s request to remove the curve will be executed on the next frame.

There were some concerns around moving PostEditChangeProperty, which is why the team is looking at a different solution. The curve editor isn’t only used for editing curves on animations, it’s also used by Niagara, etc, but the OnModelHasChanged is only bound to changes in the animation data model. So we’d also need to implement similar delegates for other data types that use rich curves.

The dev team also think that there’s still room for a crash to happen with the call to PostEditChange in OnModelHasChanged, since it may still be possible for something to modify the curve after that point but before the bracket has closed.

I have a couple of follow-up questions about your setup. Do you ever run into this issue with the curve isn’t removed, or does the modifier always have to remove the curve in some way for the crash to happen? And are you able to share your changes to OnModelHasChanged?

5

Yes, for this crash to happen, it’s when an animation modifier removes the curve. The only other similar crash is the one I posted about on [Content removed]

We haven’t made any changes to OnModelHasChanged, we’re currently going with “don’t touch those curves, the editor will crash if you do” solution.

6

Ok thanks, in that case we’ll go ahead with the change to just prevent the crash when removing curves. We have a shelf with the changes at 48511675 if you want to try that out? They should be simple to integrate.