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

Reflect Diff-ing #944

Closed
wants to merge 1 commit into from
Closed

Reflect Diff-ing #944

wants to merge 1 commit into from

Conversation

cart
Copy link
Member

@cart cart commented Nov 28, 2020

This enables "diff-ing" Reflect values. The general idea is that you can do:

let a = Foo { x: 1, y: 2 };
let b = Foo { x: 1, y: 3 };

let diff = a.diff(&b).unwrap();

// diff will be None if they are the same

if let Some(diff) = diff {
  // `diff` is a DynamicStruct that contains { y: 3 }
} else {
  // diff will be None if `a` and `b` are the same
}

A "rule" that should always hold true for diffs is: for a given let diff = a.diff(&b), calling a.apply(&diff) should produce a value equivalent to b

Because of this, this implementation currently has one big caveat: we only return a real "diff" for Struct types. Every other type is treated as a "value type". They will return the "full" value if there is any mismatch.

  • Map: we can't store Map diffs in DynamicMap because it doesn't encode "removed values". If there is any difference, the diff will contain the "full" Map value.
    • The "rule" above doesn't apply to maps yet because we don't clear the map when we call apply.
  • List: we can't store List diffs in DynamicList because it doesn't encode "removed values" or allow for sparse indexing. The diff will contain the "full" List value.
    • The "rule" above doesn't apply to lists yet because we don't clear the list when we call apply.
  • TupleStruct: DynamicTupleStruct doesn't support sparse indexing so we must include every field.

I'm considering a couple of options for the cases above:

  1. just embrace "diff everything else as a value type" (ex: change apply() to clear collection values)
  2. Return new "diff types" that encode the necessary information. This increases the api surface, but also gives us more flexibility.

@cart cart added C-Enhancement A new feature A-Core Common functionality for all bevy apps labels Nov 28, 2020
@cart
Copy link
Member Author

cart commented Nov 28, 2020

(2) feels "better" to me, but it does also significantly complicate the serializer / deserializer situation because they need to know how to deserialize diffs (and distinguish them from other types, while still encoding the original type)

@jbarthelmes
Copy link

In (1) you could also distinguish two modes of operation like merge and overwrite.

Option (2) is more powerful but also harder (esp. for lists).
For maps at least the MapDiff type should be equivalent to Map<K, ElementDiff<V>> where

enum ElementDiff<T> {
  Added(T),
  Removed,
  Updated(T),
  // NoDifference, // maybe for completeness
}

I think diffing lists generally has a hard computational complexity. The naive algorithm would be to treat lists as Map<Idx, T> and proceed as with maps. As you can imagine that would produce horrible diffs for a simple insert at index 0. (Approach A)

You can also represent the ListDiff as a sequence of update instructions. I think that is what serde-diff does for lists and other types as well. (Approach B)

As a combination of the two approaches, I'm imagining a greedy algorithm that finds maximal sub-lists where it can use Approach A and cuts them out into nodes of a Rope-like tree. On the parent-level Rope it would use Approach B. These two parts would work in mutual recursion. The resulting ListDiff is a tree. This idea is probably not novel and there is literature on generic binary diffing that you can use for reference.

As for tuples, it depends on the size of the data whether treating them as maps or opaquely (or hybrid) is more efficient. I don't know enough rust but I imagine if "sizeof" is available in the deriving macro you could even do optimized code generation.

@cart
Copy link
Member Author

cart commented Dec 3, 2020

I'm going to hold off on wrapping this up / merging it until I start working on nested scenes (which was the primary motivator for this feature). That was what I was planning on doing next, but I think my focus is better spent elsewhere. Nested scenes really start getting valuable / desirable when we have an editor, so I'd rather build features like this within the context of a real bevy editor / let that inform the design.

Base automatically changed from master to main February 19, 2021 20:44
@alice-i-cecile alice-i-cecile added A-Reflection Runtime information about types and removed A-Core Common functionality for all bevy apps labels Apr 14, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@DJMcNab DJMcNab removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Aug 7, 2021
@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Sep 23, 2021
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
@cart
Copy link
Member Author

cart commented Jun 16, 2022

closing this as its not particularly novel, the changes are small, and the conflicts are big enough that adapting this would be hard. If we decide we need this, starting from scratch is probably the move.

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 S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants