Skip to content
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

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

Open
samw3 opened this issue Mar 4, 2020 · 9 comments · Fixed by godotengine/godot#68420 · May be fixed by godotengine/godot#90049
Milestone

Comments

@samw3
Copy link

samw3 commented Mar 4, 2020

Describe the project you are working on:
A 2D game with a lot of node work in the editor.

Describe the problem or limitation you are having in your project:
I often forget to set exported script variables which do not have a default and which require a value from the inspector. If I could see a warning icon when I have forgotten to set something it would really help my workflow and cut down on hunting through nodes to find something I missed in a debug session.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Add a warning icon (yellow triangle with exclamation) to inspector exported variable fields which need a setting from the inspector, similar to the collision shape reminders in the Scene tab.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
One way to approach this would be to add an optional required flag to export hints, perhaps before the type:
export(required, int) var healthPoints

If the required hint is specified and the exported var does not have a default and if the user has not supplied a value in the inspector, then the inspector would display a warning icon (yellow triangle with exclamation) inside of the script variable label right aligned.

WarningIcon

Also, source code such as this would be marked as a warning or error:
export(required, int) var healthPoints = 10
Warning: required export variable has a default value. To remove this warning either remove the required keyword or the default value.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
I don't believe it can be worked around, but maybe I'm missing something.

Is there a reason why this should be core and not an add-on in the asset library?:
This relates to how the core editor works.

@samw3 samw3 changed the title An export variable hint which shows a warning icon if the value has not been set in the editor An export variable hint which shows a warning icon if the value has not been set in the inspector Mar 4, 2020
@linkpy
Copy link

linkpy commented Mar 4, 2020

I think this would be hard to make. Exported variable's default value are the default value of their type (a string would just be an empty string, an integer would be 0, etc). What happens when you have required on an exported int, but the value you want is 0 ? The editor wont be able to tell the difference.

Instead you can use _get_configuration_warning :

func _get_configuration_warning() -> String:
    if some_exported_value == 0:
        return "some_exported_value needs a value different than 0."

    return ""

The warning icon will be shown in the SceneTree, next to the node icon which is more visible in my honest opinion. Also, keep in mind that the script needs to have tool at its top to allow the editor to call _get_configuration_warning.

@samw3
Copy link
Author

samw3 commented Mar 4, 2020

I see your point. _get_configuration_warning is cool, I didn't know it existed. Still, the code is more verbose if you have a lot of these (every export needs a setter with update_configuration_warning), but it is a workaround which works.

@Calinou
Copy link
Member

Calinou commented Mar 4, 2020

In addition to _get_configuration_warning(), you can use push_error() and push_warning() to print error/warning messages that will be displayed in the editor's Errors tab.

@Calinou
Copy link
Member

Calinou commented Oct 20, 2021

To resolve this, we could likely amend _get_configuration_warning()'s format to use more advanced objects instead of just a String (or PackedStringArray in 4.0). Each warning would then be able to optionally refer to a single property as a string. We can go further and also allow for node configuration errors. Individual warnings could be given IDs so that they can be dismissed permanently (with serialization), etc. This would all remain optional.

This would make building node configuration warnings more complex, but it'll allow for significantly more freedom, including the ability to implement auto-fix actions with Callables. A system similar to UndoRedo can be used here.

@RedMser
Copy link

RedMser commented Nov 8, 2022

I've implemented part of this proposal at godotengine/godot#68420

It allows for linking a configuration warning to a property, which easily allows for this specific use case.

As it's a dictionary, it should be easy to extend this to support configuration errors, assigning IDs, auto-fix actions, etc.

@TriceratopsAteVelociraptor

Please allow the ability to ignore configuration warnings or visually localize them to the
configuration (sort of what is already done with script warnings). If kept as is, then you have to set aside the mental space to deal with a constant notification on the screen and know that it's what you want.

@akien-mga akien-mga added this to the 4.3 milestone Feb 9, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Feb 17, 2024

Reopening, as the feature is getting reverted due to compatibility breakage in GDExtension (godotengine/godot#88455). It's still wanted, but the implementation should be different.

Since we can't modify the existing _get_configuration_warnings() method, the idea is to create another one - I suggest _get_property_configuration_warnings(), which would take a TypedArray of Dictionary. This would also make it easier to determine when inspector should be updated (see godotengine/godot#88182), because there would be a separate method responsible for property warnings.

The new method could be called together with the old one, although having a separate update_property_configuration_warnings() would make it cheaper to use in terms of required updates.

@KoBeWi KoBeWi reopened this Feb 17, 2024
@AThousandShips AThousandShips modified the milestones: 4.3, 4.x Feb 17, 2024
@Zylann
Copy link

Zylann commented Feb 24, 2024

Would there be a particular reason for this to be limited to nodes? _get_configuration_warnings has historically been nodes-only, likely because it shows up in the scene tree. But property warnings show in the inspector, where you can see not just nodes, but resources too (In fact, my module actually has resource configuration warnings, which so far I just funnel recursively all the way back to nodes, accumulating their path to keep context; only difference with this proposal, is that they are not always specific to a property).

@kendallroth
Copy link

I could see a benefit in mechanism to indicate property field errors/warnings/info (via different icons), as it can be very useful to draw attention to a unique property (whether showing an error, warning, or info). For example, perhaps a max_quantity field where setting it to 0 would display an info icon beside the field (to draw visual attention, albeit not a warning). There are a variety of similar use cases like this that could be helpful if already being extended 🤷.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants