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

bevy_reflect: Reflection diffing #13253

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 6, 2024

Objective

Closes #5563.

Note

Much of this code was actually written almost two years ago. I did my best to revive the work, but there may be subtle inconsistencies with the rest of the codebase or reduced quality in some areas (I was young okay!). So please do feel free to point those out so I can correct them!

Sometimes you need more information than just "are these two values equal?" Sometimes you need something more like "why aren't these two values equal?" This is where diffing comes in.

With diffing, we can pinpoint exactly why two values differ from each other. This can be really helpful for handling specific differences, which would otherwise require lots of specific if-statement spaghetti.

Additionally, because diffing is solely concerned with the changes, it means they can also be more efficient in memory usage. Rather than storing a copy of the entire changed value, we can simply store a copy of the change itself. This can be a massive win for large structs where only a single field is changed.

Because of the smaller memory usage, it can also have huge gains on networking. Instead of sending entire objects to the server, you can just send the changes.

This PR is an attempt to add some basic support for diffing within bevy_reflect.

Solution

Adds reflection-based diffing.

This adds two methods to Reflect: Reflect::diff and Reflect::apply_diff.

Reflect::diff

When Reflect::diff is called on two values, it will return a Diff enum.

This enum contains variants for: NoChange, Replaced, and Modified.

NoChange is pretty self-explanatory. There is no perceived change1 between the two values.

Replaced usually means that the two values are not of the same type. There's no good way to assess differences between two different types, so we just say that it was replaced.

Modified is where the fun begins. This contains a DiffType which is another enum that holds kind-specific diffs, such as StructDiff, MapDiff, and TupleDiff. These values are recursive in that they may contain other Diff values.

#[derive(Reflect)]
struct Foo {
    bar: Vec<i32>
}

let old = Foo { bar: vec![1, 2, 3] };
let new = Foo { bar: vec![1, 2, 4, 5] };
dbg!(old.diff(&new).unwrap());
// Output (with some whitespace/commas removed):
// Modified(
//   Struct(
//     StructDiff {
//       fields: {
//         "bar": Modified(
//           List(
//             ListDiff {
//               changes: [
//                 Deleted(2),
//                 Inserted(3, Borrowed(4)),
//                 Inserted(3, Borrowed(5)),
//               ],
//             },
//           ),
//         ),
//       },
//     },
//   ),
// )

Reflect::apply_diff

With a Diff created, you can then apply the changes to the old value to get the new one.

By default, Diff will attempt to store references (hence the Borrowed in the output above) rather than owned data. The full type is actually Diff<'old, 'new>. However, note that you can convert this to an owned instance using Diff::clone_diff, which returns a Diff<'static, 'static>.

We'll need to convert this to an owned diff if we wish to modify the original value (see Open Questions for details).

let mut old = Foo { bar: vec![1, 2, 3] };
let new = Foo { bar: vec![1, 2, 4, 5], };
let diff = old.diff(&new).unwrap().clone_diff();

diff.apply(&mut old).unwrap();
assert_eq!(old.bar, vec![1, 2, 4, 5]);

And with this, we have successfully applied our changes to the original value. Note that Reflect::apply is strictly additive, so calling old.apply(&new) would not have removed the 3 from bar. We can consider addressing this in the future (perhaps by internally using diffing).

Mini-Example

As a fun sample of how this can be used, here's a proof-of-concept undo/redo system:

Undo/Redo Example
#[derive(Debug)]
struct HistoryEvent {
    forwards: Diff<'static, 'static>,
    backwards: Diff<'static, 'static>,
}

impl HistoryEvent {
    pub fn new(old: &dyn Reflect, new: &dyn Reflect) -> Result<Self, DiffError> {
        Ok(Self {
            forwards: old.diff(new)?.clone_diff(),
            backwards: new.diff(old)?.clone_diff(),
        })
    }

    pub fn undo(&self, value: &mut dyn Reflect) -> Result<(), DiffApplyError> {
        self.backwards.clone_diff().apply(value)
    }

    pub fn redo(&self, value: &mut dyn Reflect) -> Result<(), DiffApplyError> {
        self.forwards.clone_diff().apply(value)
    }
}

#[derive(Default)]
struct History {
    events: Vec<HistoryEvent>,
    index: usize,
}

impl History {
    pub fn push(&mut self, event: HistoryEvent) {
        self.events.truncate(self.index);
        self.events.push(event);
        self.index += 1;
    }

    pub fn undo(&mut self, value: &mut dyn Reflect) -> Result<(), DiffApplyError> {
        if self.index == 0 {
            return Ok(());
        }

        self.index -= 1;
        self.events[self.index].undo(value)
    }

    pub fn redo(&mut self, value: &mut dyn Reflect) -> Result<(), DiffApplyError> {
        if self.index == self.events.len() {
            return Ok(());
        }

        self.events[self.index].redo(value)?;
        self.index += 1;

        Ok(())
    }
}

// ------------------------------- //

#[derive(Reflect, Debug, PartialEq)]
struct Player {
    health: i32,
    mana: i32,
}

impl Player {
    pub fn take_damage(&mut self, damage: i32) {
        self.health -= damage;
    }

    pub fn consume_mana(&mut self, cost: i32) {
        self.mana -= cost;
    }
}

let mut history = History::default();

let mut player = Player {
    health: 100,
    mana: 100,
};

let old = player.clone_value();
player.take_damage(25);

history.push(HistoryEvent::new(&*old, &player).unwrap());

let old = player.clone_value();
player.consume_mana(35);

history.push(HistoryEvent::new(&*old, &player).unwrap());

history.undo(&mut player).unwrap();

assert_eq!(
    player,
    Player {
        health: 75,
        mana: 100
    }
);

history.undo(&mut player).unwrap();

assert_eq!(
    player,
    Player {
        health: 100,
        mana: 100
    }
);

history.redo(&mut player).unwrap();

assert_eq!(
    player,
    Player {
        health: 75,
        mana: 100
    }
);

Open Questions

Currently, Diff is required to contain the 'old lifetime due to the EntryDiff::Deleted variant:

/// An entry with the given key was removed.
Deleted(ValueDiff<'old>),

We could consider storing an owned value instead of a borrowed one, but this raises two problems. First, it eagerly clones data we might not want (some applications may just want to examine a diff rather than apply it). Second, there are currently some issues regarding dynamic types and hashing (see #6601). By eagerly cloning this data, we risk users hitting that issue more often. Although, this hashing problem will already be experienced if users call clone_diff before applying the diff.

Should we go ahead and convert it to an owned value to get rid of the 'old requirement?

Future Work

This PR sets the groundwork for more diffing work.

Specifically, I want to make these diffs serializable. This will make it much easier to compactly serialize and send these diffs over the network or even between processes.

I originally had that work done, but probably due to a git stash malfunction, I can't seem to find it. If this PR gets general consensus among the community, I'll restart work on that front as well.

Testing

To test, you can run the tests for the bevy_reflect crate:

cargo test --package bevy_reflect

Changelog

TL;DR

  • Added reflection-based diffing

Added

  • Reflect::diff trait method
  • Reflect::apply_diff trait method (with default implementation)
  • Array::apply_array_diff trait method
  • Enum::apply_enum_diff trait method
  • List::apply_list_diff trait method
  • Map::apply_map_diff trait method
  • Struct::apply_struct_diff trait method
  • Tuple::apply_tuple_diff trait method
  • TupleStruct::apply_tuple_struct_diff trait method
  • diff module
    • ArrayDiff struct
    • DiffApplyError enum
    • DiffApplyResult type alias
    • DiffError enum
    • DiffResult type alias
    • DiffType enum
    • Diff enum
    • ElementDiff enum
    • EntryDiff enum
    • EnumDiff enum
    • ListDiff struct
    • MapDiff struct
    • StructDiff struct
    • TupleDiff struct
    • TupleStructDiff struct
    • ValueDiff enum
    • diff_array function
    • diff_enum function
    • diff_list function
    • diff_map function
    • diff_struct function
    • diff_tuple_struct function
    • diff_tuple function
    • diff_value function

Migration Guide

Manual implementors of Reflect will need to add an implementation for the new Reflect::diff method.

Additionally, they may need to add an implementation for the relevant subtrait method:

  • Array::apply_array_diff
  • Enum::apply_enum_diff
  • List::apply_list_diff
  • Map::apply_map_diff
  • Struct::apply_struct_diff
  • Tuple::apply_tuple_diff
  • TupleStruct::apply_tuple_struct_diff

Footnotes

  1. Recall that reflection is not perfect at detecting differences. Fields could be ignored which might make it seem like two values are the same when in actuality they are not. This is something users of reflection should expect, but it's important as a reminder nonetheless.

@MrGVSV MrGVSV added C-Enhancement A new feature A-Reflection Runtime information about types labels May 6, 2024
@MrGVSV MrGVSV added D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Enhancement A new feature D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Reflect: helpers for "diffing" dynamic structs
1 participant