-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add basic system for specifying and validating archetype invariants #5121
Add basic system for specifying and validating archetype invariants #5121
Conversation
This will not actually impact system parallelization; this is done on the basis of factual rather than hypothetical compatibility by checking the bitset of created archetypes. |
5103468
to
34d5714
Compare
I recognize that this action would increase the cost of adding a new archetype. Are archetypes cached so that they aren't immediatly deleted when there are no entities? I'm concerned of the case where an entity with a unique archetype that has a marker component that rapidly gets added and removed over time. This would cause creating that archetype and the archetype without the marker many times over a short time frame if there is no caching. Also would it be a good idea to disable archetype invariant checks on release builds? |
In short, yes. The world's archetype list stores all archetypes, past and present. This means that your "flickering marker" example would only incur two archetype checks :).
I considered this, but since this is planned to be used for soundness (with system parameters), I think it should always be on. That being said, I am open to including a setting to disable it in release mode, depending on how that system parameter integration ends up looking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR overall! This is a very good addition to the ECS. And I'm so surprised that the implementation is so simple!
Sorry for the amount of comments 😢 They are mostly punctuation corrections though.
Implementation
What's the reasoning behind single-component bundles “having” to use AllOf
instead of AtLeastOneOf
? Any future proofing when the bundle may grow to include more components?
Is it possible or desirable to have a “no other than” statement? It would be false only if the archetype contains components that are not in the bundle. In set terminology, the archetype must be a (non-strict) subset of the bundle. It would make possible a less restrictive version of ArchetypeInvariant::full_bundle
. For example if I have:
ArchetypeInvariant {
predicate: ArchetypeStatement::<BundleA>::at_least_one_of(),
consequence: ArchetypeStatement::<BundleA>::no_other_than(),
}
it would not allow archetypes with at least one component outside of BundleA
, given that it contains at least one component from BundleA
.
ArchetypeInvariant
constructor ideas
- subset:
AtLeastOneOf<B>
→NoOtherThan<B>
- disjunction:
AtLeastOneOf<B1>
→NoneOf<B2>
(This is useful for avoiding query conflicts) - intersection:
AtLeastOneOf<B1>
→AtLeastOneOf<B2>
Other basic set operations may yield other invariants, but I don't know if they are all useful.
Docs
Nice use of doc comments! I also advise adding an example later in development to showcase the feature.
Changes in component.rs
seems irrelevant to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @Nilirad! No worries about the large number of comments; you had a lot of great suggestions!
What's the reasoning behind single-component bundles “having” to use
AllOf
instead ofAtLeastOneOf
?
The original idea here is that, for any simplified logical statement, there would be a single, canonical way to represent it in the archetype invariant system. Since both AllOf
and AtLeastOneOf
are equivalent for single-component bundles, we wanted to provide a convention for users to follow.
Is it possible or desirable to have a “no other than” statement?
I think this is a great addition, but I decided to call it Only
, since I think that's a bit more clear. Added as of 49a0c26!
ArchetypeInvariant
constructor ideas
I really like these! I'll make sure to add them soon :). Although for intersection
, I think using AllOf
for both sides of the invariant is a more common case.
Nice use of doc comments! I also advise adding an example later in development to showcase the feature.
Yep, this is planned :). (See the todo section in the PR description)
Changes in component.rs seems irrelevant to this PR.
They kind of are. I decided to improve the Component
docs while I was messing around in there. Where I come from, these kind of small changes are standard practice, but I can move those docstring updates to their own PR if that's desired.
I agree with most of your doc comments, and fixed them in commits 5ba438b and 288a786. For the few I disagreed with, I left comments below.
I think that the lines “This may include empty archetypes: archetypes that contain no entities.” can be kept for now, so you can resolve them for now. Be sure to double-check that all methods that states that actually triggers the re-check though, maybe by adding some tests. I think that changes to |
Just added these tests on my end, and they fail: I added a failing condition for the archetype #[test]
#[should_panic]
fn re_check_all_archetypes_on_invariant_insertion() {
let mut world = World::new();
world.spawn().insert_bundle((A,));
let untyped_archetype_invariant = super::ArchetypeInvariant {
predicate: super::ArchetypeStatement::<(A,)>::all_of(),
consequence: super::ArchetypeStatement::<(A,)>::none_of(),
}
.into_untyped(&mut world);
world.archetype_invariants.add(untyped_archetype_invariant);
}
#[test]
#[should_panic]
fn re_check_all_archetypes_on_untyped_invariant_insertion() {
let mut world = World::new();
world.spawn().insert_bundle((A,));
let untyped_archetype_invariant = super::ArchetypeInvariant {
predicate: super::ArchetypeStatement::<(A,)>::all_of(),
consequence: super::ArchetypeStatement::<(A,)>::none_of(),
}
.into_untyped(&mut world);
world.add_untyped_archetype_invariant(untyped_archetype_invariant);
} So either the doc comment about re-checking in |
Wondering if we can use const generics to generalize “at least” and “at most” statements: pub enum ArchetypeStatement<B: Bundle, N: usize> {
// ...
AtLeast(PhantomData<B>, PhantomData<N>),
AtMost(PhantomData<B>, PhantomData<N>),
} Something like that, I don't know how the exact syntax would be. |
That's because you're calling add on the World's |
We could do that, but I don't think it's helpful to add at this stage. I actually considered doing this, but I think that the increased complexity isn't worth the potential gain, especially because it would make the common case less readable. It would be very cool, but I can't think of many cases where you'd actually want or need that kind of arbitrary checking. I would be open to adding this later if it's a requested pattern though. |
2363ad0
to
0f17607
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc fixes and a couple of design thoughts.
…tils` (#5174) # Summary This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by #4299 and #5121. As a result: - This method was moved to `bevy_utils` for easier reuse. - The legibility and robustness of this method has been significantly improved. - Harder test cases have been added. This change was split out of #4299 to unblock it and make merging / reviewing the rest of those changes easier. ## Changelog - added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display. - removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
I spent some time exploring a trait-based API with @alice-i-cecile, but we came to the conclusion that it would likely cause more problems than it solves. See the PR description for a full justification |
Nilirad is right. This doesn't belong here, it belongs on the `Archetype` docs
The new API is equivalent, but it has a nicer syntax, especially for binary invariants. It may not seem like much of an improvement now, but it will get much cleaner once "components as bundles" is merged.
bors try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.add_archetype_invariant(<(Player,)>::forbids::<(Camera,)>())
I really like the look of the new API (although, of course, it will be much nicer after we can remove those brackets). Very legible.
…tils` (bevyengine#5174) # Summary This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121. As a result: - This method was moved to `bevy_utils` for easier reuse. - The legibility and robustness of this method has been significantly improved. - Harder test cases have been added. This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier. ## Changelog - added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display. - removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done another round of review, and with components-as-bundles merged this is ready to go.
The one-at-a-time component addition can panicking is unfortunate, but a) this is a performance footgun anyways b) this PR is already plenty big and c) archetype invariants are fully opt-in. I think it's desirable to fix anyways, but shouldn't block a merge here.
Added to Cart's Attention Board.
I don't think this would close #1481. Of the three use cases mentioned there (helping system ambiguity checker, checking assumptions, allowing overlapping queries), it would only close the second one. |
That's a good point. Personally, I think #1481 should still be closed, and those two tasks should be split into two newer issues, now that so much has changed. I don't feel too strongly though. I'll remove the closing language from the PR description for now. |
…tils` (bevyengine#5174) # Summary This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121. As a result: - This method was moved to `bevy_utils` for easier reuse. - The legibility and robustness of this method has been significantly improved. - Harder test cases have been added. This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier. ## Changelog - added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display. - removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
…tils` (bevyengine#5174) # Summary This method strips a long type name like `bevy::render::camera::PerspectiveCameraBundle` down into the bare type name (`PerspectiveCameraBundle`). This is generally useful utility method, needed by bevyengine#4299 and bevyengine#5121. As a result: - This method was moved to `bevy_utils` for easier reuse. - The legibility and robustness of this method has been significantly improved. - Harder test cases have been added. This change was split out of bevyengine#4299 to unblock it and make merging / reviewing the rest of those changes easier. ## Changelog - added `bevy_utils::get_short_name`, which strips the path from a type name for convenient display. - removed the `TypeRegistry::get_short_name` method. Use the function in `bevy_utils` instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the example for this, very nicely documented!
/// | ||
/// Whenever a new archetype invariant is added to a world, all existing archetypes are re-checked. | ||
/// This may include empty archetypes- archetypes that contain no entities. | ||
/// Prefer [`add_archetype_invariant`](App::add_archetype_invariant) where possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth it to add why this is preferred
/// | ||
/// Whenever a new archetype invariant is added, all existing archetypes are re-checked. | ||
/// This may include empty archetypes- archetypes that contain no entities. | ||
/// Prefer [`add_archetype_invariant`](World::add_archetype_invariant) where possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it might be beneficial to add why this is the case
for archetype in &self.archetypes.archetypes { | ||
let archetype_components = archetype.components.indices(); | ||
untyped_invariant.test_archetype(archetype_components, self.components()); | ||
} | ||
|
||
self.archetype_invariants.add(untyped_invariant); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work?
for archetype in &self.archetypes.archetypes { | |
let archetype_components = archetype.components.indices(); | |
untyped_invariant.test_archetype(archetype_components, self.components()); | |
} | |
self.archetype_invariants.add(untyped_invariant); | |
self.add_untyped_archetype_invariant(untyped_invariant); |
/// The generic [`Bundle`] type `B1` is always used in the `premise`, | ||
/// while `B2` is used in the `consequence`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you feel about renaming the generics to Premise
and Consequence
?
While I reviewed the other code, I was first confused why the archtypes have bundles, but now it makes sense.
Renaming them could make it easier to understand the code, at the cost of having longer names (and maybe making it less obvious that they are generic parameters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the idea of renaming these.
/// Archetypes are only modified when a novel archetype (set of components) is seen for the first time; | ||
/// swapping between existing archetypes will not trigger these checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "checked" instead of "modified"?
} | ||
} | ||
|
||
/// Defines helper methods to eaily contruct common archetype invariants. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (crumbs in the S
key? 😀)
/// Defines helper methods to eaily contruct common archetype invariants. | |
/// Defines helper methods to easily construct common archetype invariants. |
/// A special module used to prevent [`ArchetypeInvariantHelpers`] from being implemented by any other module. | ||
/// | ||
/// For more information, see: <https://rust-lang.github.io/api-guidelines/future-proofing.html>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth it to link to the Restrictions RFC here, which could (as far as I understand it) replace this pattern once implemented.
Closing this out. I still want this at some point, but there is work to be done building consensus, defining error handling patterns and so on. This is also going to be way easier to implement with hooks these days. The end user API is still great though, and should be borrowed from aggressively. |
Objective
Closes point 2 of #1481
Impulse
Often, when adding components and bundles to entities, there are implicit rules about which components can or can't appear with each other. For example, in most projects, an entity with a
Player
component should never have aCamera
component. Users would like to be able to specify these rules (invariants) more explicitly, both for static analysis, and soundness.Current Way
If a user wants to codify these rules, they must roll their own implementation, mostly likely by adding a System which checks each of these rules against the
World
'sArchetypes
.Struggle
There are several difficulties with the current way:
Without
clauses in systems with multiple queries, and prevent false positives when checking system order ambiguities.)New Way
We will add a first-class system for specifying and validating archetype invariants:
ArchetypeInvariant
, which represents a structured logical statement that must hold true for allArchetype
sArchetypeInvariant
is invalidated, the engine panics, and outputs a nice error message:'Archetype invariant violated! The following invariants were violated for archetype ["B", "A"]: {Premise: AnyOf(A), Consequence: NoneOf(C, B)}'
ArchetypeInvariants
on theWorld
.App
Archetype
is added to theWorld
, we will check it against the existing invariantsChangelog
ArchetypeStatment
, and associated methods for construction and testing archetypesArchetypeInvariant
, and associated methods for construction and testing archetypesimpl
forBundle
with helper methods for constructing common invariantsUntypedArchetypeStatement
andUntypedArchetypeInvariant
, to act as a type-erased version of the above. (See design justifications below for details.)ArchetypeInvariants
, to store a list ofArchetypeInvariant
s.World.archetype_invariants: ArchetypeInvariants
, to store archetype invariantsWorld::add_archetype_invariant
andWorld::add_untyped_archetype_invariant
, to add new invariants to a worldApp::add_archetype_invariant
andApp::add_untyped_archetype_invariant
, to add new invariants to a app's world.archetype_invariants
example.Archetype::new
to check against the list of invariants from theWorld
before returning the constructed archetypeDesign Justifications
ArchetypeInvariant
) and untyped (UntypedArchetypeInvariant
) API. The typed API allows for very convenient specification of invariants that use only Rust types. The untyped API is required to support dynamic components.World
, as different worlds may have wildly different archetypes and rules. Additionally, this provides more natural support for headless worlds.Archetype::new
is called ensures that all commands and methods (including future ones) will include invariant validation.insert_bundle
instead ofinsert
will likely solve the majority of these cases.Trait-based API
We use an explicit type for archetype invariants instead of a trait, as using a trait would cause problems when attempting to use archetype invariants to resolve system parameter conflicts (and execution order ambiguities, which follow similar logic). Reasoning:
test
method that takes an archetype and outputs a boolean for whether it is valid. It is natural to want to use this to catch disjoint system parameters that overlap, such asQuery<&Transform, With<Player>>
andQuery<&mut Transform, With<Camera>>
. One might want to just take the union of the two queries to construct a minimal archetype and check them against the invariant. Then, if the invariant fails, we know the archetype will never appear and the queries are disjoint, right? Unfortunately, no.Player::requires::<Life>()
. These queries, when combined, would fail the invariant (sinceLife
is not present), making the queries look disjoint when they are not necessarily.unsafe
.This means that the trait approach locks us into one of two options:
unsafe
. This means that users would need to either use only the provided helper types, or manually implement an unsafe trait.Option 1 is unappealing for fairly obvious reasons. Although we may be able to provide helper types for all reasonable invariants, I doubt this is likely to happen. Furthermore, I'd rather avoid manual implementation of unsafe traits entirely if possible.
Option 2 is more palatable but restricts important use cases in ways that will surprise users and leave frustrating edges in the engine. Debatably-prettier APIs aren't worth it.
Future Work
ArchetypeStatment
s andArchetypeInvariant
s. We decided that this optimization is premature for this PR.ArchetypeStatement
that allows the user to provide an arbitrary function. This should not be used commonly, but may be helpful as a safety valve when working with strange or uncommon invariants.