Support rolling back resources #622
Conversation
nick-e
force-pushed
the
resource-rollback
branch
from
659c376 to
f04e8d7
Compare
lightyear/src/utils/ready_buffer.rs
Outdated
| /// Adds an item to the heap marked by time | ||
| pub fn push(&mut self, key: K, item: T) { | ||
| self.heap.push(ItemWithReadyKey { key, item }); | ||
| } | ||
|
|
||
| /// Returns a reference to the item with the highest `K` value or None if the queue is empty. | ||
| /// Does not remove the item from the queue. | ||
| pub fn peek_latest_item(&self) -> Option<(K, &T)> { |
There was a problem hiding this comment.
Wouldn't this return the item with the lowest K value?
Can we add a unit test for it?
There was a problem hiding this comment.
According to the docs, std::collections::BinaryHeap is a max-heap: https://doc.rust-lang.org/std/collections/struct.BinaryHeap.html#method.peek
I've added a unit test for the new function.
There was a problem hiding this comment.
Yes but I inverted the Ord order of ItemWithReadyKey to make it a min-heap, or I thought..
I'll have to take a closer look after merging
| fn eq(&self, other: &Self) -> bool { | ||
| self.item == other.item && self.key == other.key | ||
| self.key == other.key |
There was a problem hiding this comment.
But what if we compare we have two component histories with values at the same ticks but the values are different?
Then we would get ready_buffer_1 == ready_buffer_2?
There was a problem hiding this comment.
ItemWithReadyKey implements PartialEq so that it can implement Ord so that it can be used in a BinaryHeap within ReadyBuffer. ReadyBuffer's comments describe its functions as dependent on the order of the key (e.g. a tick or instant) and does not mention being dependent on the order of the item's value.
ReadyBuffer doesn't implement PartialEq so ready_buffer_1 == ready_buffer_2 doesn't compile but if it did then the equality would describe whether or not the two buffers have the same keys.
I don't think it would make sense for a ReadyBuffer to have multiple items at the same key. That could result in a scenario where a component has two values at a given tick.
This also allows rollback for resources that don't implement PartialEq.
|
This looks really good overall! I just left a couple comments |
nick-e
force-pushed
the
resource-rollback
branch
from
f04e8d7 to
392d62a
Compare
nick-e
force-pushed
the
resource-rollback
branch
from
392d62a to
05dd459
Compare
|
Thanks! the changes look great |
|
Coming back to this, a EDIT: actually that's not relevant because this is PR is for non-networked resources, so we are not receiving server updates for them |
Support rollback for resources. Make the ReadyBuffer not require its item to implement PartialEq. ReadyBuffer should only have one item per tick and so only the tick needs to be compared and not the item. Call
App::add_resource_rollback<R>()to add rollback support for a resource.Fixes #585