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

Undo and redo logic with generic atomic actions and auto component change undo/redo #2

Merged
merged 16 commits into from
Sep 23, 2024

Conversation

rewin123
Copy link
Contributor

@rewin123 rewin123 commented Jan 17, 2024

This code provides a lightweight solution with minimal dependencies for implementing automatic undo functionality in an editor. The key concept behind this library is to leverage Add, Changed, and RemovedComponent filters to dynamically create a chain of world state changes.

Core concept

All important world change can be describe as

pub trait EditorChange {
    fn revert(
        &self,
        world: &mut World,
        entity_remap: &HashMap<Entity, Entity>,
    ) -> Result<ChangeResult, String>;

    fn debug_text(&self) -> String;

    fn get_inverse(&self) -> Arc<dyn EditorChange + Send + Sync>;
}

These changes are then stored in the resource as Vec<Arc<dyn EditorChange>>, allowing seamless sequential undo operations through the implementation of the revert method within the EditorChange trait.

Change chain and undo/redo demonstration from space_editor crate
ezgif-7-fd8c7bd006

Example usage
Minimal example

use bevy::prelude::*;
use undo::*;

fn main() {
    let mut app = App::new()
      .add_plugins((DefaultPlugins, UndoPlugin)) //Add Undo plugin
      .auto_undo::<Name>() //register Name component in undo system
      .auto_reflected_undo::<Parent>() //example register components that not implement Clone, but implement Reflect
      .auto_reflected_undo::<Children>()
      .add_systems(Startup, setup)
      .run();
  }
  
fn setup(
        mut cmds : Commands) {
    cmds.spawn((
    UndoMarker, //Mark entity to be used in undo logic
    Name::new("Some name")
  ));
}
  
fn do_undo(
    mut events: EventWriter<UndoRedo>) {
  events.send(UndoRedo::Undo); //Will delete Name component
}

This is example from test, that restore two entities with parent-child relation after destruction

#[test]
    fn test_undo_with_remap() {
        let mut app = App::new();
        app.add_plugins(MinimalPlugins).add_plugins(UndoPlugin);
        app.add_plugins(HierarchyPlugin);

        app.auto_reflected_undo::<Parent>();
        app.auto_reflected_undo::<Children>();

        let test_id_1 = app.world.spawn(UndoMarker).id();
        let test_id_2 = app.world.spawn(UndoMarker).id();

        app.world.send_event(NewChange {
            change: Arc::new(AddedEntity { entity: test_id_1 }), //Undo can only catch delete/add/change for components, but not for entity spawn/delete. So entity spawn/delete must be added to change chain manually
        });
        app.world.send_event(NewChange {
            change: Arc::new(AddedEntity { entity: test_id_2 }),
        });

        app.update();
        app.update();

        app.world.entity_mut(test_id_1).add_child(test_id_2); //Make parent-child relation. undo logic will catch it automatically 

        app.update();
        app.update();
        app.cleanup();

        app.world.entity_mut(test_id_1).despawn_recursive();
        app.world.send_event(NewChange {
            change: Arc::new(RemovedEntity { entity: test_id_1 }),
        });

        app.update();
        app.update();

        app.world.send_event(UndoRedo::Undo);

        app.update();
        app.update();
        app.update();

        assert!(app.world.get_entity(test_id_1).is_none()); //Check thats old entities despawned
        assert!(app.world.get_entity(test_id_2).is_none());
        assert_eq!(app.world.entities().len(), 2); //Check that new entities was restored

        let mut query = app.world.query::<&Children>();
        assert!(query.get_single(&app.world).is_ok()); //Check that new 2 entities has parent-child relation
    }

While the code is currently marked as a draft in the pull request, it lacks sufficient documentation and could benefit from improved code cleanliness. Despite these issues, the implemented functionality holds significant potential for future editor prototypes. I'm seeking assistance in determining the level of documentation required for the code to be accepted into the project.

@rewin123
Copy link
Contributor Author

I added an example, I think it's clear further how to use it
ezgif-3-3e6d3781ea

@rewin123 rewin123 marked this pull request as ready for review January 19, 2024 21:50
@rewin123 rewin123 requested a review from JMS55 January 19, 2024 21:52
@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jan 21, 2024

Looks good, would it be possible to warn (once) about changes to components which did not register undo?

@rewin123
Copy link
Contributor Author

rewin123 commented Jan 21, 2024

Thanks!
Do you want throw warnings about changes to components whose changes are hidden with OneFrameUndoIgnore? Or warn about changes in all components that are not registered via auto_reflected_undo?

@MiniaczQ
Copy link
Contributor

MiniaczQ commented Jan 22, 2024

I meant the latter, I'm not sure what first is about 🤔

@ohsalmeron
Copy link

Looking awesome! is there a way we can help with anything? like adding more features

@rewin123
Copy link
Contributor Author

rewin123 commented Jan 23, 2024

I meant the latter, I'm not sure what first is about 🤔

Theoretically yes. But we need to create a system to request changes to all components for all entities. That could be an expensive calculation, I think.

OneFrameUndoIgnore is component for hiding entity from undo systems during two frames. Its need to no register changes, which made by Undo or Redo action

@rewin123
Copy link
Contributor Author

rewin123 commented Jan 23, 2024

Looking awesome! is there a way we can help with anything? like adding more features

Thanks!

This plugin does not support automatic change logging for resources and for entity creation and destruction events. Probably some of these things should be added next.

@ThomasAlban
Copy link

This looks really cool, and interesting to see your implementation of this. I tried myself to implement a commands based undo system in bevy for a 3rd party editor I'm working on, but ultimately gave up due to complications arising from things like other components storing entity IDs which, if deleted and reinserted in the undo system, would then have a different entity ID. How does this implementation get around this problem?

@rewin123
Copy link
Contributor Author

rewin123 commented Feb 20, 2024

Ugh. That's a very good question.

The proposed Undo plugin solves this problem partially. If deletion of a group of entities was detected, Undo can be used to restore the whole group, and the plugin will also set up-to-date values of all fields with Entity type in all restored components by recursive function based on Reflect trait. So, ingroup entity links will be work after Undo action.

However, if entities outside the deleted group had references to that group, they will not be restored as there is no created logic to restore Entity references.

Although I have an idea how to do it a bit brootforce style. For example, register all components that may have references and when recreating an entity using Undo, run through all registered components with references, through all their fields with Entity type and if there is an id with a restored Entity, replace the value with up-to-date.

@ThomasAlban
Copy link

You could also, perhaps, instead of actually deleting entities, delete all the components off the entity (so it's just an empty entity) and then replace all the components on an undo - that way the ID is reserved so it doesn't need to be manually replaced.
Another question I have, is how (if at all) does your system deal with the fact that modifications could happen over multiple frames, e.g. dragging the position of something using a gizmo - as you wouldn't want the undo stack filling up with every frame the thing was modified.
Complex relationships are always a challenge when building commands undo systems, the way I'm planning on implementing undo (unless I end up being able to use your system!) is just serialising the bevy world every time an undo point needs to be set - I haven't tried this approach yet though, so I don't know how well it will work!

@rewin123
Copy link
Contributor Author

The entity saving approach could indeed solve the problem of saving dependencies. However, if there are frequently created short-lived entities in the world ( for example particle system), it is actually possible to overflow the u32 index of the entity without deleting them. (Although it would take 111 hours for 10000 entities per second, this is still achievable).

The proposed system has two stages for dealing with time durationally extended changes. The first stage is to register the change of a component using Change. As soon as a component is changed the entity is marked with ChangeMarker. The second system waits until all marked components are no longer in the changed state. Once this happens, the system waits two more frames (so as not to register changes to the stack due to a one-frame stop) and only then creates an event that a component of such an entity has been changed.

The event is caught by the system that manages the entire Undo system stack. If multiple changes happen in a single frame or multiple frames in a row, they are logged as a single combined ManyChanges change. This allows you to undo an time extended change with a single Ctrl-Z. For example, such a combination is necessary because the change of position from parent to child is transferred with a delay of 1 frame from the Transform calculation to the PostUpdate schedule. And it also allows any other changes that have been passed down the reaction chain to be combined.

@ThomasAlban
Copy link

ah cool, that sounds pretty great. Your system seems really cool and far beyond anything I was able to achieve! If you can implement a way to register entities that may hold entity references which would need to be updated, then I'd love to be able to start using your system in my own editor. The entity reference thing is important for me because my editor stores a graph data structure where each entity holds references to 'next' and 'previous' entities. Without entity ref updating things would very quickly break!

@alice-i-cecile
Copy link
Member

@rewin123 I'm inclined to merge this; can you swap the crate name to match the new crate structure?

@rewin123
Copy link
Contributor Author

@rewin123 I'm inclined to merge this; can you swap the crate name to match the new crate structure?

Yeah. I just finished updating to the latest version of bevy and renamed crate in the process ^_^

@rewin123 rewin123 changed the title Dependless undo and redo logic for componets Undo and redo logic for componets Sep 23, 2024
@rewin123 rewin123 changed the title Undo and redo logic for componets Undo and redo logic with generic atomic actions and auto component change detection Sep 23, 2024
@rewin123 rewin123 changed the title Undo and redo logic with generic atomic actions and auto component change detection Undo and redo logic with generic atomic actions and auto component change undo/redo Sep 23, 2024
@rewin123 rewin123 added S-Ready-For-Final-Review This PR is ready to be merged. Go for it! A-Undo Related to undo/redo logic or bevy_undo crate labels Sep 23, 2024
})
.insert(Controller)
.insert(UndoMarker) //Only entities with this marker will be able to undo
.insert(OneFrameUndoIgnore::default()); // To prevert add "Transform add" change in change chain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.insert(OneFrameUndoIgnore::default()); // To prevert add "Transform add" change in change chain
.insert(OneFrameUndoIgnore::default()); // To prevent adding "Transform add" change in change chain

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 23, 2024
Merged via the queue into bevyengine:main with commit bc2c235 Sep 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Undo Related to undo/redo logic or bevy_undo crate S-Ready-For-Final-Review This PR is ready to be merged. Go for it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants