godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Better handling of displaying deleted resources in inspector #10394

Open TsFreddie opened 3 months ago

TsFreddie commented 3 months ago

Describe the project you are working on

Puzzle game with user-oriented editor.

Describe the problem or limitation you are having in your project

While working on a UI theme, it often requires creating and removing style resources. However, inspector does not indicate whether a resource is dangling. This might lead to confusion or even data loss if user is not careful.

This problem is further amplified when user comes back from afk with godot inspector currently showing a non-existent resource.

The frustration originate from using the theme editor, but other scenarios can also showcase the problem. Here's a possibly non-exhaustive list of them:

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

This proposal is derived from the topic here: https://github.com/godotengine/godot/issues/42295

The problem is more obvious when there is a dedicated editor panel involved, which usually does not provide deletion conformation. Whether it needs to be further addressed case by case in each core editor panel would be a separate issue, but addressing the problem in the inspector directly would also allow custom editor panel such as those in addons to have the same benefit.

The inspector should indicate such, if the currently editing resource is dangling or "deleted". Such indicator should be obvious and clearly convey the idea: if user inspect anything else, the resource will be lost.

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

Technical rationale:

Possible implementation:

Visual:

Interaction:

Text:

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

No, it requires modification to the behavior of core inspector.

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

Inspector is core.

dalexeev commented 3 months ago

This is not entirely true. As long as there is at least one reference, the resource (as a descendant of RefCounted) will not be deleted.

The inspector could check the reference count (get_reference_count() <= 1), whether the resource is saved on disk (resource_path.is_empty()), and whether it has unsaved changes, to display the warning.

However, note that this also applies to a newly created resource, so the warning may be confusing to the user.

TsFreddie commented 3 months ago

This is not entirely true. As long as there is at least one reference, the resource (as a descendant of RefCounted) will not be deleted.

That's why I used "dangling" to describe such resource in the actual proposal itself.

I've updated the proposal to handle saved resources. Thanks for the reminder.

However, note that this also applies to a newly created resource, so the warning may be confusing to the user.

I forgot about this. oof. gonna need more idea on handling this case which sounds like complicated the whole check which i thought would be trivial. I do intent on drafting PR for this myself, I could take a more closer look once I do try it.

Calinou commented 3 months ago

However, note that this also applies to a newly created resource, so the warning may be confusing to the user.

In this case, we could have a special case to detect this (e.g. check if the inspector was created after using the Create New Resource dialog), and change the warning accordingly:

This resource has not been saved yet.

It is possible to go back to unsaved resources using the previous/next navigation buttons in the inspector.

TsFreddie commented 3 months ago

Did some tests and ref count seems a bit confusing at times. The most offending one is that the ref count increases every time a property is edited.

This seems unintended. Maybe someone can explain the behavior or confirm this should be an issue. (Found out why, see edit below)

https://github.com/user-attachments/assets/bf5ca37b-433c-4971-9a66-762af8312adb

I temporarily hacked the change counter display in the header to show ref_count->get_reference_count() value instead.

Edit: This is due to undo redo operations holding references to resources, which makes sense. It seems more complicated than first glance to use ref count to determine whether a resource is dangling.

First thought is to count undo redo entries and every reference used in inspector, e.g. preview windows. Although this seem too complicated to maintain the correct count properly.