Skip to content

Add EntityDoesNotExistError, replace cases of Entity as an error, do some easy Resultification#17855

Merged
alice-i-cecile merged 6 commits intobevyengine:mainfrom
JaySpruce:entity_dne_error
Feb 16, 2025
Merged

Add EntityDoesNotExistError, replace cases of Entity as an error, do some easy Resultification#17855
alice-i-cecile merged 6 commits intobevyengine:mainfrom
JaySpruce:entity_dne_error

Conversation

@JaySpruce
Copy link
Copy Markdown
Member

@JaySpruce JaySpruce commented Feb 13, 2025

Objective

There's no general error for when an entity doesn't exist, and some methods are going to need one when they get Resultified. The closest thing is EntityFetchError, but that error has a slightly more specific purpose.

Solution

  • Added EntityDoesNotExistError.
    • Contains Entity and EntityDoesNotExistDetails.
  • Changed EntityFetchError and QueryEntityError:
    • Changed NoSuchEntity variant to wrap EntityDoesNotExistError and renamed the variant to EntityDoesNotExist.
  • Renamed EntityFetchError to EntityMutableFetchError to make its purpose clearer.
  • Renamed TryDespawnError to EntityDespawnError to make it more general.
  • Changed World::inspect_entity to return Result<[ok], EntityDoesNotExistError> instead of panicking.
  • Changed World::get_entity and WorldEntityFetch::fetch_ref to return Result<[ok], EntityDoesNotExistError> instead of Result<[ok], Entity>.
  • Changed UnsafeWorldCell::get_entity to return Result<UnsafeEntityCell, EntityDoesNotExistError> instead of Option<UnsafeEntityCell>.

Migration Guide

  • World::inspect_entity now returns Result<impl Iterator<Item = &ComponentInfo>, EntityDoesNotExistError> instead of impl Iterator<Item = &ComponentInfo>.
  • World::get_entity now returns EntityDoesNotExistError as an error instead of Entity. You can still access the entity's ID through the error's entity field.
  • UnsafeWorldCell::get_entity now returns Result<UnsafeEntityCell, EntityDoesNotExistError> instead of Option<UnsafeEntityCell>.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Feb 13, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 13, 2025
@github-actions
Copy link
Copy Markdown
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@JaySpruce JaySpruce changed the title Add EntityDoesNotExistError and start using it Add EntityDoesNotExistError, replace cases of Entity as an error, do some easy Resultification Feb 14, 2025
))?;
let ecell = cell
.get_entity(id)
.map_err(EntityMutableFetchError::EntityDoesNotExist)?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we impl From<EntityDoesNotExistError> for EntityMutableFetchError, then ? will do the error mapping for us, right? Would that be worth doing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like that impl: I think it's always the correct mapping.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had thought it might be better to be explicit about what conversion is happening, but thinking about it again, using From is probably better

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 15, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Feb 16, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

@JaySpruce I'll merge this once the new conflicts are resolved.

@JaySpruce
Copy link
Copy Markdown
Member Author

@alice-i-cecile, good to go

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 16, 2025
Merged via the queue into bevyengine:main with commit ee44560 Feb 16, 2025
@JaySpruce JaySpruce deleted the entity_dne_error branch February 16, 2025 22:28
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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants