Pose Breakdowner bug in Gooseberry rigs #44219

New Issue

Juan Pablo Bouza · 2015-04-01T16:56:58+02:00

Juan Pablo Bouza commented

2015-04-01 16:56:58 +02:00

Hi I've attached a file with Victor's rig.

I'm sorry for not simplifying this, but it is a bug that I have known of for a very long time but was never able to recreate. Now I finally have an action where it pops up.

Just open the file and press shift+E (breakdowner), move the mouse a bit to modify the pose, and you'll most likely notice that the custom property that the bone has gets modified by the breakdowner. The bad thing is that this property drives the Rotation Order, so this issue can cause huge animation problems.

Just to make this clear, you'll notice that in the action the property is keyed and changes its values, well, those changes happened randomly, it wasn't Beorn who modified those values manually.

So, the thing is that the breakdowner makes values of bone custom properties jump "randomly".

[breakdowner_bug.blend](https://archive.blender.org/developer/F157089/breakdowner_bug.blend) Hi I've attached a file with Victor's rig. I'm sorry for not simplifying this, but it is a bug that I have known of for a very long time but was never able to recreate. Now I finally have an action where it pops up. Just open the file and press shift+E (breakdowner), move the mouse a bit to modify the pose, and you'll most likely notice that the custom property that the bone has gets modified by the breakdowner. The bad thing is that this property drives the Rotation Order, so this issue can cause huge animation problems. Just to make this clear, you'll notice that in the action the property is keyed and changes its values, well, those changes happened randomly, it wasn't Beorn who modified those values manually. So, the thing is that the breakdowner makes values of bone custom properties jump "randomly".

Juan Pablo Bouza commented

2015-04-01 16:56:58 +02:00

Changed status to: 'Open'

Juan Pablo Bouza commented

2015-04-01 16:56:58 +02:00

Added subscriber: @jpbouza-4

Campbell Barton was assigned by Lukas Tönne

2015-04-01 17:42:34 +02:00

Lukas Tönne commented

2015-04-01 17:42:34 +02:00

Added subscriber: @LukasTonne

Lukas Tönne commented

2015-04-01 17:42:34 +02:00

The issue is caused by an fcurve animating the rotation_mode property. This is probably not intentional, but since it's generally allowed it should be handled gracefully. When using the "breakdowner" tool the fcurve at some point sets an invalid enum value outside the allowed -1..6 range for rotation modes:

https://developer.blender.org/diffusion/B/browse/gooseberry/source/blender/makesrna/intern/rna_pose.c$275

This leads to invalid access of the static rotOrders array and subsequent random memory and crash.

https://developer.blender.org/diffusion/B/browse/gooseberry/source/blender/blenlib/intern/math_rotation.c$1375

The issue is caused by an fcurve animating the `rotation_mode` property. This is probably not intentional, but since it's generally allowed it should be handled gracefully. When using the "breakdowner" tool the fcurve at some point sets an invalid enum value outside the allowed -1..6 range for rotation modes: https://developer.blender.org/diffusion/B/browse/gooseberry/source/blender/makesrna/intern/rna_pose.c$275 This leads to invalid access of the static `rotOrders` array and subsequent random memory and crash. https://developer.blender.org/diffusion/B/browse/gooseberry/source/blender/blenlib/intern/math_rotation.c$1375

Lukas Tönne commented

2015-04-01 17:44:17 +02:00

Added subscriber: @ideasman42

Lukas Tönne commented

2015-04-01 17:44:17 +02:00

@ideasman42: How can we ensure fcurves on enum properties don't cause undefined results? Should this be handled at fcurves, RNA or BLI_math?

Beorn Leonard commented

2015-04-01 18:00:22 +02:00

Added subscriber: @BeornLeonard

Juan Pablo Bouza commented

2015-04-01 18:08:24 +02:00

If I understand this correctly, the undefined results causes a crash.

So we have two issues, this crash, and the original cause of the bug report, which is that the Breakdowner should not change the value of this custom property.

Furthermore, if you have two keys in the property, with a value of 1.0, when the breakdowner supposedly interpolates between 1.0 and 1.0, it might throw a value different to 1.0, which doesn't make sense... (at least from a simple user point of view)

If I understand this correctly, the undefined results causes a crash. So we have two issues, this crash, and the original cause of the bug report, which is that the Breakdowner should not change the value of this custom property. Furthermore, if you have two keys in the property, with a value of 1.0, when the breakdowner supposedly interpolates between 1.0 and 1.0, it might throw a value different to 1.0, which doesn't make sense... (at least from a simple user point of view)

Joshua Leung commented

2015-04-06 14:22:26 +02:00

Added subscriber: @JoshuaLeung

Campbell Barton commented

2015-04-07 01:04:32 +02:00

Dont see a really nice way to resolve this...

The correct solution is likely to be:

FCurve code would get the enum. Search for the value and check it exists in the enum, - before assigning.

However this would give some reasonable overhead- especiallty for dynamically allocated enums.

We could instead define that animatable enums must clamp their input in the set function.

Dont see a really nice way to resolve this... The correct solution is likely to be: FCurve code would get the enum. Search for the value and check it exists in the enum, - before assigning. However this would give some reasonable overhead- especiallty for dynamically allocated enums. We could instead define that animatable enums must clamp their input in the set function.

Joshua Leung commented

2015-04-07 01:37:25 +02:00

I just spotted this last night.... (Why I didn't notice it before is another matter)

Having had a look this morning, the fix would be simple if the custom properties in question were proper enums (and not ints). Specifically, that would open up two possibilities:

In pose_slide.c :: pose_slide_apply_props() - we could just knock out line 331. This way, the breakdown tools would not try to blend between these values, since AFAIK, in most cases, they're pretty much discrete/different behaviours instead of being a set of logical continuous values
In pose_slide.c again, we could also check for the FCURVE_DISCRETE_VALUES flag on the FCurves that are being blended. This flag will get set if the keyframing code detects that the property in question is an enum or boolean, which shouldn't be blended between intermediate values

While this still wouldn't solve the issue of invalid values getting used via other places (i.e edited FCurves), it would solve the immediate problem here. What is unclear though is whether custom properties can be defined in a way that they adopt the enum values from another enum. Maybe it's possible, maybe not; I'm not familiar enough with bpy to say.

As for the second (and more serious issue) with RNA:
Indeed, it does look like having the FCurve code check whether it's about to set a valid value is one of the better options considering what else goes on in Blender. I'm not sure how the Python API does it, but I'm sure it has to do something similar since it throws exceptions about this, while the UI generally doesn't have this problem (since it only shows valid values anyway).

Dynamic enums are an issue too of course. If we have a way of knowing the size in advance for these (without having to just generate them), maybe we could guess? Otherwise, we may just leave these unclamped.

Initially, I was leaning towards favouring making the RNA set callbacks check their own values themselves. In theory, they should know best what to expect, and this may allow some optimisations. The only problem is that this does also result in makesrna generating a lot more code to perform all this checking, meaning that our binary sizes are likely to see another good bump. Another potential issue is that, by introducing the check at RNA level, any other checks performed elsewhere will now result in a double-up of checking, which is a slight performance cost. That said, in the interests of having a good + solid API, putting these checks there is the best option, since any other RNA frontends which may crop up (e.g. the C++ api, extra language bindings?) will all have these abilities baked in by default. Somewhat negating that argument though is that we don't have any plans to introduce anything more ;)

Finally, regarding @jpbouza-4's point about 1.0 to 1.0: Good point! Those checks should be simple to add

I just spotted this last night.... (Why I didn't notice it before is another matter) Having had a look this morning, the fix would be simple if the custom properties in question were proper enums (and not ints). Specifically, that would open up two possibilities: 1) In pose_slide.c :: pose_slide_apply_props() - we could just knock out line 331. This way, the breakdown tools would not try to blend between these values, since AFAIK, in most cases, they're pretty much discrete/different behaviours instead of being a set of logical continuous values 2) In pose_slide.c again, we could also check for the FCURVE_DISCRETE_VALUES flag on the FCurves that are being blended. This flag will get set if the keyframing code detects that the property in question is an enum or boolean, which shouldn't be blended between intermediate values While this still wouldn't solve the issue of invalid values getting used via other places (i.e edited FCurves), it would solve the immediate problem here. What is unclear though is whether custom properties can be defined in a way that they adopt the enum values from another enum. Maybe it's possible, maybe not; I'm not familiar enough with bpy to say. --- As for the second (and more serious issue) with RNA: Indeed, it does look like having the FCurve code check whether it's about to set a valid value is one of the better options considering what else goes on in Blender. I'm not sure how the Python API does it, but I'm sure it has to do something similar since it throws exceptions about this, while the UI generally doesn't have this problem (since it only shows valid values anyway). Dynamic enums are an issue too of course. If we have a way of knowing the size in advance for these (without having to just generate them), maybe we could guess? Otherwise, we may just leave these unclamped. Initially, I was leaning towards favouring making the RNA set callbacks check their own values themselves. In theory, they should know best what to expect, and this may allow some optimisations. The only problem is that this does also result in makesrna generating a lot more code to perform all this checking, meaning that our binary sizes are likely to see another good bump. Another potential issue is that, by introducing the check at RNA level, any other checks performed elsewhere will now result in a double-up of checking, which is a slight performance cost. That said, in the interests of having a good + solid API, putting these checks there is the best option, since any other RNA frontends which may crop up (e.g. the C++ api, extra language bindings?) will all have these abilities baked in by default. Somewhat negating that argument though is that we don't have any plans to introduce anything more ;) --- Finally, regarding @jpbouza-4's point about 1.0 to 1.0: Good point! Those checks should be simple to add

Juan Pablo Bouza commented

2015-04-07 04:35:12 +02:00

OK... well if it makes things simpler, I would also contemplate the possibility that the breakdowner doesn't take into account custom properties!

After all, the tool is called "pose breakdowner". not property breakdowner!

If you think of it, this feature is meant for animation, and custom properties belong more to the rigging world than to any other...

What I want to say is that I wouldn't go crazy finding a solution to this, even less if it impacts on performance... I don't think anyone would use the breakdowner to interpolate custom properties values... it's more simple to just set the numbers manually.

OK... well if it makes things simpler, I would also contemplate the possibility that the breakdowner doesn't take into account custom properties! After all, the tool is called "pose breakdowner". not property breakdowner! If you think of it, this feature is meant for animation, and custom properties belong more to the rigging world than to any other... What I want to say is that I wouldn't go crazy finding a solution to this, even less if it impacts on performance... I don't think anyone would use the breakdowner to interpolate custom properties values... it's more simple to just set the numbers manually.

Joshua Leung commented

2015-04-07 04:48:59 +02:00

@jpbouza-4: Well, IIRC rigs like the Sintel rigs and/or some of the rigify-based ones (last time I checked) are heavily reliant on custom properties for facial animation. So in those cases, custom properties aren't just tools for riggers to use to define the rig, but end up playing an integral role in defining what the pose should be! Hence, it makes sense for those that the breakdowner can be used.

Juan Pablo Bouza commented

2015-04-07 05:32:55 +02:00

Ok you win, just don't make blender slower :p

Beorn Leonard commented

2015-04-07 12:24:05 +02:00

It would be good to double check that. Rigify rigs use transforms for posing, and custom props for properties, like IK/FK and hinges, which you don't need to be included in the breakdowner..
From memory, the Sintel rig did indeed use custom properties for facial stuff. However, I can't double check that now, as last time I checked, the Sintel rig didn't work in the current Blender.

The point jpbouza makes is a valid one. There should be a separation between properties and pose controls, but maybe the solution is in how the rig is setup and/or whether the animator is using keying sets. (which I think most of us don't)

It would be good to double check that. Rigify rigs use transforms for posing, and custom props for properties, like IK/FK and hinges, which you don't need to be included in the breakdowner.. From memory, the Sintel rig did indeed use custom properties for facial stuff. However, I can't double check that now, as last time I checked, the Sintel rig didn't work in the current Blender. The point jpbouza makes is a valid one. There should be a separation between properties and pose controls, but maybe the solution is in how the rig is setup and/or whether the animator is using keying sets. (which I think most of us don't)

Joshua Leung commented

2015-04-08 04:45:30 +02:00

I've just committed two partial fixes which resolve some of the most immediate issues noted here, but not the underlying RNA problem. That is:

Enum properties will no longer be affected by the breakdowner. However, since the custom property causing trouble here is an int, this still doesn't change the situation for gooseberry rigs (unless other changes have happened since)
The cause of the actual crash has been fixed so that it can't happen in future - either from other bad FCurves/properties, or from bad code passing in wrong values

I've looked into adding some of the relevant checks into the animsys_write_setting() function for enums, but quickly discovered the problem of what should happen if we encounter an invalid value. If we simply skip setting values, this does lead to non-threadsafe behaviour (i.e. if it doesn't change the value when jumping around, the value you'll have will depend on what order you visited the frames in).

I've just committed two partial fixes which resolve some of the most immediate issues noted here, but not the underlying RNA problem. That is: 1) Enum properties will no longer be affected by the breakdowner. However, since the custom property causing trouble here is an int, this still doesn't change the situation for gooseberry rigs (unless other changes have happened since) 2) The cause of the actual crash has been fixed so that it can't happen in future - either from other bad FCurves/properties, or from bad code passing in wrong values I've looked into adding some of the relevant checks into the animsys_write_setting() function for enums, but quickly discovered the problem of what should happen if we encounter an invalid value. If we simply skip setting values, this does lead to non-threadsafe behaviour (i.e. if it doesn't change the value when jumping around, the value you'll have will depend on what order you visited the frames in).

Julian Eisel commented

2015-04-24 15:52:03 +02:00

Added subscriber: @JulianEisel

Julian Eisel commented

2015-04-24 15:52:03 +02:00

@JoshuaLeung, so what do you think, is this fixable? Let's move it to the todo if not.

Joshua Leung commented

2015-05-17 15:19:27 +02:00

Changed this to a todo.

The crash which sparked the initial bug report shouldn't happen now.
However, the issues with FCurves trying to write invalid values still stand. Those however have existed in Blender for a while, so I guess we can say that trying to fix this can go on the todolist for further investigation at a later date.

Changed this to a todo. * The crash which sparked the initial bug report shouldn't happen now. * However, the issues with FCurves trying to write invalid values still stand. Those however have existed in Blender for a while, so I guess we can say that trying to fix this can go on the todolist for further investigation at a later date.

Campbell Barton removed their assignment 2016-08-11 06:35:19 +02:00

Christoph Lendenfeld commented

2020-11-12 18:03:36 +01:00

Added subscriber: @ChrisLend

Christoph Lendenfeld commented

2020-11-12 18:03:36 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Christoph Lendenfeld closed this issue

2020-11-12 18:03:36 +01:00

Christoph Lendenfeld self-assigned this 2020-11-12 18:03:36 +01:00

Christoph Lendenfeld commented

2020-11-12 18:03:36 +01:00

This can be seen as resolved.
The issue with RNA goes deeper and would need a Design task, but it hasn't been an issue since.
Except that there is a regression that enum properties crash blender #82669

This can be seen as resolved. The issue with RNA goes deeper and would need a Design task, but it hasn't been an issue since. Except that there is a regression that enum properties crash blender #82669

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Pose Breakdowner bug in Gooseberry rigs #44219