Inconsistent edit and save behavior for embedded BehaviorTrees

[ydeltastar]

Godot version

4.3

LimboAI version

1.3-dev

LimboAI variant

GDExtension / AssetLib

Issue description

Embedded resources are considered read-only when the editor is not focused on the scene that holds them. In the BT editor, the inspector becomes read-only but the tree can still be edited.

It looks like it works with the default editor settings because it forces all scenes to save but when run/auto_save/save_before_running is disabled, manual save doesn't work for embedded BT of other scenes so changes are never applied.

How to reproduce

• MRP: bt_embedded_issue.zip
• Open main.tscn, main.tscn's behavior tree, and icon.tscn.
• Run the project. It outputs "print 1" and "print 2".
• Disable editor setting run/auto_save/save_before_running.
• While icon.tscn's is the focused edited scene in the editor, move "print 2" above the "print 1" in the tree. And save (Ctrl+S).
• Run the project. It still outputs print 1 and then print 2.
• Now focus on main.tscn, save and run. The print orders changed.

[limbonaut]

Ctrl+S is bound to "Save Scene" - so it wouldn't work in such case. It's Ctrl+Alt+S to save the current BT, but it's broken anyway - even on button click. I made a PR to fix it.

[ydeltastar]

Other resource editors, such as the script and shader editor, save embedded with Ctrl+S when their scenes are not focused.
After taking a look at the source, I think this can be solved with EditorPlugin._save_external_data because it is called on all plugins when any scene saves.

[limbonaut]

EditorPlugin._save_external_data is called by the EditorNode when a scene is saved. Asking a scene to be saved in that callback creates a circular call dependency loop. I think, we'd need a reliable dirty state tracking to break such a loop, but currently the dirty state tracking is not very reliable. Every task would need to report changes with _emit_changed() for it to work, and I feel like it's too much to ask from users. Every core task is calling emit_changed, but how many people actually implement emit_changed in their setters? So currently we assume that everything is dirty all the time (there is still code to track dirty state, but it's unused).

PS: It's kinda weird that "Save Scene" command ends up saving every change in the editor, which can also end up saving other scenes too (but not all of them necessarily?). It's probably because there is no dependency graph implemented for resources.

[limbonaut]

A little bit more context...
Currently, script editor behaves like this:

• When a scene is saved, EditorPlugin::_save_external_data is called.
• Script editor plugin saves each dirty external script and collects scene paths that have a dirty built-in script (and need to be saved) into a hash set.
• If there are scenes in that set, EditorNode::save_scene_list(scene_paths) is called.
  Function save_scene_list is capable to break out of a loop, printing an error. However, it's not available in GDExtension, and we still would need to crack the dirty state tracking problem.
  Maybe we need a different approach. Intercepting Ctrl+S when LimboAI editor is visible? Not sure if it's a good idea.

[ydeltastar]

PS: It's kinda weird that "Save Scene" command ends up saving every change in the editor, which can also end up saving other scenes too (but not all of them necessarily?). It's probably because there is no dependency graph implemented for resources.

The saving process is overall a bit hacky in the engine. I don't use autosave, for example, because it tries to save every scene open in the editor so some resources also save multiple times, it triggers multiple filesystem_changed signals, some editors do heavy operations after it, and other issues. It snowballs the more files a project has and slows down project startup a lot waiting for the process to finish. Manually saving at least affects a single scene and a few files so it is much faster.

Well, the worst-case scenario is losing changes so I understand going for redundancy if it means avoiding losing data. A dependency graph could help for sure.

Every task would need to report changes with _emit_changed() for it to work, and I feel like it's too much to ask from users. Every core task is calling emit_changed, but how many people actually implement emit_changed in their setters? So currently we assume that everything is dirty all the time (there is still code to track dirty state, but it's unused).

Maybe EditorProperty or EditorInspector can solve this. It has signals for property changes so the dirty tracking logic could rely on the inspector itself.

[limbonaut]

Maybe EditorProperty or EditorInspector can solve this. It has signals for property changes so the dirty tracking logic could rely on the inspector itself.

It's a promising concept. Might be a good solution if we can make it handle properties with built-in resources somehow.

[limbonaut]

#243 fixes built-in resources not being saved on save action (UI button or Ctrl-Alt-S). Keeping this open, because it's only partly addressed. I think that pressing Ctrl+S should affect what is on screen right now, and it's not always the case.