blender/blender
142
451
Fork 651
Code Issues 6.5k Pull Requests 823 Projects 18 Wiki Activity

Drivers: implement fallback values for RNA path based variables. #110135

Merged
Alexander Gavrilov merged 3 commits from angavrilov/blender:pr-driver-var-default into main 2024-01-08 15:25:08 +01:00
Conversation 29 Commits 3 Files Changed 13 +446 -57
Alexander Gavrilov commented 2023-07-15 13:09:53 +02:00
Member
Copy Link

As discussed in #105407, it can be useful to support returning
a fallback value specified by the user instead of failing the driver
if a driver variable cannot resolve its RNA path. This especially
applies to context variables referencing custom properties, since
when the object with the driver is linked into another scene, the
custom property can easily not exist there.

This patch adds an optional fallback value setting to properties
based on RNA path (including ordinary Single Property variables
due to shared code and similarity). When enabled, RNA path lookup
failures (including invalid array index) cause the fallback value
to be used instead of marking the driver invalid.

A flag is added to track when this happens for UI use. It is
also exposed to python for lint type scripts.

When the fallback value is used, the input field containing
the property RNA path that failed to resolve is highlighted in red
(identically to the case without a fallback), and the driver
can be included in the With Errors filter of the Drivers editor.
However, the channel name is not underlined in red, because
the driver as a whole evaluates successfully.

This PR will be pushed as a sequence of 3 commits separating major logical changes:

  • Drivers Editor: apply red underline to drivers that failed evaluation.
  • Drivers: refactor driver_get_variable_property to return an enum.
  • Drivers: implement fallback values for RNA path based variables.

This comment has an attached file that demonstrates a hypothetical use case for this feature.

#105407 (comment)

Specifically, I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context.

Without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot below, defaults for drivers allow implementing those behaviors there as well.

The same applies to any scene-global settings, e.g. a hypothetical example by @Mets:

  • I drive some crowd properties like Shape Keys and Materials, with Scene custom properties.
  • I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties.
  • With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created).

| image | image |

As discussed in #105407, it can be useful to support returning a fallback value specified by the user instead of failing the driver if a driver variable cannot resolve its RNA path. This especially applies to context variables referencing custom properties, since when the object with the driver is linked into another scene, the custom property can easily not exist there. This patch adds an optional fallback value setting to properties based on RNA path (including ordinary Single Property variables due to shared code and similarity). When enabled, RNA path lookup failures (including invalid array index) cause the fallback value to be used instead of marking the driver invalid. A flag is added to track when this happens for UI use. It is also exposed to python for lint type scripts. When the fallback value is used, the input field containing the property RNA path that failed to resolve is highlighted in red (identically to the case without a fallback), and the driver can be included in the With Errors filter of the Drivers editor. However, the channel name is not underlined in red, because the driver as a whole evaluates successfully. ------ This PR will be pushed as a sequence of 3 commits separating major logical changes: * Drivers Editor: apply red underline to drivers that failed evaluation. * Drivers: refactor driver_get_variable_property to return an enum. * Drivers: implement fallback values for RNA path based variables. ------ This comment has an attached file that demonstrates a hypothetical use case for this feature. https://projects.blender.org/blender/blender/issues/105407#issuecomment-980756 Specifically, I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context. Without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot below, defaults for drivers allow implementing those behaviors there as well. The same applies to any scene-global settings, e.g. a hypothetical example by @Mets: * I drive some crowd properties like Shape Keys and Materials, with Scene custom properties. * I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties. * With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created). | ![image](https://projects.blender.org/attachments/e17f2237-669f-40ff-9832-afcd15f8795a) | ![image](https://projects.blender.org/attachments/9aaff4fc-b1ef-4808-9ce1-82e5c6a0633d) |
Screenshot_20230731_175238.png
29 KiB
Screenshot_20230731_175133.png
63 KiB
Alexander Gavrilov requested review from Sergey Sharybin 2023-07-15 13:10:31 +02:00
Alexander Gavrilov added the
Interest
Animation & Rigging
 label 2023-07-15 13:10:46 +02:00
Alexander Gavrilov force-pushed pr-driver-var-default from e085f61ab4 to 1a7dc42620 2023-07-16 14:19:39 +02:00 Compare
Alexander Gavrilov commented 2023-07-16 14:20:17 +02:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-07-16 14:20:21 +02:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110135) when ready.
Sergey Sharybin requested review from Nathan Vegdahl 2023-07-17 09:54:19 +02:00
Sergey Sharybin requested review from Sybren A. Stüvel 2023-07-17 09:54:19 +02:00
Sergey Sharybin commented 2023-07-17 11:05:59 +02:00
Owner
Copy Link

I am not sure about the following two things:

  • Why do we need the checkbox next to the default value? Can it be just a Default Value slider, with a default value of 0 (which matches the current implicit behavior, I believe). And then if rigger wants it to be different they just modify a single value.

  • What is the flag which indicates the default value was actually used is used for.

Can please you elaborate on this?

I am not sure about the following two things: - Why do we need the checkbox next to the default value? Can it be just a `Default Value` slider, with a default value of 0 (which matches the current implicit behavior, I believe). And then if rigger wants it to be different they just modify a single value. - What is the flag which indicates the default value was actually used is used for. Can please you elaborate on this?
Alexander Gavrilov commented 2023-07-17 11:53:39 +02:00
Author
Member
Copy Link
  • Why do we need the checkbox next to the default value? Can it be just a Default Value slider, with a default value of 0 (which matches the current implicit behavior, I believe). And then if rigger wants it to be different they just modify a single value.

It does not match the implicit behavior, because the implicit behavior marks the driver as invalid, which I think actually blocks it from subsequent evaluation. I.e. it is a hard error.

  • What is the flag which indicates the default value was actually used is used for.

Internally it is used to still highlight the path in red, even though the invalid flag isn't applied. I also expose it to python api for scripts to use if they want (the invalid flag on the drivers is likewise exposed).

By the way, I wonder if a default fallback should include the driver in the drivers with errors filter or not? One one hand the lookup still fails, on the other hand the whole point is that it is 'expected' behavior (in the fallback example image it is truly expected for some lookups to always fail).

> - Why do we need the checkbox next to the default value? Can it be just a `Default Value` slider, with a default value of 0 (which matches the current implicit behavior, I believe). And then if rigger wants it to be different they just modify a single value. It does not match the implicit behavior, because the implicit behavior marks the driver as invalid, which I think actually blocks it from subsequent evaluation. I.e. it is a hard error. > - What is the flag which indicates the default value was actually used is used for. Internally it is used to still highlight the path in red, even though the invalid flag isn't applied. I also expose it to python api for scripts to use if they want (the invalid flag on the drivers is likewise exposed). By the way, I wonder if a default fallback should include the driver in the drivers with errors filter or not? One one hand the lookup still fails, on the other hand the whole point is that it is 'expected' behavior (in the fallback example image it is truly expected for some lookups to always fail).
Nathan Vegdahl commented 2023-07-18 18:16:09 +02:00
Member
Copy Link

Another possible design for this would be for invalid variables to simply evaluate to None, which can then be checked for in a Scripted Expression driver:

v if v != None else s if s != None else w

Not quite as user friendly, and certainly less discoverable. But depending on how often we expect use cases that need this to come up, it could make sense.

Another possible design for this would be for invalid variables to simply evaluate to `None`, which can then be checked for in a `Scripted Expression` driver: ``` v if v != None else s if s != None else w ``` Not quite as user friendly, and certainly less discoverable. But depending on how often we expect use cases that need this to come up, it could make sense.
Alexander Gavrilov commented 2023-07-19 08:57:24 +02:00
Author
Member
Copy Link

Another possible design for this would be for invalid variables to simply evaluate to None, which can then be checked for in a Scripted Expression driver:

It can't really, it has to be a number. Simple Expressions only support floating point values.

You can use NaN though, you just need to allow entering it through the UI as the default value, and support isfinite() in simple expressions.

> Another possible design for this would be for invalid variables to simply evaluate to `None`, which can then be checked for in a `Scripted Expression` driver: It can't really, it has to be a number. Simple Expressions only support floating point values. You can use NaN though, you just need to allow entering it through the UI as the default value, and support isfinite() in simple expressions.
Sergey Sharybin commented 2023-07-19 11:22:03 +02:00
Owner
Copy Link

It does not match the implicit behavior, because the implicit behavior marks the driver as invalid, which I think actually blocks it from subsequent evaluation. I.e. it is a hard error.

Ah, right.

Internally it is used to still highlight the path in red, even though the invalid flag isn't applied. I also expose it to python api for scripts to use if they want (the invalid flag on the drivers is likewise exposed).

By the way, I wonder if a default fallback should include the driver in the drivers with errors filter or not? One one hand the lookup still fails, on the other hand the whole point is that it is 'expected' behavior (in the fallback example image it is truly expected for some lookups to always fail).

It should be somehow clear in the interface that the default value is used, but it should not be displayed in the same way as error.

> It does not match the implicit behavior, because the implicit behavior marks the driver as invalid, which I think actually blocks it from subsequent evaluation. I.e. it is a hard error. Ah, right. > Internally it is used to still highlight the path in red, even though the invalid flag isn't applied. I also expose it to python api for scripts to use if they want (the invalid flag on the drivers is likewise exposed). > > By the way, I wonder if a default fallback should include the driver in the drivers with errors filter or not? One one hand the lookup still fails, on the other hand the whole point is that it is 'expected' behavior (in the fallback example image it is truly expected for some lookups to always fail). It should be somehow clear in the interface that the default value is used, but it should not be displayed in the same way as error.
Alexander Gavrilov commented 2023-07-19 11:26:36 +02:00
Author
Member
Copy Link

It should be somehow clear in the interface that the default value is used, but it should not be displayed in the same way as error.

The way I see it, the fact that the path fails to resolve is the same so it's fine to highlight it as an error. Basically the default value feature is like a try {} catch(NotResolved) { return default; } in a way.

> It should be somehow clear in the interface that the default value is used, but it should not be displayed in the same way as error. The way I see it, the fact that the path fails to resolve is the same so it's fine to highlight it as an error. Basically the default value feature is like a `try {} catch(NotResolved) { return default; }` in a way.
Nathan Vegdahl commented 2023-07-20 11:08:53 +02:00
Member
Copy Link

I've put this on the itinerary to discuss at the next Animation & Rigging module meeting (Thursday 2023-07-27 18:00 CEST/Amsterdam time), because I think it would be good to get some feedback from riggers there on how they would want it to work.

It can't really, it has to be a number. Simple Expressions only support floating point values.

It doesn't have to be a Simple Expression. My suspicion (though I could be wrong) is that only a minority of drivers would be set up this way, in which case the benefits of avoiding Python evaluation wouldn't be especially important.

Having said that, we'd still need a checkbox to distinguish driver variables that are "allowed" to be None from those that aren't. So I agree that it's probably not the way to go, since we'll be adding UI either way, and this approach is less user friendly.

But it's always worth exploring alternative designs to help ensure that we land on the one with the most appropriate set of trade offs.

I've put this on the itinerary to discuss at the next Animation & Rigging module meeting (Thursday 2023-07-27 18:00 CEST/Amsterdam time), because I think it would be good to get some feedback from riggers there on how they would want it to work. > It can't really, it has to be a number. Simple Expressions only support floating point values. It doesn't have to be a Simple Expression. My suspicion (though I could be wrong) is that only a minority of drivers would be set up this way, in which case the benefits of avoiding Python evaluation wouldn't be especially important. Having said that, we'd still need a checkbox to distinguish driver variables that are "allowed" to be `None` from those that aren't. So I agree that it's probably not the way to go, since we'll be adding UI either way, and this approach is less user friendly. But it's always worth exploring alternative designs to help ensure that we land on the one with the most appropriate set of trade offs.
Alexander Gavrilov force-pushed pr-driver-var-default from 1a7dc42620 to db68e7bec9 2023-07-24 16:40:31 +02:00 Compare
Demeter Dzadik commented 2023-07-27 19:36:04 +02:00
Member
Copy Link

Sergey asked me about this in DMs some time ago, here's what I said (also repeated in the Anim Module meeting):

Example use case (entirely hypothetical):

  • I drive some crowd properties like Shape Keys and Materials, with Scene custom properties.
  • I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties.
  • With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created).

That said:

  • Do you @angavrilov have something concrete like the above, but real? I know there's a lot of previous discussions, I didn't go through all of that. I think if there's something like this in there, it needs to go into the PR text itself, because all this PR seems to need right now is to convince people that the feature is indeed useful. (Based on reactions in Anim Module meeting)
  • I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to only_show_errors. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good.

Off-topic / Suggestions for separate patch:

  • Right-click pop-up driver editor could be like 50% wider.
  • Spacing of the ID type selector is broken (bunch of empty space between it, and its "Prop:" label. Also, that label is inaccurate, it should say "ID:" or "Datablock:".)
  • Eyedropper button next to "Add Driver Variable" button doesn't work, and I only now noticed it exists, not sure what it's supposed to do? It only shows up in the right-click pop-up, not in the regular driver editor?
Sergey asked me about this in DMs some time ago, here's what I said (also repeated in the Anim Module meeting): Example use case (entirely hypothetical): - I drive some crowd properties like Shape Keys and Materials, with Scene custom properties. - I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties. - With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created). That said: - Do you @angavrilov have something concrete like the above, but real? I know there's a lot of previous discussions, I didn't go through all of that. I think if there's something like this in there, it needs to go into the PR text itself, because all this PR seems to need right now is to convince people that the feature is indeed useful. (Based on reactions in Anim Module meeting) - I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to `only_show_errors`. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good. Off-topic / Suggestions for separate patch: - Right-click pop-up driver editor could be like 50% wider. - Spacing of the ID type selector is broken (bunch of empty space between it, and its "Prop:" label. Also, that label is inaccurate, it should say "ID:" or "Datablock:".) - Eyedropper button next to "Add Driver Variable" button doesn't work, and I only now noticed it exists, not sure what it's supposed to do? It only shows up in the right-click pop-up, not in the regular driver editor?
Alexander Gavrilov commented 2023-07-27 20:14:51 +02:00
Author
Member
Copy Link
  • I drive some crowd properties like Shape Keys and Materials, with Scene custom properties.
  • I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties.
  • With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created).

Well basically that's the idea.

Also, without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot above, defaults for drivers allow implementing those behaviors.

  • Do you @angavrilov have something concrete like the above, but real? I know there's a lot of previous discussions, I didn't go through all of that. I think if there's something like this in there, it needs to go into the PR text itself, because all this PR seems to need right now is to convince people that the feature is indeed useful. (Based on reactions in Anim Module meeting)

I don't have a 'production' use case (as in a real scene), but I made an artificial example:

#105407 (comment)

I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context. This is basically a more concrete instance of your usage case idea.

  • I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to only_show_errors. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good.

In the driver UI itself the rna path field is highlighted in red when it can't be resolved. I made sure to preserve that even when default is triggered, so basically a red path with default enabled = default used.

  • Eyedropper button next to "Add Driver Variable" button doesn't work, and I only now noticed it exists, not sure what it's supposed to do? It only shows up in the right-click pop-up, not in the regular driver editor?

I think it's a known bug that nobody fixed. It was there when I was adding Copy As New Driver. IIRC there is a UI context problem with popups, as in some kind of ambiguity between the context of the button that triggered the popup (needed to populate the popup obviously) and the context of the button you clicked in the popup itself. This needs attention of a UI expert who knows that stuff.

> - I drive some crowd properties like Shape Keys and Materials, with Scene custom properties. > - I append the crowd into a new Scene. Now all the drivers are broken because this scene doesn't have the custom properties. > - With this PR, I could specify a variety of default values for all the drivers, so that when the crowd gets loaded to a new file, it still looks like a varied crowd, there's just no longer a way to customize the variation. (until the properties are re-created). Well basically that's the idea. Also, without this feature drivers are less flexible than the View Layer Attribute nodes in materials, because Attributes already allow implementing defaults via the Alpha output (if the property has <4 channels, alpha behaves basically like a 'found' flag), and automatically search for the property in View Layer, Scene and World (to e.g. allow specifying the value per Scene or per View Layer at the scene layout maker's discretion rather than the rigger's). As shown in the screenshot above, defaults for drivers allow implementing those behaviors. > - Do you @angavrilov have something concrete like the above, but real? I know there's a lot of previous discussions, I didn't go through all of that. I think if there's something like this in there, it needs to go into the PR text itself, because all this PR seems to need right now is to convince people that the feature is indeed useful. (Based on reactions in Anim Module meeting) I don't have a 'production' use case (as in a real scene), but I made an artificial example: https://projects.blender.org/blender/blender/issues/105407#issuecomment-980756 I was mainly thinking of an NPR lighting setup that replaces most of the standard lighting calculations (and/or uses the ad-hoc light group trick that uses R/G/B channels to separate lights into three groups instead of representing actual color), and thus needs direct access to custom light positions, colors etc from materials and actual lamp objects (they would still be needed for detecting shadows). Lighting data is obviously scene (or layer) global and should be in the context. This is basically a more concrete instance of your usage case idea. > - I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to `only_show_errors`. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good. In the driver UI itself the rna path field is highlighted in red when it can't be resolved. I made sure to preserve that even when default is triggered, so basically a red path with default enabled = default used. > - Eyedropper button next to "Add Driver Variable" button doesn't work, and I only now noticed it exists, not sure what it's supposed to do? It only shows up in the right-click pop-up, not in the regular driver editor? I think it's a known bug that nobody fixed. It was there when I was adding Copy As New Driver. IIRC there is a UI context problem with popups, as in some kind of ambiguity between the context of the button that triggered the popup (needed to populate the popup obviously) and the context of the button you clicked in the popup itself. This needs attention of a UI expert who knows that stuff.
Alexander Gavrilov force-pushed pr-driver-var-default from db68e7bec9 to 0a1bdb8f0c 2023-07-29 14:54:04 +02:00 Compare
Alexander Gavrilov commented 2023-07-29 15:06:21 +02:00
Author
Member
Copy Link
  • I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to only_show_errors. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good.

I implemented this: now Only Show Errors includes drivers with failed defaults. However this made it difficult to distinguish drivers that completely failed and those that were saved by defaults. So I made another UI behavior change as a separate commit included in this PR.

Specifically, @Sergey in chat said that channel names of failed drivers are underlined with a red line in the dopesheet. Actually, that isn't true. However, I thought that it makes sense to do that, and implemented it, applying it to completely failed drivers.

Thus:

  • A driver that works perfectly has no red underline, and is not included in Only Errors.
  • A driver with broken property references that were protected by defaults is included in Only Errors, but still has no underline (since it evaluated successfully overall, sort of like a caught exception).
  • A driver that failed to evaluate is included in Only Errors, and has a red underline.
> - I tend to agree that successfully falling back to the default should still be treated as an "error", in the sense that it shows up when the Driver Editor is set to `only_show_errors`. I haven't tested the patch, but if it adds a UI label in the driver UI itself (below the expression) that says "ERROR: Property not found. Fell back to default." or something briefer that fits better, I think that's good. I implemented this: now Only Show Errors includes drivers with failed defaults. However this made it difficult to distinguish drivers that completely failed and those that were saved by defaults. So I made another UI behavior change as a separate commit included in this PR. Specifically, @Sergey in chat said that channel names of failed drivers are underlined with a red line in the dopesheet. Actually, that isn't true. However, I thought that it makes sense to do that, and implemented it, applying it to completely failed drivers. Thus: * A driver that works perfectly has no red underline, and is not included in Only Errors. * A driver with broken property references that were protected by defaults is included in Only Errors, but still has no underline (since it evaluated successfully overall, sort of like a caught exception). * A driver that failed to evaluate is included in Only Errors, and has a red underline.
Screenshot_20230729_160541.png
72 KiB
Screenshot_20230729_160541.png
Sergey Sharybin commented 2023-07-31 11:45:11 +02:00
Owner
Copy Link

Specifically, @Sergey in chat said that channel names of failed drivers are underlined with a red line in the dopesheet.

I du not really remember mentioning this. Dopesheet does not display drivers.
In the Drivers editor, however, invalid drivers are high-lighted with a red underline. I see this in almost any production file. So there is definitely a mechanism for this. However, it is possible that a wrong RNA path on a variable does not trigger the driver to be considered failed and underlined with red.

A driver with broken property references that were protected by defaults is included in Only Errors, but still has no underline

To me it is a bis strange on a semantic level of the UI and intent of the feature.

If a "fallback" to use a default value considered an error then it feels that the default value is just there to help hiding the error, which is not really good.
If use of a default value is a valid way of defining your rigs, then it should not appear in the errors list.

> Specifically, @Sergey in chat said that channel names of failed drivers are underlined with a red line in the dopesheet. I du not really remember mentioning this. Dopesheet does not display drivers. In the Drivers editor, however, invalid drivers are high-lighted with a red underline. I see this in almost any production file. So there is definitely a mechanism for this. However, it is possible that a wrong RNA path on a variable does not trigger the driver to be considered failed and underlined with red. > A driver with broken property references that were protected by defaults is included in Only Errors, but still has no underline To me it is a bis strange on a semantic level of the UI and intent of the feature. If a "fallback" to use a default value considered an error then it feels that the default value is just there to help hiding the error, which is not really good. If use of a default value is a valid way of defining your rigs, then it should not appear in the errors list.
Nathan Vegdahl commented 2023-07-31 12:02:40 +02:00
Member
Copy Link

If a "fallback" to use a default value considered an error then it feels that the default value is just there to help hiding the error, which is not really good.
If use of a default value is a valid way of defining your rigs, then it should not appear in the errors list.

I agree. I think it would nevertheless be good to have a way to quickly find drivers that are using default fall backs. So maybe it should be a separate filter?

> If a "fallback" to use a default value considered an error then it feels that the default value is just there to help hiding the error, which is not really good. > If use of a default value is a valid way of defining your rigs, then it should not appear in the errors list. I agree. I think it would nevertheless be good to have a way to quickly find drivers that are using default fall backs. So maybe it should be a separate filter?
Sergey Sharybin commented 2023-07-31 12:10:37 +02:00
Owner
Copy Link

I think having it a separate filter will indeed be a better and clearer fit to the UI.

I think having it a separate filter will indeed be a better and clearer fit to the UI.
Alexander Gavrilov commented 2023-07-31 14:39:50 +02:00
Author
Member
Copy Link

I du not really remember mentioning this. Dopesheet does not display drivers.
In the Drivers editor, however, invalid drivers are high-lighted with a red underline. I see this in almost any production file. So there is definitely a mechanism for this. However, it is possible that a wrong RNA path on a variable does not trigger the driver to be considered failed and underlined with red.

I'm sure you meant the drivers editor, but you did say dopesheet: 😜

Sergey 16:09
the invalid drivers have red underline in the dopesheet

Also, from the code and actual behavior in a small test, invalid drivers (as in the driver variables or expression failed to evaluate) are definitely not highlighted like that. Only drivers on non-existant properties (actually F-Curves in general) get that highlighting currently.

I think having it a separate filter will indeed be a better and clearer fit to the UI.

I think a separate "filter" would be too much - I interpret that as being of equal status to the other three like Only Show Errors, including a button directly in the toolbar etc. However it could be an "option" of the Only Show Errors filter, available in the Filters dropdown.

> I du not really remember mentioning this. Dopesheet does not display drivers. > In the Drivers editor, however, invalid drivers are high-lighted with a red underline. I see this in almost any production file. So there is definitely a mechanism for this. However, it is possible that a wrong RNA path on a variable does not trigger the driver to be considered failed and underlined with red. I'm sure you meant the drivers editor, but you did say dopesheet: 😜 > Sergey 16:09 > the invalid drivers have red underline in the dopesheet Also, from the code and actual behavior in a small test, invalid drivers (as in the driver variables or expression failed to evaluate) are definitely **not** highlighted like that. Only drivers on non-existant properties (actually F-Curves in general) get that highlighting currently. > I think having it a separate filter will indeed be a better and clearer fit to the UI. I think a separate "filter" would be too much - I interpret that as being of equal status to the other three like Only Show Errors, including a button directly in the toolbar etc. However it could be an "option" of the Only Show Errors filter, available in the Filters dropdown.
Nathan Vegdahl commented 2023-07-31 16:12:48 +02:00
Member
Copy Link

Gah! Also, I forgot to report back after the animation & rigging module meeting. The two main things discussed were:

  • We're a bit concerned that this makes the driver editor UI even more visually cluttered and confusing than it already is. One way we might address this would be to put it under a collapsible "advanced" section (or something like that) that's collapsed by default. But that also might not be great. In any case, it would be good to put a reasonable effort into seeing if this can be addressed or not.
  • If that can't be addressed, we think there might be a question of "is it worth the added complexity?" to consider. That's not to suggest that it wouldn't be useful, but there's a complexity/usefulness tradeoff to consider, and this seems maybe a bit niche.
  • @Mets already mentioned this, but having some concrete real use cases listed in the PR description (ideally actual issues that users have run into and/or user requests that this would help address) would be good. And just in general, having a set of real use cases would help ensure that we address design questions in the most helpful way. There was a bit of discussion in #105407 already, such as this comment. And @Mets and you have now helped flesh that out here as well. So maybe just getting a bit more feedback from some other users that might want something like this.

(Oops, forgot some stuff. Edited to add.)

Gah! Also, I forgot to report back after the animation & rigging module meeting. The two main things discussed were: - We're a bit concerned that this makes the driver editor UI *even more* visually cluttered and confusing than it already is. One way we might address this would be to put it under a collapsible "advanced" section (or something like that) that's collapsed by default. But that also might not be great. In any case, it would be good to put a reasonable effort into seeing if this can be addressed or not. - If that can't be addressed, we think there might be a question of "is it worth the added complexity?" to consider. That's not to suggest that it wouldn't be useful, but there's a complexity/usefulness tradeoff to consider, and this seems maybe a bit niche. - @Mets already mentioned this, but having some concrete real use cases listed in the PR description (ideally actual issues that users have run into and/or user requests that this would help address) would be good. And just in general, having a set of real use cases would help ensure that we address design questions in the most helpful way. There was a bit of discussion in #105407 already, such as [this comment](https://projects.blender.org/blender/blender/issues/105407#issuecomment-911628). And @Mets and you have now helped flesh that out here as well. So maybe just getting a bit more feedback from some other users that might want something like this. (Oops, forgot some stuff. Edited to add.)
Alexander Gavrilov force-pushed pr-driver-var-default from 0a1bdb8f0c to e75bbac195 2023-07-31 16:49:20 +02:00 Compare
Alexander Gavrilov changed title from Drivers: implement default fallback values for RNA path based variables. to Drivers: implement fallback values for RNA path based variables. 2023-07-31 16:53:10 +02:00
Alexander Gavrilov commented 2023-07-31 17:00:36 +02:00
Author
Member
Copy Link

I decided to completely rebrand the feature from 'default' to 'fallback', because that's what it actually is - as signified by multiple UI and comment strings using 'default fallback'.

  • We're a bit concerned that this makes the driver editor UI even more visually cluttered and confusing than it already is. One way we might address this would be to put it under a collapsible "advanced" section (or something like that) that's collapsed by default. But that also might not be great. In any case, it would be good to put a reasonable effort into seeing if this can be addressed or not.

I don't think any kind of 'advanced section' is appropriate or useful, because it's already one line of UI. One thing I can do is hide the input field for the actual fallback value unless it's enabled (see attached).

  • @Mets already mentioned this, but having some concrete real use cases listed in the PR description (ideally actual issues that users have run into and/or user requests that this would help address) would be good. And just in general, having a set of real use cases would help ensure that we address design questions in the most helpful way. There was a bit of discussion in #105407 already, such as this comment.

First, that specific comment is completely off topic and irrelevant, because it misunderstands what 'fallback' and 'default' were about in the discussion. Also, I have already provided my example case both here and in that task multiple times.

Edit: I also added a filter option on whether to include drivers with fallbacks in Only Show Errors.

I decided to completely rebrand the feature from 'default' to 'fallback', because that's what it actually is - as signified by multiple UI and comment strings using 'default fallback'. > - We're a bit concerned that this makes the driver editor UI *even more* visually cluttered and confusing than it already is. One way we might address this would be to put it under a collapsible "advanced" section (or something like that) that's collapsed by default. But that also might not be great. In any case, it would be good to put a reasonable effort into seeing if this can be addressed or not. I don't think any kind of 'advanced section' is appropriate or useful, because it's already one line of UI. One thing I can do is hide the input field for the actual fallback value unless it's enabled (see attached). > - @Mets already mentioned this, but having some concrete real use cases listed in the PR description (ideally actual issues that users have run into and/or user requests that this would help address) would be good. And just in general, having a set of real use cases would help ensure that we address design questions in the most helpful way. There was a bit of discussion in #105407 already, such as [this comment](https://projects.blender.org/blender/blender/issues/105407#issuecomment-911628). First, that specific comment is completely off topic and irrelevant, because it misunderstands what 'fallback' and 'default' were about in the discussion. Also, I have __already__ provided my example case both here and in that task multiple times. Edit: I also added a filter option on whether to include drivers with fallbacks in Only Show Errors.
Screenshot_20230731_180710.png
58 KiB
Screenshot_20230731_175238.png
29 KiB
Screenshot_20230731_180710.png Screenshot_20230731_175238.png
👍 1
Alexander Gavrilov commented 2023-07-31 21:38:22 +02:00
Author
Member
Copy Link

@blender-bot package

@blender-bot package
Blender Bot commented 2023-07-31 21:38:25 +02:00
Member
Copy Link

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110135) when ready.
Nathan Vegdahl commented 2023-08-01 10:03:40 +02:00
Member
Copy Link

I'm definitely liking this direction!

I'm definitely liking this direction!
Alexander Gavrilov force-pushed pr-driver-var-default from e75bbac195 to 89923a2a18 2023-09-12 12:17:52 +02:00 Compare
Nathan Vegdahl commented 2023-10-16 11:35:26 +02:00
Member
Copy Link

Some feedback from testing:

  • The fallback option seems to only exist for Single Property and Context Property variable types. I think that's probably fine for now, but maybe that could be added in a follow up PR, for consistency.
  • The fallback option only shows up once the driver variable is fully hooked up. In a sense, this is nice from a visual clutter standpoint. But I think in this case it would be better to have it always visible, because the user might know the feature exists but not know where to find it, or may simply want to set the fallback value first before setting up the rest of the variable fields.
Some feedback from testing: - The fallback option seems to only exist for `Single Property` and `Context Property` variable types. I think that's probably fine for now, but maybe that could be added in a follow up PR, for consistency. - The fallback option only shows up once the driver variable is fully hooked up. In a sense, this is nice from a visual clutter standpoint. But I think in this case it would be better to have it always visible, because the user might know the feature exists but not know where to find it, or may simply want to set the fallback value first before setting up the rest of the variable fields.
Nathan Vegdahl requested changes 2023-10-16 12:05:04 +02:00
Nathan Vegdahl left a comment
Member
Copy Link

Just a partial review so far (haven't made it through all the code yet). So far it's looking good code-wise. Just one minor nit.

Just a partial review so far (haven't made it through all the code yet). So far it's looking good code-wise. Just one minor nit.
source/blender/blenkernel/intern/fcurve_driver.cc Outdated
@ -294,1 +317,4 @@
else {
if (dtar_try_use_fallback(dtar)) {
ptr = PointerRNA_NULL;
*r_prop = NULL;
Nathan Vegdahl commented 2023-10-16 11:51:07 +02:00
Member
Copy Link

Use nullptr instead of NULL.

Use `nullptr` instead of `NULL`.
angavrilov marked this conversation as resolved
Alexander Gavrilov commented 2023-10-16 16:39:50 +02:00
Author
Member
Copy Link
  • The fallback option seems to only exist for Single Property and Context Property variable types. I think that's probably fine for now, but maybe that could be added in a follow up PR, for consistency.

Well, the fallback is provided for the case when resolving the RNA path fails. This is only meaningful for properties with an RNA path, and probably only really useful for Context Property, because for the other type you presumably are in control of what datablock it tries to access. I supported it for both mainly because they share most of the implementation code.

> - The fallback option seems to only exist for `Single Property` and `Context Property` variable types. I think that's probably fine for now, but maybe that could be added in a follow up PR, for consistency. Well, the fallback is provided for the case when resolving the RNA path fails. This is only meaningful for properties with an RNA path, and probably only really useful for Context Property, because for the other type you presumably are in control of what datablock it tries to access. I supported it for both mainly because they share most of the implementation code.
Alexander Gavrilov force-pushed pr-driver-var-default from 89923a2a18 to e6b81f0346 2023-10-16 17:17:32 +02:00 Compare
Nathan Vegdahl commented 2023-10-17 10:40:35 +02:00
Member
Copy Link

Well, the fallback is provided for the case when resolving the RNA path fails.

Is it not triggered if the ID pointer is null? Although even more niche, I could see that also being useful, if someone has a workflow where they append in a rig and then delete the parts they don't need. (Likely not for characters, but for articulated props, etc.) It also just feels like a pretty natural "why not?" kind of thing, where it might as well be available for all variable types for consistency.

> Well, the fallback is provided for the case when resolving the RNA path fails. Is it not triggered if the ID pointer is null? Although even more niche, I could see that also being useful, if someone has a workflow where they append in a rig and then delete the parts they don't need. (Likely not for characters, but for articulated props, etc.) It also just feels like a pretty natural "why not?" kind of thing, where it might as well be available for all variable types for consistency.
Nathan Vegdahl requested changes 2023-10-17 11:52:39 +02:00
Nathan Vegdahl left a comment
Member
Copy Link

Continuing my review. Still have more to go. So far, just nits. Looking good!

Continuing my review. Still have more to go. So far, just nits. Looking good!
source/blender/editors/animation/anim_filter.cc Outdated
@ -1304,3 +1308,3 @@
if ((ads) && (ads->filterflag & ADS_FILTER_ONLY_ERRORS)) {
/* skip if no errors... */
if (fcurve_has_errors(fcu) == false) {
if (!fcurve_has_errors(fcu, ads->filterflag2 & ADS_FILTER_DRIVER_FALLBACK_AS_ERROR)) {
Nathan Vegdahl commented 2023-10-17 11:30:25 +02:00
Member
Copy Link

I think it would read a little better if ads was just passed to fcurve_has_errors() itself, and then the flag check can be done in there.

(Usually I would agree with keeping function parameters minimal in scope. But in this case I think there's a semantic change to fcurve_has_errors(), where it now effectively depends on ads anyway.)

I think it would read a little better if `ads` was just passed to `fcurve_has_errors()` itself, and then the flag check can be done in there. (Usually I would agree with keeping function parameters minimal in scope. But in this case I think there's a semantic change to `fcurve_has_errors()`, where it now effectively depends on `ads` anyway.)
angavrilov marked this conversation as resolved
source/blender/editors/space_graph/graph_buttons.cc Outdated
@ -743,0 +747,4 @@
if (dtar->options & DTAR_OPTION_USE_FALLBACK) {
uiLayout *row = uiLayoutRow(layout, true);
uiItemR(row, dtar_ptr, "use_fallback_value", UI_ITEM_NONE, "", ICON_NONE);
uiItemR(row, dtar_ptr, "fallback_value", UI_ITEM_NONE, nullptr, ICON_NONE);
Nathan Vegdahl commented 2023-10-17 11:43:29 +02:00
Member
Copy Link

Is there a semantic difference between passing "" vs nullptr to uiItemR() here? If not, let's be consistent. It looks like most calls to uiItemR() are passing nullptr when there's no value.

Is there a semantic difference between passing `""` vs `nullptr` to `uiItemR()` here? If not, let's be consistent. It looks like most calls to `uiItemR()` are passing `nullptr` when there's no value.
Alexander Gavrilov commented 2023-10-17 17:41:53 +02:00
Author
Member
Copy Link

nullptr means default caption, while "" means no caption at all, so they are different.

nullptr means default caption, while "" means no caption at all, so they are different.
👍 1
nathanvegdahl marked this conversation as resolved
source/blender/makesdna/DNA_anim_types.h Outdated
@ -332,1 +335,4 @@
/** Driver Target options. */
typedef enum eDriverTarget_Options {
/** use the fallback value */
Nathan Vegdahl commented 2023-10-17 11:51:09 +02:00
Member
Copy Link

The comment here is fairly redundant with the name. Maybe add a tiny bit more detail, like:

/* Use the fallback value when the target is invalid. */
The comment here is fairly redundant with the name. Maybe add a tiny bit more detail, like: ``` /* Use the fallback value when the target is invalid. */ ```
angavrilov marked this conversation as resolved
Nathan Vegdahl requested changes 2023-10-19 12:58:53 +02:00
Nathan Vegdahl left a comment
Member
Copy Link

Just a few more nits, but otherwise the code looks good to me.

Since I'm not super familiar with driver code, I'd like a review from at least one more person. @Sergey would you mind giving this a quick review?

One final change I'd like to see: rather than hiding the fallback checkbox etc. if the ID target isn't filled in, I think graying it out would be better. That makes it a little more discoverable and a little less jarring.

Just a few more nits, but otherwise the code looks good to me. Since I'm not super familiar with driver code, I'd like a review from at least one more person. @Sergey would you mind giving this a quick review? One final change I'd like to see: rather than hiding the fallback checkbox etc. if the ID target isn't filled in, I think graying it out would be better. That makes it a little more discoverable and a little less jarring.
source/blender/makesrna/intern/rna_action.cc Outdated
@ -658,0 +663,4 @@
prop,
"Variable Fallback As Error",
"Include drivers that relied on fallback values to evaluate some of their variables "
"into the Only Show Errors filter, even if the driver evaluation succeeded overall");
Nathan Vegdahl commented 2023-10-19 12:39:06 +02:00
Member
Copy Link

I'm struggling a bit to come up with something better, but maybe this reads a little better:

Include drivers in the Only Show Errors filter that relied on variable fallback values for their evaluation, even if their evaluation otherwise succeeded

I'm struggling a bit to come up with something better, but maybe this reads a little better: > Include drivers in the Only Show Errors filter that relied on variable fallback values for their evaluation, even if their evaluation otherwise succeeded
Nathan Vegdahl commented 2023-10-20 10:03:22 +02:00
Member
Copy Link

You changed it to "The Only Show Errors filter should include drivers that...", which reads like a recommendation, not a description. I think it's better to either stick with what you had, or switch to what I wrote (either is fine). This one is admittedly tricky to phase clearly and succinctly.

You changed it to "The Only Show Errors filter should include drivers that...", which reads like a recommendation, not a description. I think it's better to either stick with what you had, or switch to what I wrote (either is fine). This one is admittedly tricky to phase clearly and succinctly.
Alexander Gavrilov commented 2023-10-20 15:26:59 +02:00
Author
Member
Copy Link

Tried yet another mix of wordings.

Tried yet another mix of wordings.
Nathan Vegdahl commented 2023-10-24 10:20:26 +02:00
Member
Copy Link

Seems about as good as we can do, I think. Only one minor nit: instead of "into", just "in" sounds more natural here.

Seems about as good as we can do, I think. Only one minor nit: instead of "into", just "in" sounds more natural here.
nathanvegdahl marked this conversation as resolved
source/blender/makesrna/intern/rna_fcurve.cc Outdated
@ -1974,0 +1977,4 @@
RNA_def_property_ui_text(prop,
"Use Fallback",
"Use the fallback value if the data path can't be resolved, instead of "
"triggering driver evaluation failure");
Nathan Vegdahl commented 2023-10-19 12:08:36 +02:00
Member
Copy Link

Maybe slightly better wording:

"Use a fallback value if the data path can't be resolved, instead of failing to evaluate"

Maybe slightly better wording: > "Use a fallback value if the data path can't be resolved, instead of failing to evaluate"
nathanvegdahl marked this conversation as resolved
source/blender/makesrna/intern/rna_fcurve.cc Outdated
@ -1974,0 +1990,4 @@
RNA_def_property_boolean_negative_sdna(prop, nullptr, "flag", DTAR_FLAG_FALLBACK_USED);
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
RNA_def_property_ui_text(
prop, "Is Fallback Used", "The last variable evaluation used the fallback value");
Nathan Vegdahl commented 2023-10-19 12:16:31 +02:00
Member
Copy Link

If I understand correctly, this isn't a setting but rather an indicator of what happened. Is that correct? If so, I think this might be a bit clearer as a description:

Indicates that the most recent variable evaluation used the fallback value

If I understand correctly, this isn't a setting but rather an indicator of what happened. Is that correct? If so, I think this might be a bit clearer as a description: > Indicates that the most recent variable evaluation used the fallback value
nathanvegdahl marked this conversation as resolved
source/blender/python/intern/bpy_rna_driver.cc Outdated
@ -62,2 +68,4 @@
}
}
else if (dtar && (dtar->flag & DTAR_FLAG_FALLBACK_USED)) {
driver_arg = PyFloat_FromDouble(dtar->defval);
Nathan Vegdahl commented 2023-10-19 11:44:02 +02:00
Member
Copy Link

Shouldn't the second part of the condition here be the same as on line 44: dtar->options & DTAR_OPTION_USE_FALLBACK? And then also set dtar->flag |= DTAR_FLAG_FALLBACK_USED?

I'm also wondering if there's a reasonable way to put the fallback case as a single guard condition, rather than spreading it out through the other cases here. Although I admit no really satisfying way pops out at me. But if we can, I think that would make the logic a littler easier to follow.

Shouldn't the second part of the condition here be the same as on line 44: `dtar->options & DTAR_OPTION_USE_FALLBACK`? And then also set `dtar->flag |= DTAR_FLAG_FALLBACK_USED`? I'm also wondering if there's a reasonable way to put the fallback case as a single guard condition, rather than spreading it out through the other cases here. Although I admit no really satisfying way pops out at me. But if we can, I think that would make the logic a littler easier to follow.
Alexander Gavrilov commented 2023-10-19 20:28:16 +02:00
Author
Member
Copy Link

It was relying on side effects of driver_get_variable_property. I refactored it to return an enum value, and also do the index bounds check internally so that all fallback checking code is in fcurve_driver.cc. Now bpy_rna_driver.cc only checks the returned enum value.

It was relying on side effects of `driver_get_variable_property`. I refactored it to return an enum value, and also do the index bounds check internally so that all fallback checking code is in fcurve_driver.cc. Now bpy_rna_driver.cc only checks the returned enum value.
Nathan Vegdahl commented 2023-10-20 10:49:40 +02:00
Member
Copy Link

Yeah, this seems better to me. Although it did make me realize even more how sprawling and scattered throughout the source code the logic for drivers is in general.

A nit that I don't feel super strongly about, but I think might improve readability: since you're returning an enum now, it might be better to use a switch rather than an if-else chain. And as a side-benefit, you can then explicitly handle all the enum variants succinctly, and then we can get compiler warnings if an enum variant is ever added but not handled.

Yeah, this seems better to me. Although it did make me realize even more how sprawling and scattered throughout the source code the logic for drivers is in general. A nit that I don't feel super strongly about, but I think might improve readability: since you're returning an enum now, it might be better to use a switch rather than an if-else chain. And as a side-benefit, you can then explicitly handle all the enum variants succinctly, and then we can get compiler warnings if an enum variant is ever added but not handled.
Alexander Gavrilov commented 2023-10-20 15:27:19 +02:00
Author
Member
Copy Link

Converted to a switch.

Converted to a switch.
👍 1
nathanvegdahl marked this conversation as resolved
Alexander Gavrilov commented 2023-10-20 15:28:27 +02:00
Author
Member
Copy Link

One final change I'd like to see: rather than hiding the fallback checkbox etc. if the ID target isn't filled in, I think graying it out would be better. That makes it a little more discoverable and a little less jarring.

The path field isn't shown without a configured datablock either. Thus the new UI follows the current behavior in my view.

> One final change I'd like to see: rather than hiding the fallback checkbox etc. if the ID target isn't filled in, I think graying it out would be better. That makes it a little more discoverable and a little less jarring. The path field isn't shown without a configured datablock either. Thus the new UI follows the current behavior in my view.
Nathan Vegdahl commented 2023-10-24 10:15:53 +02:00
Member
Copy Link

The path field isn't shown without a configured datablock either. Thus the new UI follows the current behavior in my view.

Ah, that's fair. Let's leave it as you have it, then. If/when we add it to the other variable types, we can move it out then.

> The path field isn't shown without a configured datablock either. Thus the new UI follows the current behavior in my view. Ah, that's fair. Let's leave it as you have it, then. If/when we add it to the other variable types, we can move it out then.
Alexander Gavrilov force-pushed pr-driver-var-default from 37c089ca8b to f7f38d4b22 2023-10-24 15:08:54 +02:00 Compare
Alexander Gavrilov commented 2023-10-24 15:09:12 +02:00
Author
Member
Copy Link

s/into/to/ and rebased

s/into/to/ and rebased
Nathan Vegdahl approved these changes 2023-10-31 11:56:04 +01:00
Nathan Vegdahl left a comment
Member
Copy Link

Looks good to me! But as I said earlier, I'd like at least one more person to review this (preferably with more familiarity with this area of Blender's code than me) before actually landing.

Looks good to me! But as I said earlier, I'd like at least one more person to review this (preferably with more familiarity with this area of Blender's code than me) before actually landing.
Sergey Sharybin refused to review 2023-11-02 11:18:51 +01:00
Sybren A. Stüvel requested changes 2023-11-03 15:47:25 +01:00
Sybren A. Stüvel left a comment
Member
Copy Link

This comment has an attached file that demonstrates a hypothetical use case for this feature.

#105407 (comment)

I appreciate the example file, especially since it shows the bigger picture. It's IMO too complex to serve as a simple example that we could use in release notes. It seems to show off features that are not necessarily related to this particular PR. Its complexity also make it hard to use for inspecting the dependency graph, as that'll also be quite complex as a result.

I've attached a more trivial example. It seems to work well, and AFAICS the code is in pretty good shape. I've left some inline notes.

When the fallback value is used, the input field containing the property RNA path that failed to resolve is highlighted in red (identically to the case without a fallback), and the driver can be included in the With Errors filter of the Drivers editor. However, the channel name is not underlined in red, because the driver as a whole evaluates successfully.

Since this is a UI change, please include a screenshot that shows what this would look like.

> This comment has an attached file that demonstrates a hypothetical use case for this feature. > > https://projects.blender.org/blender/blender/issues/105407#issuecomment-980756 I appreciate the example file, especially since it shows the bigger picture. It's IMO too complex to serve as a simple example that we could use in release notes. It seems to show off features that are not necessarily related to this particular PR. Its complexity also make it hard to use for inspecting the dependency graph, as that'll also be quite complex as a result. I've attached a more trivial example. It seems to work well, and AFAICS the code is in pretty good shape. I've left some inline notes. > When the fallback value is used, the input field containing the property RNA path that failed to resolve is highlighted in red (identically to the case without a fallback), and the driver can be included in the With Errors filter of the Drivers editor. However, the channel name is not underlined in red, because the driver as a whole evaluates successfully. Since this is a UI change, please include a screenshot that shows what this would look like.
110135-simple-example.blend
124 KiB
source/blender/blenkernel/BKE_fcurve_driver.h Outdated
@ -144,3 +153,1 @@
struct PointerRNA *r_ptr,
struct PropertyRNA **r_prop,
int *r_index);
eDriverVariablePropertyResult driver_get_variable_property(
Sybren A. Stüvel commented 2023-11-03 15:26:20 +01:00
Member
Copy Link

It's a good idea to replace the bool return type with something more descriptive. This should be done in a separate commit, though, where first the existing functionality is moved to the enum, and then in a 2nd commit the enum can be extended with the DRIVER_VAR_PROPERTY_FALLBACK value for this particular feature.

It's a good idea to replace the `bool` return type with something more descriptive. This should be done in a separate commit, though, where first the existing functionality is moved to the `enum`, and then in a 2nd commit the enum can be extended with the `DRIVER_VAR_PROPERTY_FALLBACK` value for this particular feature.
angavrilov marked this conversation as resolved
source/blender/editors/animation/anim_channels_defines.cc Outdated
@ -4557,6 +4557,23 @@ static bool achannel_is_being_renamed(const bAnimContext *ac,
return false;
}
/** Check if the animation channel name should be underlined in red due to fatal errors. */
Sybren A. Stüvel commented 2023-11-03 15:29:36 +01:00
Member
Copy Link

The word "fatal" can be removed -- if it were fatal, the Blender process would have terminated already.

The word "fatal" can be removed -- if it were fatal, the Blender process would have terminated already.
angavrilov marked this conversation as resolved
source/blender/makesdna/DNA_anim_types.h Outdated
@ -319,3 +319,4 @@
* (NOTE: these get reset every time the types change).
*/
short flag;
/** Single-bit user-visible toggles (not reset on type change). */
Sybren A. Stüvel commented 2023-11-03 15:34:16 +01:00
Member
Copy Link

Document that this is of eDriverTarget_Options type.

Document that this is of `eDriverTarget_Options` type.
angavrilov marked this conversation as resolved
source/blender/makesdna/DNA_anim_types.h Outdated
@ -329,2 +331,3 @@
int _pad1;
/* Fallback value to use with DTAR_OPTION_USE_FALLBACK. */
float defval;
Sybren A. Stüvel commented 2023-11-03 15:35:05 +01:00
Member
Copy Link

Avoid shortened names, float fallback_value is much better, also because it's referred to as "fallback value" in other places as such as well.

Avoid shortened names, `float fallback_value` is much better, also because it's referred to as "fallback value" in other places as such as well.
angavrilov marked this conversation as resolved
source/blender/makesrna/intern/rna_action.cc Outdated
@ -658,0 +661,4 @@
RNA_def_property_ui_text(
prop,
"Variable Fallback As Error",
"Include drivers that relied on variable fallback values for their evaluation "
Sybren A. Stüvel commented 2023-11-03 15:37:32 +01:00
Member
Copy Link

"variable fallback values" is a bit ambiguous; I think "any fallback values" would be better.

"variable fallback values" is a bit ambiguous; I think "any fallback values" would be better.
angavrilov marked this conversation as resolved
source/blender/makesrna/intern/rna_action.cc Outdated
@ -658,0 +662,4 @@
prop,
"Variable Fallback As Error",
"Include drivers that relied on variable fallback values for their evaluation "
"in the Only Show Errors filter, even if the driver evaluation otherwise succeeded");
Sybren A. Stüvel commented 2023-11-03 15:39:08 +01:00
Member
Copy Link

"if the driver evaluation otherwise succeeded" can be replaced by "when the driver evaluation succeeded".

The word "otherwise" might be a bit confusing, and AFAIK in this form the word "when" would be appropriate.

"if the driver evaluation otherwise succeeded" can be replaced by "when the driver evaluation succeeded". The word "otherwise" might be a bit confusing, and AFAIK in this form the word "when" would be appropriate.
angavrilov marked this conversation as resolved
source/blender/makesrna/intern/rna_fcurve.cc Outdated
@ -1974,0 +1977,4 @@
RNA_def_property_ui_text(prop,
"Use Fallback",
"Use the fallback value if the data path can't be resolved, instead of "
"failing to evaluate the whole driver");
Sybren A. Stüvel commented 2023-11-03 15:39:59 +01:00
Member
Copy Link

Since "failing a partial driver" is never possible, I think the word "whole" can be removed.

Since "failing a partial driver" is never possible, I think the word "whole" can be removed.
angavrilov marked this conversation as resolved
source/blender/python/intern/bpy_rna_driver.cc Outdated
@ -41,0 +38,4 @@
anim_eval_context, driver, dvar, dtar, true, &ptr, &prop, &index))
{
case DRIVER_VAR_PROPERTY_SUCCESS:
if (prop) {
Sybren A. Stüvel commented 2023-11-03 15:45:38 +01:00
Member
Copy Link

This code was already nested too deeply, and this change adds yet another indentation on top of that. I feel it should first be cleaned up. By replacing assignments to driver_arg by a return statement, and some reordering of the conditions, I think a lot of indentation can be avoided.

For example, instead of having the entire function body indented like this:

if (driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) {
  // ... all the code sits here
}

it could just be:

if (!driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) {
  return nullptr;
}
// ... all the code sits here
This code was already nested too deeply, and this change adds yet another indentation on top of that. I feel it should first be cleaned up. By replacing assignments to `driver_arg` by a `return` statement, and some reordering of the conditions, I think a lot of indentation can be avoided. For example, instead of having the entire function body indented like this: ```cpp if (driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) { // ... all the code sits here } ``` it could just be: ```cpp if (!driver_get_variable_property(anim_eval_context, driver, dvar, dtar, &ptr, &prop, &index)) { return nullptr; } // ... all the code sits here ```
Alexander Gavrilov commented 2023-11-14 17:18:56 +01:00
Author
Member
Copy Link

You can't do this with a switch on enum. However I restructured the contents of the case branches.

You can't do this with a switch on enum. However I restructured the contents of the case branches.
Alexander Gavrilov force-pushed pr-driver-var-default from b897fdce9a to 208186205e 2023-11-14 17:17:02 +01:00 Compare
Alexander Gavrilov commented 2023-11-14 17:18:13 +01:00
Author
Member
Copy Link
  • Rebased on main to update and introduce the separate refactor commit.
  • Updated comments and messages.
  • Renamed defval to fallback_value - note that this breaks the example file a bit.
- Rebased on main to update and introduce the separate refactor commit. - Updated comments and messages. - Renamed defval to fallback_value - note that this breaks the example file a bit.
Alexander Gavrilov force-pushed pr-driver-var-default from 208186205e to 06f9abb6d5 2023-12-25 16:59:56 +01:00 Compare
Alexander Gavrilov commented 2023-12-25 21:37:41 +01:00
Author
Member
Copy Link
  • Rebased on main.
  • Added tests.
  • Fixed wrongly inverted is_fallback_used (found while making the new tests).
* Rebased on main. * Added tests. * Fixed wrongly inverted `is_fallback_used` (found while making the new tests).
Sybren A. Stüvel approved these changes 2024-01-08 11:20:42 +01:00
Sybren A. Stüvel left a comment
Member
Copy Link

LGTM! Just a small request, can be handled when landing the PR.

LGTM! Just a small request, can be handled when landing the PR.
source/blender/blenkernel/intern/fcurve_driver.cc Outdated
@ -146,6 +146,17 @@ bool driver_get_target_property(const DriverTargetContext *driver_target_context
return true;
}
static bool dtar_try_use_fallback(DriverTarget *dtar)
Sybren A. Stüvel commented 2024-01-08 11:03:30 +01:00
Member
Copy Link

Since this function doesn't seem to actually try and use the fallback, I think the naming is slightly misleading. How about dtar_should_use_fallback instead?

Only after reading the rest of the code, I realise that this function both acts as a query ('should use fallback?') and as an operation ('mark as fallback used').

This function is tightly bound to the places where it is called, as it expects that the caller actually returns the fallback value. Please document this assumption in the function docstring. Then the function name can stay as it is.

Since this function doesn't seem to actually try and use the fallback, I think the naming is slightly misleading. How about `dtar_should_use_fallback` instead? Only after reading the rest of the code, I realise that this function both acts as a query ('should use fallback?') and as an operation ('mark as fallback used'). This function is tightly bound to the places where it is called, as it expects that the caller actually returns the fallback value. Please document this assumption in the function docstring. Then the function name can stay as it is.
source/blender/editors/animation/anim_channels_defines.cc Outdated
@ -4560,2 +4560,4 @@
}
/** Check if the animation channel name should be underlined in red due to errors. */
static bool achannel_is_broken(const bAnimListElem *ale)
Sybren A. Stüvel commented 2024-01-08 11:13:06 +01:00
Member
Copy Link

Refactoring this into its own function makes it so much better to understand, thanks!

Refactoring this into its own function makes it so much better to understand, thanks!
source/blender/python/intern/bpy_rna_driver.cc Outdated
@ -57,3 +36,1 @@
}
}
else {
switch (driver_get_variable_property(
Sybren A. Stüvel commented 2024-01-08 11:15:38 +01:00
Member
Copy Link

This is also much nicer than the nested ifs :)

This is also much nicer than the nested `if`s :)
Alexander Gavrilov force-pushed pr-driver-var-default from 06f9abb6d5 to 4ccd1bec83 2024-01-08 15:24:21 +01:00 Compare
Alexander Gavrilov merged commit d0ef66ddff into main 2024-01-08 15:25:08 +01:00
Alexander Gavrilov deleted branch pr-driver-var-default 2024-01-08 15:25:10 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Nathan Vegdahl
Sybren A. Stüvel
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
EEVEE & Viewport

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
EEVEE & Viewport

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Priority
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

Archived

  
Type
Report

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
6 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110135
No description provided.
Delete Branch "angavrilov/blender:pr-driver-var-default"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?