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

Rename Parent to ChildOf and Children to ParentOf #17412

Closed
alice-i-cecile opened this issue Jan 17, 2025 · 20 comments · Fixed by #17427
Closed

Rename Parent to ChildOf and Children to ParentOf #17412

alice-i-cecile opened this issue Jan 17, 2025 · 20 comments · Fixed by #17427
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Milestone

Comments

@alice-i-cecile
Copy link
Member

When I started learning how to use Bevy recently, this was the first stumbling block. It seemed straightforward that Parent was a marker component (or "driving concept") for a parent entity!

Originally posted by @physgun in #17398 (comment)

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Hierarchy C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-SME Decision or review from an SME is required and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 17, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 17, 2025
@alice-i-cecile alice-i-cecile changed the title Rename Parent to ChildOf. Rename Parent to ChildOf and Children to ParentOf Jan 17, 2025
@alice-i-cecile
Copy link
Member Author

This is the IsA vs HasA terminology debate, to borrow some terms from flecs.

@alice-i-cecile
Copy link
Member Author

When doing this, we should be sure to keep the old names around as deprecated type aliases for a cycle.

@viridia
Copy link
Contributor

viridia commented Jan 17, 2025

I proposed in #17398 (comment) that each relationship actually have three names:

  • The component in the 'from' entity (the one in the one-to-many relationship), e.g. ParentOf (formerly Children).
  • The component in the 'to' entity, e.g. ChildOf (formerly Parent).
  • The name of the builder / constructor used to spawn a new entity that is related to the current entity.

So for example, something like:

Relation type From Component To Component Constructor
children ParentOf ChildOf Children
observer ObservedBy ObserverOf Observers
ownership OwnerOf OwnedBy (none, construction is always implicit)

I didn't specify names for reactions because IMHO reactions don't need a special relation type: ownership is sufficient.

@cart
Copy link
Member

cart commented Jan 17, 2025

I would prefer it if we didn't need to build special-cased constructors for these things:

  1. I don't love having two names for the same concept.
  2. I'd like these APIs to be relationship-type-driven, especially given that Relationships are a "generic user-facing system". Ideally they define both sides of the relationship and can immediately start spawning things in the same way Bevy's built-in relationships do it.

Ex: If ParentOf is the component name, then it should also be the thing that we use during spawn. ParentOf is actually reasonable in this context:

world.spawn((
    Foo,
    ParentOf::spawn((
        Spawn(Bar),
        Spawn(Baz),
    )),
))

world.spawn((
    Foo,
    parent_of![
        Bar,
        Baz,
    ]
));

bsn! {(
    Foo,
    ParentOf [
        Bar,
        Baz,
    ]
)}

(note that my current plan for BSN and ParentOf is to make [ ] imply ParentOf [])

If we do want to refer to the "collection of childof relationships" as "children" in this context, then we should violate our "IsA" constraint and just give it the name that makes us happy. Needing to connect ParentOf to a new "children" concept name is confusing and needlessly complicated. I don't think losing IsA here is a particular loss. It wouldn't be the first or the last case where "IsA" naming doesn't quite fit (as mentioned above).

@cart
Copy link
Member

cart commented Jan 19, 2025

Just put out the rename PR. I left some more thoughts there (but lets discuss here):

  • 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.

@viridia
Copy link
Contributor

viridia commented Jan 19, 2025

One problem with depending on the "Of" and "By" suffixes is that they can point in opposite directions depending on the verb chosen:

  • The "observe" verb points upwards (sub-entities observe super-entities), so "ObservedBy" points from one to many.
  • The "own" verb points downwards (super-entities own sub-entities), so "OwnedBy" points from many to one.

This means you can't rely on the suffix alone to indicate the directionality of the relationship, you have to know the meaning of the word that describes the relationship in order to know the directionality.

@mgi388
Copy link
Contributor

mgi388 commented Jan 19, 2025

I hope this comment amounts to more than just bike shedding, but what about ParentRef and ChildRefs? I refer to my own code (admittedly it's arguable whether my code is a good reference), where I use these two suffixes and it seems intuitive to me. I'd also possibly ask, if the XOf and XBy conventions become recommended, would my code ideally be rewritten using those conventions? If so, does that make sense?

I removed the derives for brevity.

/// A component that can store a reference to a list of unit entities.
pub struct UnitRefs(pub Vec<Entity>);

/// A component to store a list of references to the target entity that a
/// regiment is moving to.
pub struct MoveTargetRefs(pub Vec<Entity>);

/// A component that can store a reference to a regiment entity.
pub struct RegimentRef(pub Entity);

/// A component that can store a reference to a regiment's leader entity.
pub struct RegimentLeaderRef(pub Entity);

Referring to the first updated example in #17427:

mut interaction_query: Query<
    (
        &Interaction,
        &mut BackgroundColor,
        &mut BorderColor,
        &ParentOf,
    ),
    (Changed<Interaction>, With<Button>),
>,

I don't know if it's just me, but I struggle to understand what "parent of" means here: It's hurts trying to work it out. I know I can work it out, but yeah it doesn't look obviously clear to me by reading the query alone.

Replace it with ChildRefs and, at the very least, I now know it's going to be a list of entities.

mut interaction_query: Query<
    (
        &Interaction,
        &mut BackgroundColor,
        &mut BorderColor,
        &ChildRefs,
    ),
    (Changed<Interaction>, With<Button>),
>,

Same with ChildOf example. Before, what are we asking for? "Give me interaction entities and its child of". To me that terminology doesn't make sense at all, but I'd be happy for someone to rewrite it with terminology that does make sense.

mut interaction_query: Query<(&Interaction, &ChildOf), (Changed<Interaction>, With<Button>)>,

After, we now know we're asking for the reference to the parent: "Give me interaction entities and the ref to its parent"

mut interaction_query: Query<(&Interaction, &ParentRef), (Changed<Interaction>, With<Button>)>,

@eidloi
Copy link

eidloi commented Jan 19, 2025

On one hand the English language already defines dedicated words for a number of relationships and rules to form words for new ones. On the other hand - as Cart pointed it out - it makes sense to specifically indicate the built-in-relationship nature of the components. This leaves the dedicated English words available to actual users (e.g. Children and Parents may be used to define a game-mechanic family relationship without interfering with the entity hierarchy used for positioning / rendering).

Whether this makes it weird to read the code is a different matter, but I think relationships need getting used to either way.

Note: Flecs is also convoluted in this matter. It's just how relationship definitions fold out.

@JeanMertz
Copy link
Contributor

JeanMertz commented Jan 19, 2025

I hope this comment amounts to more than just bike shedding, but what about ParentRef and ChildRefs?

I must say, the XRef/XRefs to indicate you're getting a reference/pointer to some other entity/entities in the world, does seem appealing to me. More appealing than XOf.

  • ParentRef: This entity has a reference to its parent (and is thus a child of that parent)
  • ChildrenRefs (or ChildRefs?): This entity has references to its children (and is thus a parent of those children)

Perhaps we could even add EntityRef/EntityRefs traits for users to derive on their own entity-reference-components, to make it easier/uniform to access those references (although it's questionable what else you can achieve with this that you can't get with Deref and DerefMut), other than the naming uniformity in the ecosystem.

And next, if we want to go that route, we could add Bevy lints to check for common encouraged/discouraged patterns:

  • X(Entity) should be renamed to XRef(Entity)
  • XRef(Entity) should derive EntityRef
  • impl EntityRef for X where X should be renamed to XRef

The only real downside I can see with this naming convention is that we already have the Ref query-type in Bevy, which might be a tad confusing, or at the very least a little strange to read: Query<Ref<ParentRef>>

@alice-i-cecile
Copy link
Member Author

The Ref terminology is going to be quite foreign and intimidating to those without a classical programming background (like game designers). These users, even if they never code, will need to interact with these types through asset files and the editor. Since this is so user facing, I'd prefer to avoid it here.

@JeanMertz
Copy link
Contributor

The Ref terminology is going to be quite foreign and intimidating to those without a classical programming background (like game designers).

I see your point. We could still apply the convention generally in the ecosystem, but then also have a type alias for Bevy-specific entity-reference-components that need better names for non-programmers.

In the ECS code, people can then use ParentRef/ChildrenRefs to write any query logic or other code, but in scenes it would have a different name (which, admittedly, will be confusing).

@alice-i-cecile
Copy link
Member Author

The other challenge with a ParentRef / ChildRef convention is that it relies on having a pair of matching, opposed nouns. Most relations won't be so fortunate there :)

@viridia
Copy link
Contributor

viridia commented Jan 19, 2025

The other challenge with a ParentRef / ChildRef convention is that it relies on having a pair of matching, opposed nouns. Most relations won't be so fortunate there :)

Indeed, I was thinking about that this morning: while there are a few cases of reciprocal nouns in English ("buyer" vs. "seller"), this only happens for the most common interpersonal relationships; most use the awkward "-ee" suffix such as "grantor" vs. "grantee" and so on.

@mgi388
Copy link
Contributor

mgi388 commented Jan 19, 2025

The other challenge with a ParentRef / ChildRef convention is that it relies on having a pair of matching, opposed nouns. Most relations won't be so fortunate there :)

@alice-i-cecile Can you elaborate on what you mean and maybe given an example? It might be the case that the example XRefs I've got in my codebase and that I put here are not relevant to this discussion! In my case I don't seem to need to have "matching, opposed nouns" every time I use an XRef component on an entity.

@alice-i-cecile
Copy link
Member Author

Before the recent relations PR landed, the standard way to create custom relations was to only define a single component for them (analogous to Parent). Bevy 0.16's relationships need two components, one for each side of the relationship. Your names make sense in that context, but will be hard to migrate.

Let's take RegimentRef, which points to the regiment that a unit belongs to. To use Bevy's native relations, you also need a component for "the entities which belong to this regiment", which would store a Vec<Entity>. We can't also call that RegimentRef: a natural name might be RegimentMembers in this case.

We could also make the RelationshipTarget component optional (you don't always need to be able to search in the other direction), but that's a completely different issue.

@mgi388
Copy link
Contributor

mgi388 commented Jan 19, 2025

Bevy 0.16's relationships need two components, one for each side of the relationship.

Ah ok this was the piece I was missing. That makes a lot of sense now.

I guess there are probably a couple more reasons why the XRef choice may not be great anyway:

  • It's shortened which can be awkward/undesired/unidiomatic.
  • There's already a Ref used in Bevy ECS queries, e.g. windows: Query<(Entity, Ref<CursorIcon>), With<Window>>, so overloading XRef at the end of component names could add a lot of confusion.

@rparrett
Copy link
Contributor

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.
-- cart

I like this. The plural side of the relationship is less ambiguous than the single side. A single entity probably won't be mistaken for "a children." And the two names seem closely related.

@Jondolf
Copy link
Contributor

Jondolf commented Jan 20, 2025

I kind of feel like there is no specific lintable naming pattern we can (or should) enforce, other than general guidelines for what the names should be describing.

Relationships are essentially just a way to describe how an entity relates to another. In some cases, a noun followed by a possessive preposition like Of makes sense, such as for ChildOf, but oftentimes a verb fits better, like the classic "Alice likes Bob", or "Bob eats apples", where Likes and Eats would be relationships. You could say LikerOf or EaterOf, but it feels awkward. The other side of the relationship is pretty natural for these verbs though, e.g. LikedBy and EatenBy.

I really like ChildOf for the new name of Parent, but ParentOf feels strange to me. For one, it perhaps doesn't encode the plural nature well enough, but more importantly, it sounds more like the core relationship component rather than the "target side" of the relationship that just stores the relationship sources. So I have a preference of keeping it as just Children.


As an aside, a use case of my own: I'm likely switching the ColliderParent component (stores the rigid body entity that a collider is attached to) in Avian to be a relationship. My current planned names are ColliderOf for the core relationship and RigidBodyColliders for storing the sources. RigidBodyOf would be a very bad name as you cannot tell what the relationship is really about (attached colliders). I guess ColliderParentOf could also work, but that doesn't indicate that it is a rigid body. So Of doesn't really work here.

@copygirl
Copy link

copygirl commented Jan 20, 2025

Just a small observation: From what I understand, there previously were components which describe what an entity "is" or "has". So an entity can be a Player or have an amount of Health. There's the option to have the former always be prepended with an "Is" prefix, and in some cases I could think of it making sense: IsFrozen, IsWet, IsEnemy, ... But it doesn't always read best.

Now, with relationships, we also have "verbs". Alice Likes Bob. Bob Eats(3) Apples. (edit: to be fair you could have "verb components" before too, so this is not necessarily related to the introduction of relationships)
Forcing them all to be unified is probably going to result in some very silly names.

So I was wondering, what if components are just named in what feels most intuitive, while avoiding confusion, with clear documentation or even a macro which describes how a component is supposed to be interpreted (could affect how an inspector displays the component), or even written (such as in a DSL)? Silly example:

#[verb(is)]
struct Player(Team);

#[verb(has)]
struct Health(f32);

#[verb(does)]
struct Likes(Entity);

As for the renaming itself, I would tend towards ChildOf for the sake of avoiding ambiguity, however Children still reads better to me, and should not introduce confusion. They also both start with Child, suggesting they might be related by name only?

Wanted to throw my thoughts out there in case someone else more familiar found a use for them.

@cart
Copy link
Member

cart commented Jan 20, 2025

Sounds like there is a reasonable amount of agreement to keep Children as-is and rename Parent to ChildOf. I've updated #17427, which I think should be good to merge now.

github-merge-queue bot pushed a commit that referenced this issue Jan 20, 2025
Fixes #17412

## Objective

`Parent` uses the "has a X" naming convention. There is increasing
sentiment that we should use the "is a X" naming convention for
relationships (following #17398). This leaves `Children` as-is because
there is prevailing sentiment that `Children` is clearer than `ParentOf`
in many cases (especially when treating it like a collection).

This renames `Parent` to `ChildOf`.

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

## Migration Guide

- The `Parent` component has been renamed to `ChildOf`.
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants