Error reporting in modifiers #101091

New Issue

Kévin Dietrich · 2022-09-15T16:18:51+02:00

Kévin Dietrich commented

2022-09-15 16:18:51 +02:00

D11591: Alembic: import arbitrary attributes adds some error reporting for attribute reading. It would be nice to formalize this, and extend it to be applicable in other areas, Alembic I/O or otherwise.

One area of improvement would be to stop outputting to the console while telling users to look it: on some platform, or depending on how Blender was started, there may not be a console readily available, and Blender would need to be restarted with a console.

This can fixed by using ReportLists where applicable.

Further, modifiers should have the ability to carry multiple error messages as multiple things can go wrong (e.g. if multiple attributes could not be imported, we should have one error per attribute). This would be the responsability of modifier type. D15492: Modifiers: add a callback to copy the error message introduces some API for modifiers to manage their own errors.

Discussion summary

Rather than D15492, extend ModifierData with an optional ReportList* which would help differentiate between info/warning/error, and have it all displayed in the UI generically.

See P3197 for a quick implementation. Here is an example, based on D11591: Alembic: import arbitrary attributes:

[D11591: Alembic: import arbitrary attributes](https://archive.blender.org/developer/D11591) adds some error reporting for attribute reading. It would be nice to formalize this, and extend it to be applicable in other areas, Alembic I/O or otherwise. One area of improvement would be to stop outputting to the console while telling users to look it: on some platform, or depending on how Blender was started, there may not be a console readily available, and Blender would need to be restarted with a console. This can fixed by using `ReportLists` where applicable. Further, modifiers should have the ability to carry multiple error messages as multiple things can go wrong (e.g. if multiple attributes could not be imported, we should have one error per attribute). This would be the responsability of modifier type. [D15492: Modifiers: add a callback to copy the error message](https://archive.blender.org/developer/D15492) introduces some API for modifiers to manage their own errors. ---------------- **Discussion summary** Rather than [D15492](https://archive.blender.org/developer/D15492), extend `ModifierData` with an optional `ReportList*` which would help differentiate between info/warning/error, and have it all displayed in the UI generically. See [P3197](https://archive.blender.org/developer/P3197.txt) for a quick implementation. Here is an example, based on [D11591: Alembic: import arbitrary attributes](https://archive.blender.org/developer/D11591): ![Capture d’écran_2022-09-15_20-07-07.png](https://archive.blender.org/developer/F13496432/Capture_d_écran_2022-09-15_20-07-07.png)

Kévin Dietrich commented

2022-09-15 16:18:51 +02:00

Added subscriber: @kevindietrich

Brecht Van Lommel commented

2022-09-15 16:29:12 +02:00

Added subscriber: @brecht

Brecht Van Lommel commented

2022-09-15 16:29:12 +02:00

I think modifier could get a optional ReportList* instead of a const char* error, and then the modifier UI can show them with the appropriate icon for info/warning/error (same as the status bar for operators).

Is D15492 mainly intended to display the error message in the attribute remapping panel rather than at the bottom of the modifier? I can see how that would avoid the error being scrolled out of view, but not sure it's worth doing something custom per modifier like that.

I think modifier could get a optional `ReportList*` instead of a `const char* error`, and then the modifier UI can show them with the appropriate icon for info/warning/error (same as the status bar for operators). Is [D15492](https://archive.blender.org/developer/D15492) mainly intended to display the error message in the attribute remapping panel rather than at the bottom of the modifier? I can see how that would avoid the error being scrolled out of view, but not sure it's worth doing something custom per modifier like that.

Hans Goudey commented

2022-09-15 16:30:30 +02:00

Added subscriber: @HooglyBoogly

Hans Goudey commented

2022-09-15 16:30:30 +02:00

If we were further along in the project of converting modifiers to nodes, I would suggest that we skip these complications, since the node warning system already supports these features.
Because Alembic import isn't a node, we can't rely on that though, so this seems reasonable. I just wanted to mention that because in the future this system should become unnecessary.

If we were further along in the project of converting modifiers to nodes, I would suggest that we skip these complications, since the node warning system already supports these features. Because Alembic import isn't a node, we can't rely on that though, so this seems reasonable. I just wanted to mention that because in the future this system should become unnecessary.

Kévin Dietrich commented

2022-09-15 16:40:33 +02:00

Added subscriber: @dr.sybren

Kévin Dietrich commented

2022-09-15 16:40:33 +02:00

In #101091#1417726, @brecht wrote:
I think modifier could get a optional ReportList* instead of a const char* error, and then the modifier UI can show them with the appropriate icon for info/warning/error (same as the status bar for operators).

Sounds like a good idea.

Is D15492 mainly intended to display the error message in the attribute remapping panel rather than at the bottom of the modifier? I can see how that would avoid the error being scrolled out of view, but not sure it's worth doing something custom per modifier like that.

No, it was because I added some logic specific to the MeshSequenceCache error inside of BKE_object_sync_to_original in D11591 to copy the error message, so @dr.sybren asked to add a callback to modifiers for that, to keep specific things out of generic places, and split the change in a separate patch.

The error message is actually displayed at the top of the modifier for some reason. I put it there because elsewhere was not working, but I don't remember why.

> In #101091#1417726, @brecht wrote: > I think modifier could get a optional ReportList* instead of a const char* error, and then the modifier UI can show them with the appropriate icon for info/warning/error (same as the status bar for operators). Sounds like a good idea. > Is [D15492](https://archive.blender.org/developer/D15492) mainly intended to display the error message in the attribute remapping panel rather than at the bottom of the modifier? I can see how that would avoid the error being scrolled out of view, but not sure it's worth doing something custom per modifier like that. No, it was because I added some logic specific to the `MeshSequenceCache` error inside of `BKE_object_sync_to_original` in [D11591](https://archive.blender.org/developer/D11591) to copy the error message, so @dr.sybren asked to add a callback to modifiers for that, to keep specific things out of generic places, and split the change in a separate patch. The error message is actually displayed at the top of the modifier for some reason. I put it there because elsewhere was not working, but I don't remember why.

Brecht Van Lommel commented

2022-09-15 16:48:07 +02:00

In #101091#1417729, @kevindietrich wrote:
No, it was because I added some logic specific to the MeshSequenceCache error inside of BKE_object_sync_to_original in D11591 to copy the error message, so @dr.sybren asked to add a callback to modifiers for that, to keep specific things out of generic places, and split the change in a separate patch.

Why was the error not just put in ModifierData.error then?

> In #101091#1417729, @kevindietrich wrote: > No, it was because I added some logic specific to the `MeshSequenceCache` error inside of `BKE_object_sync_to_original` in [D11591](https://archive.blender.org/developer/D11591) to copy the error message, so @dr.sybren asked to add a callback to modifiers for that, to keep specific things out of generic places, and split the change in a separate patch. Why was the error not just put in `ModifierData.error` then?

Kévin Dietrich commented

2022-09-15 16:51:41 +02:00

In #101091#1417731, @brecht wrote:
Why was the error not just put in ModifierData.error then?

Unit tests were failing (it is mentioned in the patch description). Those errors are recoverable with the remapping system, so they are not really errors that should make unit tests fail (I am talking about added unit tests, not existing ones).

> In #101091#1417731, @brecht wrote: > Why was the error not just put in `ModifierData.error` then? Unit tests were failing (it is mentioned in the patch description). Those errors are recoverable with the remapping system, so they are not really errors that should make unit tests fail (I am talking about added unit tests, not existing ones).

Brecht Van Lommel commented

2022-09-15 17:40:19 +02:00

Ah ok, then using ReportList* to distinguish between info/warning/error may be a good fit.

Regarding duplication with geometry nodes, I'm hoping that eventually we can get logs from modifiers, nodes, depsgraph, render all into the info editor. And so for that reason I think moving in the direction of using ReportList more makes sense, and especially here where code is shared with operators that already use it.

Ah ok, then using `ReportList*` to distinguish between info/warning/error may be a good fit. Regarding duplication with geometry nodes, I'm hoping that eventually we can get logs from modifiers, nodes, depsgraph, render all into the info editor. And so for that reason I think moving in the direction of using `ReportList` more makes sense, and especially here where code is shared with operators that already use it.

Kévin Dietrich commented

2022-09-15 20:12:20 +02:00

So we are on the same page, here is what I quickly did (based on my alembic branch): P3197

And the result (2 attributes fail reading out of 13):

So we are on the same page, here is what I quickly did (based on my alembic branch): [P3197](https://archive.blender.org/developer/P3197.txt) And the result (2 attributes fail reading out of 13): ![Capture d’écran_2022-09-15_20-07-07.png](https://archive.blender.org/developer/F13496432/Capture_d_écran_2022-09-15_20-07-07.png)

Rowan Ibbeken commented

2022-09-19 15:52:32 +02:00

Added subscriber: @Rowan-Ibbeken

Sybren A. Stüvel commented

2022-10-06 17:21:40 +02:00

How would this deal when there are many errors? Is there some way that flooding the UI is prevented? Or can messages be grouped somehow? Was the UI module involved in this change?

Kévin Dietrich commented

2022-10-28 14:42:45 +02:00

In #101091#1428616, @dr.sybren wrote:
How would this deal when there are many errors? Is there some way that flooding the UI is prevented? Or can messages be grouped somehow? Was the UI module involved in this change?

The UI module was not involved with this change, although a couple core UI member (according to the #user_interface project page) have commented here.

For many errors, I guess we could use a dedicated panel which will only be displayed when there are some errors, and whose header will indicate that are errors. Although, we would need a dedicated error panel for each modifier type, if I understand how subpanels are setup via their parent id. I don't about other grouping mechanism which could be used.

> In #101091#1428616, @dr.sybren wrote: > How would this deal when there are many errors? Is there some way that flooding the UI is prevented? Or can messages be grouped somehow? Was the UI module involved in this change? The UI module was not involved with this change, although a couple core UI member (according to the #user_interface project page) have commented here. For many errors, I guess we could use a dedicated panel which will only be displayed when there are some errors, and whose header will indicate that are errors. Although, we would need a dedicated error panel for each modifier type, if I understand how subpanels are setup via their parent id. I don't about other grouping mechanism which could be used.

Sybren A. Stüvel commented

2022-11-11 14:25:57 +01:00

In #101091#1417748, @brecht wrote:
Ah ok, then using ReportList* to distinguish between info/warning/error may be a good fit.

I like that option too.

Regarding duplication with geometry nodes, I'm hoping that eventually we can get logs from modifiers, nodes, depsgraph, render all into the info editor.

Messages like these IMO don't belong in the info editor, at least not if it would behave like printing to stdout. Since they're produced on every evaluation of the modifier, that could get spammy really quickly.

In #101091#1438328, @kevindietrich wrote:
For many errors, I guess we could use a dedicated panel which will only be displayed when there are some errors, and whose header will indicate that are errors. Although, we would need a dedicated error panel for each modifier type, if I understand how subpanels are setup via their parent id. I don't about other grouping mechanism which could be used.

That sounds a bit tricky, also in terms of UI/UX. Maybe just showing the first N errors and then collapse the rest under a "show more" button would be enough.

> In #101091#1417748, @brecht wrote: > Ah ok, then using `ReportList*` to distinguish between info/warning/error may be a good fit. I like that option too. > Regarding duplication with geometry nodes, I'm hoping that eventually we can get logs from modifiers, nodes, depsgraph, render all into the info editor. Messages like these IMO don't belong in the info editor, at least not if it would behave like printing to `stdout`. Since they're produced on every evaluation of the modifier, that could get spammy really quickly. > In #101091#1438328, @kevindietrich wrote: > For many errors, I guess we could use a dedicated panel which will only be displayed when there are some errors, and whose header will indicate that are errors. Although, we would need a dedicated error panel for each modifier type, if I understand how subpanels are setup via their parent id. I don't about other grouping mechanism which could be used. That sounds a bit tricky, also in terms of UI/UX. Maybe just showing the first N errors and then collapse the rest under a "show more" button would be enough.

Brecht Van Lommel commented

2022-11-11 15:05:56 +01:00

In #101091#1444452, @dr.sybren wrote:
Messages like these IMO don't belong in the info editor, at least not if it would behave like printing to stdout. Since they're produced on every evaluation of the modifier, that could get spammy really quickly.

The main use case is users being able to see all warnings/errors in the current scene and fix them. For that indeed it should not work like stdout, but rather retain and replace the errors somehow per object, per depsgraph for cyclic dependency errors, etc.

I think this would work well as a new mode in the info editor, distinct from the current operator/property logging that works like stdout. And then you can imagine additional modes like render, viewport, memory usage, etc. I don't think we need a distinct editor for each.

> In #101091#1444452, @dr.sybren wrote: > Messages like these IMO don't belong in the info editor, at least not if it would behave like printing to `stdout`. Since they're produced on every evaluation of the modifier, that could get spammy really quickly. The main use case is users being able to see all warnings/errors in the current scene and fix them. For that indeed it should not work like stdout, but rather retain and replace the errors somehow per object, per depsgraph for cyclic dependency errors, etc. I think this would work well as a new mode in the info editor, distinct from the current operator/property logging that works like stdout. And then you can imagine additional modes like render, viewport, memory usage, etc. I don't think we need a distinct editor for each.

Sybren A. Stüvel commented

2022-11-11 16:06:33 +01:00

👍 I can certainly see that working!

:+1: I can certainly see that working!

Kévin Dietrich commented

2022-11-16 06:04:29 +01:00

Also worth noting, there used to be a GSOC project that could be revived to extend the info editor: #78164 (GSoC 2020 Info Editor Improvements), with similar functionalities as the ones discussed here.

Brecht Van Lommel commented

2022-12-14 20:27:44 +01:00

D15492 mentions this task has no clear conclusion. What I guess is remaining is how to deal with the UI in case of many reports?

I think a panel is the proper solution, I don't think we need a custom "show more" mechanism.

It seems reasonable to register an error subpanel for every modifier type, or all modifiers types that potentially report many errors.

We could support panel types having multiple parents, it's mainly the Python API that is not compatible with this. Panel types have a list of child panel types, and the subpanel type itself only needs to know if it has a parent or not. Not sure it's worth getting into this compared to just registering a panel for the handful of modifiers that need it though.

[D15492](https://archive.blender.org/developer/D15492) mentions this task has no clear conclusion. What I guess is remaining is how to deal with the UI in case of many reports? I think a panel is the proper solution, I don't think we need a custom "show more" mechanism. It seems reasonable to register an error subpanel for every modifier type, or all modifiers types that potentially report many errors. We could support panel types having multiple parents, it's mainly the Python API that is not compatible with this. Panel types have a list of child panel types, and the subpanel type itself only needs to know if it has a parent or not. Not sure it's worth getting into this compared to just registering a panel for the handful of modifiers that need it though.

Hans Goudey commented

2022-12-14 20:31:21 +01:00

The problem with adding a subpanel for errors is that it isn't possible to display a sub-panel conditionally based on some modifier data, due to the way modifier panels are created.
If we're okay with the subpanel always displaying, that's okay, but that doesn't seem great to me.

Generally I'm not sure that it's worth putting that much effort into this right now, since modifiers will generally be implemented as nodes in the future, and in cases with many error messages, it would probably make more sense to go there instead anyway.

The problem with adding a subpanel for errors is that it isn't possible to display a sub-panel conditionally based on some modifier data, due to the way modifier panels are created. If we're okay with the subpanel always displaying, that's okay, but that doesn't seem great to me. Generally I'm not sure that it's worth putting that much effort into this right now, since modifiers will generally be implemented as nodes in the future, and in cases with many error messages, it would probably make more sense to go there instead anyway.

Brecht Van Lommel commented

2022-12-14 20:40:42 +01:00

Don't panels like that have a poll() function?

My main concern is to get the USD/Alembic improvements landed in time for Blender 3.5, since this is something users requests a lot. Error reporting in the existing modifier is not any better so I would not even consider improving that a blocker personally.

Don't panels like that have a `poll()` function? My main concern is to get the USD/Alembic improvements landed in time for Blender 3.5, since this is something users requests a lot. Error reporting in the existing modifier is not any better so I would not even consider improving that a blocker personally.

Hans Goudey commented

2022-12-14 20:43:32 +01:00

I definitely don't consider this blocking the Alembic/USD import patches. They're totally unrelated implementation-wise.
We shouldn't try to bundle these things together arbitrarily, holding important features like that hostage.

Don't panels like that have a poll() function?

They do, but it takes the PanelType instead of the Panel unfortunately, so knowing which modifier it applies to will be very hacky at best.

I definitely don't consider this blocking the Alembic/USD import patches. They're totally unrelated implementation-wise. We shouldn't try to bundle these things together arbitrarily, holding important features like that hostage. --- >Don't panels like that have a poll() function? They do, but it takes the `PanelType` instead of the `Panel` unfortunately, so knowing which modifier it applies to will be very hacky at best.

Brecht Van Lommel commented

2022-12-14 20:54:00 +01:00

I assumed there would be a "modifier" entry in the context that it would look at, not the panel.

I assumed there would be a `"modifier"` entry in the context that it would look at, not the panel.

Hans Goudey commented

2022-12-14 20:59:06 +01:00

There isn't currently. Maybe that would work, though I'm not exactly sure where that would be set, based on where PanelType::poll is called.
Anyway, I think it's fair to say it's not trivial, whatever that means for planning an implementation.

There isn't currently. Maybe that would work, though I'm not exactly sure where that would be set, based on where `PanelType::poll` is called. Anyway, I think it's fair to say it's not trivial, whatever that means for planning an implementation.

Kévin Dietrich commented

2022-12-15 06:15:23 +01:00

In #101091#1460589, @brecht wrote:
My main concern is to get the USD/Alembic improvements landed in time for Blender 3.5, since this is something users requests a lot. Error reporting in the existing modifier is not any better so I would not even consider improving that a blocker personally.

Only the Alembic attributes import patch depends on this. The Geometry Set patch depends in turn on the attributes one, but it's just about loading attributes for Curves and PointCloud. This little dependency can removed and either added to the other patch, or committed separately.

Then for the attributes patch, I could remove the dependency on this error reporting by revising the unit tests. Besides UI improvement, as noted in the past, there was the need to differentiate between various types of errors so we don't abort the unit tests while testing "recoverable errors" via the attribute remapping system.

In #101091#1460586, @HooglyBoogly wrote:
The problem with adding a subpanel for errors is that it isn't possible to display a sub-panel conditionally based on some modifier data, due to the way modifier panels are created.
If we're okay with the subpanel always displaying, that's okay, but that doesn't seem great to me.

We already show empty (sub)panels at least in the MeshSequenceCache modifier when there is no CacheFile data block set (for the same problem of not being able to show them conditionally). So this is not unprecedented.

In #101091#1460586, @HooglyBoogly wrote:
Generally I'm not sure that it's worth putting that much effort into this right now, since modifiers will generally be implemented as nodes in the future, and in cases with many error messages, it would probably make more sense to go there instead anyway.

I don't know about the timeline, but unless this happens in the next few months and all modifiers are simply gone, I think it does make sense to spend a bit of time improving the UI/UX here.

> In #101091#1460589, @brecht wrote: > My main concern is to get the USD/Alembic improvements landed in time for Blender 3.5, since this is something users requests a lot. Error reporting in the existing modifier is not any better so I would not even consider improving that a blocker personally. Only the Alembic attributes import patch depends on this. The Geometry Set patch depends in turn on the attributes one, but it's just about loading attributes for Curves and PointCloud. This little dependency can removed and either added to the other patch, or committed separately. Then for the attributes patch, I could remove the dependency on this error reporting by revising the unit tests. Besides UI improvement, as noted in the past, there was the need to differentiate between various types of errors so we don't abort the unit tests while testing "recoverable errors" via the attribute remapping system. > In #101091#1460586, @HooglyBoogly wrote: > The problem with adding a subpanel for errors is that it isn't possible to display a sub-panel conditionally based on some modifier data, due to the way modifier panels are created. > If we're okay with the subpanel always displaying, that's okay, but that doesn't seem great to me. We already show empty (sub)panels at least in the `MeshSequenceCache` modifier when there is no `CacheFile` data block set (for the same problem of not being able to show them conditionally). So this is not unprecedented. > In #101091#1460586, @HooglyBoogly wrote: > Generally I'm not sure that it's worth putting that much effort into this right now, since modifiers will generally be implemented as nodes in the future, and in cases with many error messages, it would probably make more sense to go there instead anyway. I don't know about the timeline, but unless this happens in the next few months and all modifiers are simply gone, I think it does make sense to spend a bit of time improving the UI/UX here.

Iliya Katushenock removed the

Status

Needs Triage

label 2023-08-24 17:33:12 +02:00

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

Error reporting in modifiers #101091