Rework the Configuration Warning system

[RedMser]

Closes https://github.com/godotengine/godot-proposals/issues/9416 Closes https://github.com/godotengine/godot-proposals/issues/550 again Closes https://github.com/godotengine/godot-proposals/issues/9098 For my first attempt at implementing this (which has since been reverted), see #68420 and #88182

What this PR does

• User facing changes
  • Configuration warnings are now a more general system called configuration info.
  • Configuration info is available not only on Node, but also on Resource.
  • Configuration info supports different severity levels, which show up as different icons: info, warning (default), error.
  • Configuration info supports being attached to a specific property, which makes the message show in the Inspector.
  • At the top of the inspector, you can now see a list of configuration info for the selected Node or Resource (see screenshots below).
• API changes
  • A new API _get_configuration_info is available which accepts a mixed Array of Dictionaries and Strings ("my warning" is equivalent to { "message": "my warning" }).
  • C++ code that did warnings.push_back(RTR("...")) will now instead use CONFIG_WARNING(RTR("...")) macro (can be suffixed with _P to refer to a specific property).
  • No compatibility breaking changes. New APIs for everything, old stuff is left untouched but marked deprecated. Any code still using _get_configuration_warnings will behave the same as before.
  • Pulled as much logic out of Node as possible, instead moving it to its own EditorConfigurationInfo class, so logic can be shared with Resources, and between different Editor UIs. Without tools enabled, get_configuration_info is not available, reducing build size by around 100KB.

Screenshots

For reviewers

First commit modifies the core and editor only, while the second commit adjusts all Nodes to use the new API.

Miscellaneous Notes

These do not have to be resolved to merge this PR, unless deemed necessary by reviewers.

• Is the name "Configuration Info" clear enough, from a user perspective? I can not think of a better name, now that it's not only used for warnings but also errors and information text... Suggestions welcome!
• Improve the UI when clicking on the icon next to a property or in the scene tree. Should use the same RichTextLabel logic as the inspector plugin does, to show severity icons and different text color based on severity.
• Would be nice to have a tooltip when hovering the icon, but since other icons in the Inspector don't have this yet, it can be done in a follow-up PR.

[Calinou]

• [ ] Test whether exported games don't break, since some of the config info system is now part of the editor and not in Node anymore.

Note that there was some discussion about whether node configuration warnings should be able to show when running a project in an editor build, but outside the editor (i.e. with a print in the Output panel). I can't find the proposal in question, but I remember discussing it myself.

Edit: This was probably in https://github.com/godotengine/godot-proposals/issues/3004#issuecomment-1722085919.

It also makes me wonder whether all _get_configuration_warnings() methods (or at least the method bodies) should be placed behind #ifdef TOOLS_ENABLED to reduce binary size.

[RedMser]

It also makes me wonder whether all _get_configuration_warnings() methods (or at least the method bodies) should be placed behind #ifdef TOOLS_ENABLED to reduce binary size.

I tried this change (ifdef the entirety of get_configuration_info) and it seems to reduce the binary size by 100KB. Doing so for the body only would be a lot more manual labor, not sure if it's worth the hassle.

[RedMser]

This PR should be ready now. I've rewritten the description, to remove finished TODOs and clarify what this PR exactly does.

A larger UI change has been made by introducing a configuration info summary in the inspector (see screenshots in PR description).

I'd like feedback whether this list should show for Resource only, or also for Node like in the current implementation.

[Mickeon]

Quoting my own comment on the matter last time just for the sake for others to remember it, haven't looked at the implementation yet.

[Calinou]

A larger UI change has been made by introducing a configuration info summary in the inspector:

Looks good! I'd make the text besides the warning icon yellow (warning_color) and use red text besides the error icon (error_color). This helps the warnings/errors stand out more.

[RedMser]

Great idea, here's how it looks after said changes:

[KoBeWi]

• [x] IMO the inspector warnings should be folded by default.
• [x] Also why are they clickable?

https://github.com/user-attachments/assets/a35283b5-f125-449a-83b7-d7c97ccd8ade

• [ ] Using unsupported severity results in broken icon: Is this intended?
• [x] There should be some error when info refers to non-existent property.

[RedMser]

IMO the inspector warnings should be folded by default.

Sure I can change that.

Also why are they clickable?

Yeah I have a TODO to replace them with RichTextLabel, so they wrap. Had ideas like copy text to clipboard etc, but didn't implement. Itemlist isn't good for this purpose. Will fix soon.

Using unsupported severity results in broken icon:

Maybe it should instead fallback to "warning" severity and print an error?

There should be some error when info refers to non-existent property.

Yeah I can try to add this as well.

[KoBeWi]

Went through the changes and overall looks fine. Though I'm not sure if there is need for supporting Strings in info arrays. It's not needed for compatibility, only for convenience.

[RedMser]

Thanks for the review KoBeWi, I did a first pass which includes all comments marked as resolved. It's a new commit so it's easier to go over just these changes. Going to do the rest in the next few days, since it's larger changes.

EDIT: also fixing the CI failure later :)

I'm not sure if there is need for supporting Strings in info arrays. It's not needed for compatibility, only for convenience.

It helps ease the transition from the old to the new system (user just has to rename the method), but indeed it wouldn't strictly be needed and currently comes with some extra complexity on the editor. I don't mind keeping it this way, or restricting the API to Array[Dictionary].

[RedMser]

OK new changes, as new commits to help make review easier.

• Replaced ItemList with RichTextLabel for the UI.
• Introduced ConfigurationInfo struct which is used everywhere internally, while the scripting API still uses Dictionary/String as before. This made the code a lot nicer ^^ I don't know if LocalVector is correct here, but since it's internal API (and not inheriting Object), using Array/TypedArray made no sense to me here.
• I changed get_configuration_info (the built-in one, not the one exposed to scripting) to use ConfigurationInfo struct, and also went and renamed warnings -> infos inside.
• Some validation was added, like printing error if message is empty, severity is unknown, property does not exist. How to make them not spam? Using _ONCE prints would work, but then the "property missing" error would only show once, even if you rename to another still-not-existing property...

IMO the inspector warnings should be folded by default.

Each time you edit a property, inspector plugins are recreated, so it collapses automatically all the time. But the opposite (auto-expanding on each edit) is also not great. Where should I store the collapse state so that it can be remembered? I should maybe push the same fix to the ControlPositioningWarning where I took most of the code from 👀

[KoBeWi]

I don't know if LocalVector is correct here

It's not. LocalVector is for storing data, for passing data around use Vector.

Where should I store the collapse state so that it can be remembered?

Check EditorSettings' project metadata.

[RedMser]

I've rebased to latest master and verified that everything still works. No problems when using the template_release either, update_configuration_(info|warnings) calls are a no-op as is intended. Not sure if there's anything left to do from my side now.

Once review is done, I will squash the commits to just have a "add the feature" commit and another "update all nodes to use the new APIs" commit. Is this okay or do you prefer one commit with everything squashed together?

[KoBeWi]

Maybe it should instead fallback to "warning" severity and print an error?

It could fallback to warning, but not sure about the error. It makes sense, but it's being spammed a lot.

https://github.com/user-attachments/assets/448a822e-9eea-4089-bc5f-6d50a45b8359

[RedMser]

It could fallback to warning, but not sure about the error. It makes sense, but it's being spammed a lot.

I changed that one to ERR_PRINT_ONCE but not sure about the ones in ConfigurationInfo::ensure_valid. I thought of changing the return type to an error enum value so that the errors could be returned as new configuration infos but didn't think about it much yet.

[KoBeWi]

The problem with ERR_PRINT_ONCE is that it will appear only once per editor session 🤔 Seems like the property error is also spamming. Maybe a way to fix both is adding a deferred error print that can happen at most once per frame. Something like queue_print_error, with check for unique messages.

[RedMser]

adding a deferred error print that can happen at most once per frame. Something like queue_print_error, with check for unique messages.

I could try implementing this (maybe as a new PR so this one doesn't get even larger scope?). Couldn't this be implemented the same way as ERR_PRINT_ONCE, but by having a static variable that keeps track of the frame number that the error was printed on, and compare if it's different?

[KoBeWi]

I'd make this "system" local to ConfigurationInfo. You just need a HashSet for errors and a method that prints all of them. Then once an error is to appear, add it to the HashSet instead and call deferred the method if HashSet was empty.

[RedMser]

I implemented it like you suggested. It ""only"" prints 3 errors per update now, which I guess is better than before 😋

[RedMser]

Thanks for the multiple reviews! I applied the latest changes and squashed everything into two commits. My recompile locally was instant, so my rebase maneuver did not accidentally swallow any changes either ^^

[Calinou]

@RedMser Could you look into rebasing this PR so I can review it?

[RedMser]

@Calinou Updated to latest master! ^^

[Calinou]

I get a build failure here after rebasing on top of master on my end:

Compiling scene/2d/physics/scu/scu_scene_2d_physics.gen.cpp ...
In file included from scene/2d/scu/scu_scene_2d_1.gen.cpp:4:
./scene/2d/light_occluder_2d.cpp: In member function 'void OccluderPolygon2D::set_polygon(const Vector<Vector2>&)':
./scene/2d/light_occluder_2d.cpp:93:9: error: 'update_configuration_warning' was not declared in this scope; did you mean 'update_configuration_info'?
   93 |         update_configuration_warning();
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |         update_configuration_info
scons: *** [scene/2d/scu/scu_scene_2d_1.gen.linuxbsd.editor.x86_64.o] Error 1
scons: building terminated because of errors.
[Time elapsed: 00:00:37.33]

[RedMser]

Seems like this got added a few hours after my rebase, haha. Did another, should work now.

[RedMser]

Thanks for your review and feedback! Screenshot after the suggested changes:

The "N Configuration Infos" text should be changed

Done, see _get_summary_text. I intentionally split the translations like this, since I assume some languages can't simply prefix/suffix the word "configuration" without changing conjugation.

Your mockup included emojis for the icons, but I decided not to add an icon for the dropdown title text. The list below already has many icons, so by not having an icon on the title, it gives it a bit of hierarchy. Let me know if you have a strong opinion on changing it.

visual feedback when hovering the dropdown the folding arrow should be moved to the left of the text (and it should point to the right when folded)

Done. I've copied the UI from the "control positioning warning" inspector plugin, so it would make sense to update that one in a new PR as well.

Line spacing should be increased

Done.

The property name written in brackets should be capitalized

Done, although it will probably fail for "localized" property style, since there's a lot of additional logic in the inspector which would need to be pulled out (see code snippet). We would also need to update the struct to include class name.

https://github.com/godotengine/godot/blob/78aaa5562f4559589cfa19f173ac762e13173b46/editor/editor_inspector.cpp#L3177-L3193

I'd consider this a known issue for now which can be optimized later.

The warning icon next to a property should [...] centered

Done by adding 50% extra width. I also noticed that I forgot to update the minimum size, so I added that and refactored duplicate code into helper methods.

When hovering the warning icon, a tooltip [...]

It is a known limitation of this PR, since none of the icon buttons in EditorProperty have tooltips right now (insert keyframe, deletable, etc.). A follow-up PR could introduce a tooltip system for all of these buttons.

[adamscott]

I would keep Node::_get_configuration_warnings() and similar methods for backward compatibility sake. Removing the functionality would break so many plugins.

I would make it so that if configuration info is given, then it takes priority on any configuration warning given.

[RedMser]

@adamscott Current behavior is that both methods exist side-by-side and work, with _get_configuration_warnings marked as deprecated (to be removed in 5.0, which will likely break plugins anyway). The results of both methods are merged together into one array, so nothing takes priority over the other.

I have not explicitly tested backwards compatibility for GDExtension or modules, but I would hope it works as intended when not compiling with DISABLE_DEPRECATED. The idea is that compiling a GDExtension for 4.3 with config warnings API, and loading that into this PR, should work as expected.

[filpano]

Is there anything preventing this from moving forward, except for updating resolving conflicts? I stumbled upon this issue today and it would be really great if this got some traction, especially seeing how most (all?) of the work has already been done.

Is there any possibility of getting this into upcoming 4.4, or is it too late for that?

[AThousandShips]

4.4 is in feature freeze so this won't make it into 4.4, it is still waiting for review by teams this affects

[arkology]

Is there any possibility of getting this into upcoming 4.4, or is it too late for that?

Due to Godot release policy - not possible) This PR doesn't even have 4.4 milestone. (Will be funny if after that it will be decided to merge it for 4.4beta2 😆 )