Rework the Configuration Warning system

[RedMser]

Describe the project you are working on

Godot Engine

Describe the problem or limitation you are having in your project

The current configuration warning system is not flexible, and practically impossible to extend. It should be replaced with something new that allows for more extensibility in the future.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The following is a list of immediate goals for the new system:

Different Severities

The new config warning system should be usable for "one-off hints", without giving the user a sense of urgency to fix all warnings. Introducing new severity levels besides warnings, such as "info" would help here. Similarly, "errors" could be shown here as well, to avoid using the Output tab for informing about known error states.

Only the highest severity would show as icon (e.g. if one node has infos, warnings and errors, then use the "error" icon). Otherwise this field has no effect.

Allow attaching to properties

See #550. Warnings should optionally be attachable to specific properties, so that it's visible in the inspector where the issue is coming from.

Make it usable for Resources

See #9098. Nodes and Resources can both be opened in inspector, so both should be able to benefit from configuration warnings.

While there exists no great location to show a "list of all warnings for a specific resource file", we can still show warnings attached to specific properties. All warnings of a sub-resource could also be shown on the property that holds the sub-resource.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Add new methods _get_configuration_info and update_configuration_info to Node and Resource

While they could be added to Object directly, they may differ slightly in implementation (e.g. thread guards for node) and may use different signals if beneficial.

_get_configuration_info would return an Array of String or Dictionary. String elements are converted to equivalent Dictionaries, similar to the current config warning system. Dictionaries allow for more extension with additional, named fields, and they are easy to create. Following fields would be accepted for now, but more may be later added:

Name	Type	Comment
message	string	Message to show.
severity	'error' | 'warning' | 'info'	Decides icon to use. If missing, defaults to 'warning'.
property	property path	If specified, attach info to a specific property.

Deprecate Node::_get_configuration_warnings

To avoid breaking binary compat, we deprecate the old config warning method instead of modifying its signature. Internally, we will merge the results of both methods' arrays, so that none of the warnings are lost.

Switch to the new system

As I understand it, it's not an issue for compatibility if all engine classes switch to the new method. This would solve the issue of the engine using deprecated methods.

Show configuration info in the inspector

This is required since configuration info that is attached to a Resource without a property would have no place to show up. I've implemented the following UI which shows whenever a configuration info exists on the inspected Node or Resource:

Future extensions

While I don't intend to implement these in the first iteration, they should be possible with this new system:

• Add an error code field, so that warnings can be hidden (stored in Project Settings): https://github.com/godotengine/godot/pull/89956
• Add an auto-fix Callable, so that warnings can be automatically fixed: https://github.com/godotengine/godot-proposals/issues/2519

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, this is a core system change.

Is there a reason why this should be core and not an add-on in the asset library?

This is a core system change.

[passivestar]

The biggest problem with configuration warnings imo is the fact that you need to mark the script as a tool, which means each lifecycle method needs a Engine.is_editor_hint() check

[RedMser]

I totally agree. The ideas proposed here shouldn't make such a change any more difficult to be realized, would someone want to implement it. But I don't really have the technical know-how to make it happen as part of my PR.

[KoBeWi]

_get_configuration_info would return an Array of String or Dictionary

Wouldn't it be better if it contained only Dictionary, and in the future structs?

The biggest problem with configuration warnings imo is the fact that you need to mark the script as a tool, which means each lifecycle method needs a Engine.is_editor_hint() check

Calling a method requires a script instance (it's an internal object that holds the data/state of the object with script) and to have a script instance in editor, the script needs to be a @tool. Currently the engine does not support partial script instances that would contain only some methods. It's all or nothing. (exported properties are kind of exception, because they are like "metadata" that doesn't require instance)

[RedMser]

Array of String or Dictionary

Wouldn't it be better if it contained only Dictionary, and in the future structs?

Structs would definitely be a great use case for this API, assuming they also get some form of inline constructor syntax similar to Dictionaries. Hoping for some automatic conversion possibilities for struct to dict, but we'll see ^^

IMO strings are less effort to write for the user. It's already a bit of a pain to create an array and append error messages to it, so I'd like to avoid making the whole ordeal even more annoying for simple cases. But as this is a fairly opinionated decision (e.g. how it would default to the "warning" severity level), I don't insist on keeping it this way either. Would like to hear more opinions, or reasons that speak against this.

[passivestar]

I think the warning system needs to be discussed in context because it may affect the requirements for it

The main problem that the configuration warning system is trying to solve is making it easy to create reusable components. That's how it's used in engine. For example you can think of the CollisionShape3D node as a component, because:

1. It can only be used with a certain parent (CollisionObject3D)
2. One CollisionObject3D can have multiple collision shapes

Component pattern alleviates Godot's one script per node limitation. One notable problem is that even though Godot is using composition itself, it does very little to help users to use composition in their own code. For example when trying to use .gdscript files for components I quickly realised that it's better to at least wrap them in scenes because:

1. It prevents the user from using a script directly on a node, making it possible to ONLY use it as a child node
2. It makes it easier to add components by dragging them into the scene because you don't need to hold Ctrl
3. It allows you to add a description to your component through the editor description field. It's shown when you hover over a node in the scene
4. It allows your components to have internal nodes, which can be useful for some components

Calling a method requires a script instance (it's an internal object that holds the data/state of the object with script) and to have a script instance in editor, the script needs to be a @ tool. Currently the engine does not support partial script instances that would contain only some methods. It's all or nothing. (exported properties are kind of exception, because they are like "metadata" that doesn't require instance)

This limitation can probably be worked around by creating a child node that will handle configuration warnings on behalf of its parent when you're using scenes as components. That child will be a tool so that the parent can remain a regular script. The only problem is that in order for that to work scenes need to be able to show configuration warnings from their children

I think composition in godot needs to be better designed and endorsed at the engine level, and the configuration warning system needs to help address those issues when the problem space is better defined. It's possible that we'll discover that in a lot of cases instead of showing a warning in editor it shouldn't be even possible to add a node into the hierarchy in the first place if the requirements aren't met (this is roughly how RequireComponent attribute works in Unity for example). All of this will need a separate proposal of course because there's a lot to discuss here

Just to be clear I do believe that this proposal and the associated PR are helpful too, the warning severity categories will definitely be helpful, I just think we need to address the elephant in the room too, which is how configuration warnings fit into the bigger picture and how they are used in practice

[RedMser]

scenes need to be able to show configuration warnings from their children

I recall seeing a proposal that shows greyed out warning icons if the node with warnings is collapsed. My changes don't affect this feature, so it's so out of scope.

it shouldn't be even possible to add a node into the hierarchy in the first place if the requirements aren't met

I disagree, Godot is too flexible to prevent the user from doing certain things. For the CollisionShape example, I'd rather see "here's a quick fix option to add a child CollisionShape node" than "you can't add any node except CollisionShape here". Reparenting and recursively searching hierarchies is too prevalent.

My current goal is to port all existing warnings over to the new system, then change severity and property designation of some.

Maybe you should voice your concerns and ideas in a new proposal/discussion thread, as I don't immediately see any relations to what I'm proposing and working on (except of course both being related to the configuration warning system).

[RedMser]

As part of adding Configuration Infos to Resources, I need a UI to show a list of them in the inspector. #9098 suggests this for a UI mockup:

I am wondering if the same UI section should also be added for the selected Node in the inspector as well. Any opinions for/against this change?

As for implementation, I will try to implement this as an EditorInspectorPlugin (which can_handle resources and possibly node as well).

EDIT: implemented in https://github.com/godotengine/godot/pull/90049