Skip to content

Move Name out of bevy_core #16894

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

Merged
merged 6 commits into from
Dec 19, 2024

Conversation

bushrat011899
Copy link
Contributor

Objective

Solution

  • Moved Name and NameOrEntity into bevy_ecs::name, and added them to the prelude.

Testing

  • CI

Migration Guide

If you were importing Name or NameOrEntity from bevy_core, instead import from bevy_ecs::name.

@bushrat011899 bushrat011899 added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change A-Core X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 19, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Uncontroversial This work is generally agreed upon labels Dec 19, 2024
@alice-i-cecile
Copy link
Member

This will be one of the first "blessed" components, so it's not a fully uncontroversial decision. I think this is the right choice though, since my understanding is that this will be needed for the BSN macro.

Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments

@@ -53,7 +53,7 @@ impl<T> Default for ReportHierarchyIssue<T> {
/// (See B0004 explanation linked in warning message)
pub fn check_hierarchy_component_has_valid_parent<T: Component>(
parent_query: Query<
(Entity, &Parent, Option<&bevy_core::Name>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary for this PR, but we could replace the Option<&Name> + Entity with NameOrEntity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree, I'll leave it off just to keep things less controversial, but it'd be a nice little follow-up.

@bushrat011899 bushrat011899 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 Dec 19, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 19, 2024
@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed X-Contentious There are nontrivial implications that should be thought through labels Dec 19, 2024
Merged via the queue into bevyengine:main with commit d4b07a5 Dec 19, 2024
34 checks passed
@ItsDoot ItsDoot added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 19, 2024
@ItsDoot ItsDoot mentioned this pull request Dec 19, 2024
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# Objective

- Contributes to bevyengine#16892

## Solution

- Moved `Name` and `NameOrEntity` into `bevy_ecs::name`, and added them
to the prelude.

## Testing

- CI

## Migration Guide

If you were importing `Name` or `NameOrEntity` from `bevy_core`, instead
import from `bevy_ecs::name`.

---------

Co-authored-by: Christian Hughes <[email protected]>
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
# Objective

- Contributes to bevyengine#16892

## Solution

- Moved `Name` and `NameOrEntity` into `bevy_ecs::name`, and added them
to the prelude.

## Testing

- CI

## Migration Guide

If you were importing `Name` or `NameOrEntity` from `bevy_core`, instead
import from `bevy_ecs::name`.

---------

Co-authored-by: Christian Hughes <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Feb 11, 2025
# Objective

After #16894, this example started logging errors:

```
ERROR bevy_asset::server: Failed to load asset 'scenes/load_scene_example.scn.ron' with asset loader 'bevy_scene::scene_loader::SceneLoader': Could not parse RON: 10:33: Expected string
```

Fixes #17798, this is the only actionable/unreported issue in there as
far as I can tell.

## Solution

Update the serialized scene with the expected format for `Name`

## Testing

`cargo run --example scene`

## Discussion

This example breaks very often and we don't always catch it. It might be
nice to have this scene either

1. produce visual output so that it can be checked
2. panic if the scene fails to load (check for LoadState::Failed)

Either of those would make the failures visible in [the example
report](https://thebevyflock.github.io/bevy-example-runner/). Not sure
which method would best suit the example.
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-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy M-Needs-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 X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants