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

Parent -> ChildOf, Children -> ParentOf #17427

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

cart
Copy link
Member

@cart cart commented Jan 19, 2025

Fixes #17412

Objective

Parent and Children use the "has a X" naming convention. There is increasing sentiment that we should use the "is a X" naming convention for components (especially relationships, following #17398).

This is embraces that by doing the following:

  1. Rename Parent to ChildOf
  2. Rename Children to ParentOf

@cart's Thoughts

  • I like that the XOf and XBy naming convention helps indicate that a component is a relationship. We don't see these names much elsewhere in the wild, so it is a reasonably strong indicator that a given component is a relationship. And it helps resolve the ambiguity of Parent (is it a parent, or does it have a parent?)
  • I think this does reduce the legibility of things, especially on the RelationshipTarget side (ex: Children vs ParentOf). There are a lot of use cases where the variable name would really benefit from being children and not parent_of. Iterating or indexing a ParentOf is harder to mentally wrap my head around compared to iterating Children. Having seen the effects on the code using these APIs, I'm kind of biased toward using the plural / "has a" naming convention (Children), while still moving to the ChildOf convention for actual Relationships. Curious what yall think. I'm still not sure where I land.

This is just the implementation PR. To discuss the path forward, do so in #17412.

@cart cart added the A-ECS Entities, components, systems, and events label Jan 19, 2025
@cart cart added this to the 0.16 milestone Jan 19, 2025
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 19, 2025
pub struct Children(Vec<Entity>);
pub struct ParentOf(Vec<Entity>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer Children. Much like I think TeacherOf should probably be Students and EmployerOf should be Employees. It doesn't fit every case, but where one "owns" another and there's a good plural form of it, I feel like that kind of naming works.

The whole "is a" vs "has a" is confusing anyways since you'd say "this entity has Health" not "this entity is a Health" (but not so for things like Player).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes down to how much everyone wants to stick with a central, un-lintable naming scheme. It is possible to adhere to IsA, for example a Health component would be something like Mortal and/or Destructible instead.

#[cfg_attr(feature = "bevy_reflect", derive(bevy_reflect::Reflect))]
#[cfg_attr(
feature = "bevy_reflect",
reflect(Component, MapEntities, VisitEntities, VisitEntitiesMut, FromWorld)
)]
pub struct Children(Vec<Entity>);
pub struct ParentOf(Vec<Entity>);
Copy link
Member

@MrGVSV MrGVSV Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively I like XTo as well. And it may be possible to use that sort of naming convention to convey certain meaning such as ownership or directionality (or sources vs targets).

But yeah this might just be my own personal preference haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Parent to ChildOf and Children to ParentOf
4 participants