Skip to content

Conversation

@Brezak
Copy link
Contributor

@Brezak Brezak commented Mar 11, 2025

Objective

Option is technically a collection.

Solution

Implement RelationShipSourceCollection for Option<Entity>

Testing

Added a new test

@Brezak Brezak marked this pull request as draft March 11, 2025 16:50
@Brezak Brezak force-pushed the relationship-collection-option branch from c65278d to 5a7d332 Compare March 11, 2025 16:52
@Brezak Brezak marked this pull request as ready for review March 11, 2025 18:01
@ItsDoot
Copy link
Contributor

ItsDoot commented Mar 11, 2025

#18087 Decided to go with Entity instead of Option<Entity> for one-to-one relationships, I don't think we need to have two ways to do the same thing.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. labels Mar 11, 2025
@alice-i-cecile
Copy link
Member

Agreed. I think this is largely redundant, but I'm open to being convinced.

@Brezak
Copy link
Contributor Author

Brezak commented Mar 11, 2025

I've mostly decided to implement RelationshipSourceCollection for every collection that can take it and Option was one of them.

Looking at #18087. I'm mainly concerned about the inevitable situation of a user spawning a entity with the Entity::PLACEHOLDER id and throwing it into a relationship. The RelationShipTarget will end up reporting that it's empty even if it isn't and I don't know what bugs that will cause. The rarity of this situation is going to turn these bugs into the worst kind of heisenbugs. Ones that require the application to be running for an hour before they manifest.

@alice-i-cecile
Copy link
Member

Looking at #18087. I'm mainly concerned about the inevitable situation of a user spawning a entity with the Entity::PLACEHOLDER id and throwing it into a relationship

We're restricting the ability to spawn entities with specific IDs and that ID will never be handed out otherwise.

That said, I do sympathize with the desire to implement this trait for every sensible collection type 🤔

@Brezak
Copy link
Contributor Author

Brezak commented Mar 11, 2025

That said, I do sympathize with the desire to implement this trait for every sensible collection type 🤔

That's blocked on #18243 since the remaining collections are implementations of a set (and therefore suffer from the same issue).

I'm going to close this since Entity works and makes this mostly redundant.

@Brezak Brezak closed this Mar 11, 2025
@Brezak Brezak deleted the relationship-collection-option branch March 21, 2025 16:36
Danielfcf16 added a commit to Danielfcf16/bevy that referenced this pull request Apr 3, 2025
Improves debugging experience by including the Name component in despawn failure messages.
This makes it easier to identify which entity caused the problem, especially in games
with many entities.

If the Name component is present, the log shows something like: 'Player' (Entity(42v1#123456)).
If not, it falls back to showing the raw entity ID.
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-Feature A new feature, making something new possible S-Nominated-To-Close A triage team member thinks this PR or issue should be closed out. 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.

3 participants