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

Implement RelationshipSourceCollection for Option<Entity> #18260

Closed

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.

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