Remote entity reservation v2 #18266
Remote entity reservation v2 #18266
Conversation
Allocation order is not guaranteed. This test did not reflect that.
flushing is no longer needed for allocation, freeing, etc, so there is no such thing as `needs_flush`, and `worth_flushing` can be true even just by allocating directly.
alice-i-cecile
added
A-ECS
| @@ -50,6 +50,9 @@ impl ArchetypeRow { | |||
| /// Index indicating an invalid archetype row. | |||
| /// This is meant to be used as a placeholder. | |||
| pub const INVALID: ArchetypeRow = ArchetypeRow(u32::MAX); | |||
| /// This is the same as [`Self::INVALID`], but it signals to [`Entities`](crate::entity::Entities) | |||
There was a problem hiding this comment.
I'd like to see this expanded to explain why it exists.
There was a problem hiding this comment.
Basically, when we reserve Entitys from EntityReservations for Entities::owned, we don't want to flush them like normal. They haven't been requested yet, so the flush doesn't make sense, and when they are requested by Entities::alloc, the caller will be the one to Entities::set the EntityLocation.
In principal, we could just have a HashSet of Entitys that have been in Entities::owned since the last flush, but that lookup takes time and memory.
Instead, by keeping a special EntityLocation::INVALID_BUT_DONT_FLUSH, we can determine whether or not to flush based only on if meta.location == EntityLocation::INVALID or not.
Hopefully that makes sense. This is complicated.
| } | ||
| } | ||
|
|
||
| /// This allows entity reservation. |
There was a problem hiding this comment.
Remember to beef these up a bit once we have consensus on the basic mechanism.
There was a problem hiding this comment.
Absolutely. Just don't want to write full docs until we settle on a design. 🥲
This was the big piece of the puzzle I was missing.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
entity allocation order is not guaranteed.
| /// # Dropping | ||
| /// | ||
| /// All reserved entities will continue to be reserved after dropping, *including those that were not iterated*. | ||
| /// Dropping this without finishing the iterator is effectively leaking an entity. |
There was a problem hiding this comment.
How bad is "leaking an entity"? Leaking memory is pretty bad but maybe leaking an index is less so?
There was a problem hiding this comment.
It's not that bad at all, and it isn't done anywhere in Bevy that I know of. The docs here were just incomplete. This is irrelevant to the rest of these changes. I just figured, while I'm here...
There was a problem hiding this comment.
ah okay I gotcha, thank you!
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
This gets rid of that pesky linear search.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
This lets us continue to check all entities directly.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
we can't free twice!
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
1 similar comment
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
entity ordering can not be relied on.
Index was 0 previously but is now 255
the order and index of the entities had changed.
…iottjPierce/bevy into remote-entity-reservation-v2
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
|
This seems to have a breaking change with testbed ui's text wrap. See here. I tested it locally and the text seems to not be rendering (aside from its background) unless the mouse is hovering over it. When the mouse isn't hovering over a text item, only the most recently hovered item is rendered. There's no meaningful logs either. I have no knowledge of the internals of the text system, and I have no idea why this is broken with this PR. It seems like this is actually an existing bug that this PR just uncovered, but IDK. AFAIK the text system doesn't depend on observer ordering or particularly ordered queries, so I can't think of a reason this would break. |
|
@alice-i-cecile I apologize for the misdirection, but I actually don't think this needs a migration guide. I was almost certain it did before, but now that the dust has settled, I don't see any breaking changes. Let me know if I'm missing something. |
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Yep revealed bug, the sort key for the transparent UI phase uses a pair of the stack index and the render entities index. I guess the render entity index was intended to break ties but it's not needed as the sort is stable and if the entity indices aren't sequential it messes up the draw order so the text in the example gets drawn beneath the colored background panels. |
fixes #18003
Objective
Long story short: we need to be able to reserve entities from outside
&World.This is an alternative to #18195.
Solution
Thanks to everyone who helped brainstorm on discord. There's still ideas being tossed around, but here's how this PR works:
All entity meta is kept in a single unlocked
Vec<EntityMeta>. We keep a pending list that can be used remotely to reserve entities, etc.When we allocate an entity directly, we pop from the new
owned: Vec<Entity>, and if it's empty, we reserve some more entities in bulk. These entities are not flushed.To ensure we don't loose entities anywhere, flushing rebalances
pendingandowned. We can tweak the proportions later, but currently, it splits the pending entities between the two evenly.Practical Change
It used to be that a brand new
Entitieswould yield sequential indices viaalloc. This was never a guarantee, but many tests relied on this ordering. The ordering is still deterministic but is no longer sequential or aligned. In other words, if you allocate 5 entities. You might get back indices[0, 4, 2, 6, 7]. Generally entities like this are opaque and this should not affect users. However, tests and benchmarks that depend on this previous, not guaranteed behavior had to be reworked.One practical effect this may have is that systems that run through
Entitys sequentially, will have a different order. This may affect query orders, observer ordering, etc. None of these things were guarantees before, but this may expose some existing bugs in those systems.I suspect this is why some rendering systems broke.
Future Work
Flushing is now completely unnecessary for many of the interactions with [
Entities]. We still need to do it before applying command queues, etc, but we don't need to flush before allocations or freeing. If we go in this direction, we should explore relaxing those requirements for performance.Testing
Functionality hasn't changed, so no new tests were added. A few old tests were revised because of the practical changes.
However, it makes sense to add tests for remote registration, so if anyone has specific concerns, I'd be happy to write a test for it.