Allow configuration warnings to refer to a property (reverted) #68420

RedMser · 2022-11-08T19:19:08Z

Configuration warnings changed from PackedStringArray to an Array that accepts a mix of Strings and Dictionaries { message: String, property?: String }.
If property is set, then show a clickable warning icon on property in the inspector.
Strings in this array are converted to an equivalent Dictionary with message filled.

This PR also lays the groundwork for further configuration warning enhancements like godotengine/godot-proposals#2519

As an example, the PointLight2D node was updated to use this system.

22-11-08_20-13-15_godot.windows.editor.dev.x86_64_Snowgeese.mp4

The PR is split into two commits for easier reviewing. The second commit only fixes the signature of the get_configuration_warnings method for all classes using it.

TODO:

Change warning icon based on how many warnings the property has. Probably have to move the property name filtering logic out of the as_string method.
Would be nice to have a tooltip when hovering the icon, but since other icons don't have this yet, it can be done in a follow-up PR.
It seems to be necessary to introduce a compatibility method for GDExtension here.
- No idea how to test it, so someone should verify what it's doing is correct. Especially since there's also a virtual method involved here, whose signature changed but didn't get any compat bind (I followed what was done in AnimationMixer.compat.inc)!

Mickeon · 2022-11-09T18:44:29Z

Wow... what work. My only concern is that it's not just a breaking change after 4.0, but it'd make the virtual method ever so laborious to use for simpler cases. Having to create { message: "Something is wrong etc. etc."}, that is.

I can assume that my concerns are exaggerated, as not everyone casually makes use of the method, but it makes me wonder if it could be an Array of Variant, where each one of them can either be a String or a Dictionary. This way the previous, simplistic behavior is also kept. Having it as a Dictionary is definitely the way to go, however.

RedMser · 2022-11-09T18:49:28Z

@Mickeon I totally agree. Since there is no concrete proposal behind this, I implemented it in the most straightforward way I could think of. But for the sake of compatibility and convenience, I'd agree that treating plain strings just like before would be nice.

Would like some more opinions on this before I try implementing anything more, e.g. if this is the kind of API breakage that's also acceptable for minor versions (since like you said, this API isn't commonly used as it stands).

MewPurPur · 2022-11-10T14:39:31Z

I think it's a bit weird duplicating config warnings like this. It gives off a feeling that two things are wrong, instead of just one.

If I gather correctly, this can be implemented in a not compat-breaking way, by having separate "node configuration" and "property configuration" warnings? Here's my idea:

Keep config warnings as is.
Have property warnings point to a property with a warning message.
Property warnings update the config warning in standardized ways, i.e. "[property_name] of this [node_name] has not been set up properly."

Mickeon · 2022-11-10T15:03:36Z

When I thought on how to implement this, I also thought of using a general String format, kind of like hint_strings, to expand the current behavior. But I do get the use of Dictionaries. I believe it allows greater control in the future. Imagine being able to click on the PhysicsBody2D's CollisionShape warning, press a button to swiftly fix the issue, and a related action Callable opens the Add Node dialog. Very convenient and user-friendly.

RedMser · 2023-07-05T13:44:38Z

Updated so that get_configuration_warnings may be a mixed array of Strings and Dictionaries. This keeps compatibility with existing C++ and GDScript code, and keeps an easy way to write simple configuration warnings.

Also for easier review, I split the changes into two commits, since one is just a large batch of signature changes which do not need reviewing.

editor/editor_inspector.cpp

scene/main/node.cpp

KoBeWi · 2023-07-05T15:44:02Z

I checked the code and the implementation seems fine, but changing return type still breaks compatibility. Although Array and PackedStringArray are mostly compatible so maybe it's not a big deal.

RedMser · 2023-07-05T17:01:36Z

editor/editor_inspector.cpp

+			for (int i = 0; i < boundaries.size(); i += 2) {
+				const int start = boundaries[i];
+				const int end = boundaries[i + 1];
+				lines.append(configuration_warning.substr(start, end - start + 1));


Once #79054 is merged, the + 1 has to be removed from here.

KoBeWi

The implementation looks fine (aside from breaking compatibility; maybe there is a solution that could avoid it). There is nice flexibility with Dictionaries, which could help with #74357

Would be nice to have a tooltip when hovering the icon, but it doesn't seem like that's easily possible with these "inline" icons? Would like to hear suggestions on how to tackle this.

This can be implemented using get_tooltip() virtual method and new_configuration_warning_hover. But other icons don't have tooltips; it could be implemented for all of them in another PR.

Mickeon · 2024-01-24T18:55:39Z

Yyyeaah, this one cannot be done now without breaking compatibility, unless it is possible for it not to.

RedMser · 2024-02-08T15:57:38Z

Hi @Mickeon , your PR #87535 removed Node::get_configuration_warnings_as_string which I intended to use here.

Clicking on the ⚠️ button that appears next to a property, a dialog similar to the one from the scene tree would show up. It would filter to only show relevant warnings, so the function would get an optional property parameter. (As discussed above, tooltips aren't currently implemented for inspector buttons, so the dialog would be the primary way to get this info for now...)

Without the function, this would involve duplicating the logic involving the bullet points, indenting new lines etc.

Do you think it would be fine to re-introduce it here, now that there'd be multiple places that need it for different purposes? Or was there a strong reason against having that function around in the first place?

Mickeon · 2024-02-08T16:06:31Z

Do you think it would be fine to re-introduce it here, now that there'd be multiple places that need it for different purposes? Or was there a strong reason against having that function around in the first place?

The reason it was removed was because it was only used two times: The tooltip, and the popup. However, the way these two are displayed is entirely different, to the point where relying on that function's result was basically useless, as the strings had to be reformatted anyhow. It also was not encompassed with TOOLS_ENABLED, which included it in release builds for no reason (if the compiler is stupid).

With that said, I am not sure if it should be brought back. At the same time, this PR may override what was made in #87535, so you may as well... go all in?

The original popup itself has always kind of sucked, this may be an opportunity to refactor it outright.

AThousandShips

Code and documentation looks good

misc/extension_api_validation/4.2-stable.expected

editor/editor_inspector.cpp

This is used by the inspector so it can show a warning icon on a specific property.

akien-mga · 2024-02-09T11:34:26Z

Thanks!

Zylann · 2024-02-10T21:24:59Z

What should GDScript plugins do if they support engine versions prior this change?

func _get_configuration_warnings() -> PackedStringArray:

Updated so that get_configuration_warnings may be a mixed array of Strings and Dictionaries. This keeps compatibility with existing C++ and GDScript code

GDScript doesn't have a preprocessor to make this conditionally implemented differently. If I write -> Array, it will break in versions prior this change.
That means I have to remove typing.

KoBeWi · 2024-02-10T21:32:08Z

That means I have to remove typing completely.

Yes, that's the solution if you want to support older versions.

aaronfranke · 2024-02-13T17:40:49Z

scene/main/node.h

-	virtual PackedStringArray get_configuration_warnings() const;
+	virtual Array get_configuration_warnings() const;
+	Dictionary configuration_warning_to_dict(const Variant &p_warning) const;
+	Vector<Dictionary> get_configuration_warnings_as_dicts() const;
+	Vector<Dictionary> get_configuration_warnings_of_property(const String &p_property = String()) const;
+	PackedStringArray get_configuration_warnings_as_strings(bool p_wrap_lines, const String &p_property = String()) const;


In #81892 I proposed removing the String conversion method from Node, as it's only used by the editor. This PR is going in the opposite direction, adding more formatting methods to Node.

IMO, only virtual Array get_configuration_warnings() const; should exist in Node. The rest are editor-specific.

That is very fair. Since they are not bound methods (and shouldn't be), it's not a problem to move them elsewhere as a refactor further down the line.

I just don't know enough about organization in C++ to really know a good place for them, or if you can just create a editor_configuration_warnings.cpp file for this and it'll magically work?

Note that it was also removed in #87535 for similar reasons.

(Also note that this change has been more compatibility-breaking than previously assumed. Heck).

dsnopek · 2024-02-15T21:51:55Z

scene/main/node.h

-	GDVIRTUAL0RC(Vector<String>, _get_configuration_warnings)
+	GDVIRTUAL0RC(Array, _get_configuration_warnings)


This breaks binary compatibility for GDExtension. Any GDExtension that implements _get_configuration_warnings() from before this change will probably segfault. Does anyone know if there's an issue discussing this yet? If not, I can make one

I just created issue #88379

…on_warnings-compat-breakage Revert #68420 to undo `get_configuration_warnings` compat breakage

…configuration_warnings-compat-breakage Revert godotengine#68420 to undo `get_configuration_warnings` compat breakage

RedMser requested review from a team as code owners November 8, 2022 19:19

RedMser force-pushed the config-warning-prop branch 2 times, most recently from ee25728 to d6b36cb Compare November 8, 2022 19:22

KoBeWi added enhancement feature proposal topic:editor breaks compat labels Nov 8, 2022

RedMser force-pushed the config-warning-prop branch from d6b36cb to e800184 Compare November 8, 2022 20:41

RedMser mentioned this pull request Nov 8, 2022

An export variable hint which shows a warning icon if the value has not been set in the inspector godotengine/godot-proposals#550

Open

Calinou added this to the 4.x milestone Nov 8, 2022

Geometror mentioned this pull request Nov 28, 2022

Move z_index, z_as_relative and y_sort_enabled from Node2D to CanvasItem #68070

Merged

YuriSizov mentioned this pull request Feb 9, 2023

Warn if custom ambient light color is ignored due to sky contribution #53927

Open

RedMser force-pushed the config-warning-prop branch from e800184 to cf1ae1b Compare July 5, 2023 13:42

KoBeWi reviewed Jul 5, 2023

View reviewed changes

editor/editor_inspector.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jul 5, 2023

View reviewed changes

editor/editor_inspector.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Jul 5, 2023

View reviewed changes

scene/main/node.cpp Outdated Show resolved Hide resolved

RedMser force-pushed the config-warning-prop branch from cf1ae1b to d9e57ab Compare July 5, 2023 16:44

RedMser commented Jul 5, 2023

View reviewed changes

KoBeWi approved these changes Jul 7, 2023

View reviewed changes

RedMser mentioned this pull request Nov 3, 2023

Add @required annotation for exported properties godotengine/godot-proposals#8343

Open

RedMser force-pushed the config-warning-prop branch from 2e1c962 to f401dab Compare January 24, 2024 17:40

RedMser requested a review from a team as a code owner January 24, 2024 17:40

RedMser force-pushed the config-warning-prop branch from f401dab to 944d65c Compare February 8, 2024 19:00

RedMser requested review from a team as code owners February 8, 2024 19:00

AThousandShips approved these changes Feb 8, 2024

View reviewed changes

misc/extension_api_validation/4.2-stable.expected Show resolved Hide resolved

RedMser force-pushed the config-warning-prop branch from 944d65c to 9a5d41f Compare February 8, 2024 19:13

akien-mga reviewed Feb 8, 2024

View reviewed changes

editor/editor_inspector.cpp Outdated Show resolved Hide resolved

RedMser added 2 commits February 8, 2024 23:05

Allow configuration warnings to refer to a property

bf37a9b

This is used by the inspector so it can show a warning icon on a specific property.

Update Node::get_configuration_warnings signature

d3852de

RedMser force-pushed the config-warning-prop branch from 9a5d41f to d3852de Compare February 8, 2024 22:06

akien-mga merged commit 86ffe92 into godotengine:master Feb 9, 2024
16 checks passed

KoBeWi mentioned this pull request Feb 10, 2024

Can't write Label text #88176

Closed

TokisanGames mentioned this pull request Feb 11, 2024

[4.4] Configuration warnings may change implementation TokisanGames/Terrain3D#321

Open

aaronfranke reviewed Feb 13, 2024

View reviewed changes

Calinou mentioned this pull request Feb 15, 2024

Add a configuration warning system for Resources godotengine/godot-proposals#9098

Open

dsnopek reviewed Feb 15, 2024

View reviewed changes

dsnopek mentioned this pull request Feb 15, 2024

GDExtension binary compatibility broken for _get_configuration_warnings() #88379

Closed

akien-mga mentioned this pull request Feb 17, 2024

Revert #68420 to undo get_configuration_warnings compat breakage #88455

Merged

akien-mga added a commit that referenced this pull request Feb 17, 2024

Merge pull request #88455 from akien-mga/revert-68420-get_configurati…

9ae8a0e

…on_warnings-compat-breakage Revert #68420 to undo `get_configuration_warnings` compat breakage

RedMser mentioned this pull request Mar 30, 2024

Rework the Configuration Warning system #90049

Open

akien-mga changed the title ~~Allow configuration warnings to refer to a property~~ Allow configuration warnings to refer to a property (reverted) Aug 14, 2024

akien-mga added a commit to akien-mga/godot that referenced this pull request Nov 6, 2024

Merge pull request godotengine#88455 from akien-mga/revert-68420-get_…

feaa6ce

…configuration_warnings-compat-breakage Revert godotengine#68420 to undo `get_configuration_warnings` compat breakage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration warnings to refer to a property (reverted) #68420

Allow configuration warnings to refer to a property (reverted) #68420

RedMser commented Nov 8, 2022 •

edited

Loading

Mickeon commented Nov 9, 2022 •

edited

Loading

RedMser commented Nov 9, 2022 •

edited

Loading

MewPurPur commented Nov 10, 2022

Mickeon commented Nov 10, 2022

RedMser commented Jul 5, 2023

KoBeWi commented Jul 5, 2023

RedMser Jul 5, 2023

KoBeWi left a comment

Mickeon commented Jan 24, 2024

RedMser commented Feb 8, 2024 •

edited

Loading

Mickeon commented Feb 8, 2024 •

edited

Loading

AThousandShips left a comment

akien-mga commented Feb 9, 2024

Zylann commented Feb 10, 2024 •

edited

Loading

KoBeWi commented Feb 10, 2024

aaronfranke Feb 13, 2024

RedMser Feb 13, 2024

Mickeon Feb 13, 2024 •

edited

Loading

dsnopek Feb 15, 2024

dsnopek Feb 15, 2024

		GDVIRTUAL0RC(Vector<String>, _get_configuration_warnings)
		GDVIRTUAL0RC(Array, _get_configuration_warnings)

Allow configuration warnings to refer to a property (reverted) #68420

Allow configuration warnings to refer to a property (reverted) #68420

Conversation

RedMser commented Nov 8, 2022 • edited Loading

Mickeon commented Nov 9, 2022 • edited Loading

RedMser commented Nov 9, 2022 • edited Loading

MewPurPur commented Nov 10, 2022

Mickeon commented Nov 10, 2022

RedMser commented Jul 5, 2023

KoBeWi commented Jul 5, 2023

RedMser Jul 5, 2023

Choose a reason for hiding this comment

KoBeWi left a comment

Choose a reason for hiding this comment

Mickeon commented Jan 24, 2024

RedMser commented Feb 8, 2024 • edited Loading

Mickeon commented Feb 8, 2024 • edited Loading

AThousandShips left a comment

Choose a reason for hiding this comment

akien-mga commented Feb 9, 2024

Zylann commented Feb 10, 2024 • edited Loading

KoBeWi commented Feb 10, 2024

aaronfranke Feb 13, 2024

Choose a reason for hiding this comment

RedMser Feb 13, 2024

Choose a reason for hiding this comment

Mickeon Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

dsnopek Feb 15, 2024

Choose a reason for hiding this comment

dsnopek Feb 15, 2024

Choose a reason for hiding this comment

RedMser commented Nov 8, 2022 •

edited

Loading

Mickeon commented Nov 9, 2022 •

edited

Loading

RedMser commented Nov 9, 2022 •

edited

Loading

RedMser commented Feb 8, 2024 •

edited

Loading

Mickeon commented Feb 8, 2024 •

edited

Loading

Zylann commented Feb 10, 2024 •

edited

Loading

Mickeon Feb 13, 2024 •

edited

Loading