-
-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework the Configuration Warning system #9416
Comments
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 |
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. |
Wouldn't it be better if it contained only Dictionary, and in the future structs?
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 |
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. |
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
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
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 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 |
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.
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). |
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 EDIT: implemented in godotengine/godot#90049 |
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
andupdate_configuration_info
toNode
andResource
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 ofString 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: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:
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.
The text was updated successfully, but these errors were encountered: