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

EditorUndoRedoManager is polluting scene history. #37

Closed
Rubonnek opened this issue Feb 14, 2024 · 5 comments
Closed

EditorUndoRedoManager is polluting scene history. #37

Rubonnek opened this issue Feb 14, 2024 · 5 comments
Labels
bug Something isn't working
Milestone

Comments

@Rubonnek
Copy link
Contributor

Under certain scenarios the usage of EditorUndoRedoManager in the LimboAI Editor plugin will pollute the scene history when it shouldn't -- for example when creating a new BehaviorTree and adding tasks to it while on an empty scene.

This will make will make undo/redo operations error out from time to time and not work as expected sometimes.

Since EditorUndoRedoManager is not able to automatically track parent/child relationships of Resource objects (which include BTTask), we need a way to force it to use a specific undo/redo history in order to properly manage undo/redo operations in the Editor.

Once the following patch is merged upstream, the LimboAI Editor undo/redo operations can be properly fixed.

@limbonaut limbonaut added the bug Something isn't working label Feb 15, 2024
@limbonaut limbonaut added this to the 1.2 milestone Jun 30, 2024
@limbonaut
Copy link
Owner

limbonaut commented Aug 21, 2024

Not sure how force_fixed_history is going to help. It seems the problem is that an edited resource without a path is associated with a currently open scene. Switching to another scene just associates future edits with that one. Closing a scene wipes the associated actions from the history.

Would be nice to have a separate history object for each BehaviorTree, but it seems that editor only supports separate histories for scenes, and a global history. It may be possible to have an anonymous UndoRedo object tracking history omitting EditorUndoRedoManager, but such history would not show up in the History tab. I'm not sure how to proceed.

PS: I tried passing BehaviorTree as a context_object in create_action(), but it didn't change anything.

@limbonaut
Copy link
Owner

So I tried to use force_fixed_history() at first with an active BehaviorTree as context_object, and it didn't help the issue.
However, I found a workaround - passing a dummy context object and calling force_fixed_history forces action to belong to the global history. Admittedly, it's a hack, but it works in both the module and GDExtension APIs. I'm about to make a PR with those changes - it works fine in my testing. The downside is that BT undo-redo history is mixed with each other and global history. It's still an improvement.

While looking through the godot editor code, I didn't find a way to create a custom history object and associate it with a BT resource. There are some methods to create history objects, but they are not exposed in the API - so they won't work with GDExtension. And editor UI also doesn't seem to support custom histories. I was contemplating the idea of creating a custom pool of UndoRedo objects, one per each BT. Ctrl-Z would have to be intercepted in the plugin code. I think, we'd be able to track history for each individual BT separately this way, but the actions won't show up in the History tab.

@Rubonnek
Copy link
Contributor Author

Not sure how force_fixed_history is going to help.

Honestly, I don't quite recall the exact usage I had in mind since I lost the changes where I had a rough implementation. I went through the patch you have and it looks close to what I had in mind.

Would be nice to have a separate history object for each BehaviorTree, but it seems that editor only supports separate histories for scenes

I keep thinking it can support separate histories for each BehaviorTree and you may have to use force_fixed_history() as well.

I suppose what I saw back when I opened the ticket was along the lines of:

  • Using get_history_id_for_object().
  • Strictly using create_action_for_history().
  • Using forced_fixed_history() somewhere.

I'm not entirely sure how this would play out in the History tab, but I hope this helps a bit.

@limbonaut
Copy link
Owner

I was thinking along those lines, but in practice I had several issues.

  • get_history_id_for_object() returns scene history for resources with empty path, which is supposed to be helped by force_fixed_history - but it didn't work. I'd have to dig deeper into code to understand why.
  • create_action_for_history() is not exposed in the API, so GDExtension can't call that.

@limbonaut
Copy link
Owner

limbonaut commented Aug 21, 2024

For the future reference, here are a few facts I established while digging through code:

  • get_history_id_for_object returns:
    • 0 for non-resource objects, and also for resources with the path set to external file
    • 1 and higher - for resources with no path set; this is the history ID of the currently edited scene

I'm not 100% sure, but it seems to me that editor currently only tracks global and per-scene histories. You can create a history object with arbitrary ID, and use it to register actions, but it's unlikely to show in the History tab, and some of those methods are not exposed - so you can't use them in GDExtension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants