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

Reduce implicit defaults by requiring more components #504

Open
Jondolf opened this issue Aug 30, 2024 · 0 comments
Open

Reduce implicit defaults by requiring more components #504

Jondolf opened this issue Aug 30, 2024 · 0 comments
Labels
C-Code-Quality Improvements to code readability, maintainability, or best practices C-Enhancement New feature or request X-Contentious There are nontrivial implications that should be thought through

Comments

@Jondolf
Copy link
Owner

Jondolf commented Aug 30, 2024

For Bevy 0.15, which will have required components.

Objective

Currently, Avian has implied defaults for numerous components. This means that when a component doesn't exist on an entity, a default value or behavior is used in its place. Examples include:

  • CollisionLayers
  • Friction
  • Restitution
  • Dominance
  • GravityScale
  • SpeculativeMargin

and probably many more.

It seems like these kinds of implied default values for components should generally be discouraged. Quoting Cart (the Project Lead of Bevy) on Discord, in the context of UI:

I would (personally) like to move away from "implied default values":

  • It makes it harder to reason about what those are, as they often don't live in / near the struct, but rather in a system somewhere
    • This also makes it possible to have different implied default values, as each system can choose how to interpret it
  • It forces archetype moves when changing to a non-default value
  • It makes it hard to query for things (ex: give me everything with a transparent background) because now you need to check both Option<BackgroundColor> and BackgroundColor(Color::NONE), and it means you need to know to do both.
  • It makes it harder to discover functionality, as the "optional" component is now floating / decoupled

along with:

In general I think if you ask yourself "does this Component have a X", then it should have the component. For example, the answer to the question "does this Node have a z index?" seems to be pretty clearly "yes it has a local z index of 0", so it should have a ZIndex component to reflect that.

This was initially in the context of UI, so I asked if this should also be considered a "best practice" that the ecosystem as a whole should follow (along with some component examples), to which the response was:

Imo in pretty much all of these cases, if you ask yourself the question "does this entity have this property", the answer is "yes". For the reasons stated above, I think they should be required.

In summary, it seems like there is quite a clear preference (from Cart) towards not having implied default values for components. In practice, this would just mean having more components automatically inserted via the required components functionality coming in Bevy 0.15.

It didn't seem like there was full consensus on this in general though, and it's not entirely clear which components count as being a "property" of an entity. I'm personally not fully sold on requiring all of the components listed earlier in cases where the component is mostly intended for opting out of default behavior, or just otherwise very niche (e.g. GravityScale and SpeculativeMargin), but we can probably decide on a case-by-case basis.

Pros

  • More explicit.
  • Better discoverability.
  • Less room for error, as the default is the same across systems.
  • Queries can just query for e.g. every CollisionLayers. No need for Option<&CollisionLayers> with a default fallback.
  • Less archetype moves.
  • If we require the components to exist and eliminate implied defaults, we can get rid of Option<T> in many queries, reducing branching.

Cons

  • Memory usage is increased, as entities have more "redundant" data stored.
  • Imo this kind of goes against the nature of an ECS. In general, in my mind, has component => enables or configures behavior, and lack of component => lack of custom behavior (but may still have sane default behavior assumed by other related components or the surrounding context).
  • Potentially more unnecessary iteration and work. For queries with Option<T>, getting rid of implied defaults is good, but what about normal queries with T? Previously, queries skipped entities without T, but now T is added for all entities that have components that require it, so queries iterate over all of them.
  • I'm not sure if I'd still be comfortable not handling the cases where required components are missing. Users/plugins could still remove the components after insertion.
@Jondolf Jondolf added C-Enhancement New feature or request X-Controversial There is active debate or serious implications around merging this PR C-Code-Quality Improvements to code readability, maintainability, or best practices X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality Improvements to code readability, maintainability, or best practices C-Enhancement New feature or request X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

1 participant