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

Remame Entity::new(id: u32) to from_raw #3108

Closed
alice-i-cecile opened this issue Nov 11, 2021 · 6 comments
Closed

Remame Entity::new(id: u32) to from_raw #3108

alice-i-cecile opened this issue Nov 11, 2021 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

What problem does this solve or what need does it fill?

Users are commonly confused by this method, and try and fail to use it for synchronization tasks.

What solution would you like?

Make this method pub(crate).

What alternative(s) have you considered?

Better document if and when it can be correctly used.

Additional context

Generally the advice when attempting to synchronize the ECS is to insert a secondary identifier as a component, rather than attempting to ensure that Entity lines up between the instances.

I have not seen any valid external use of this API, but if they exist, we should attempt to create workarounds or focus on the documentation path.

@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 labels Nov 11, 2021
@mockersf
Copy link
Member

This is currently used in scenes and in transforms. Transforms could probably be rework to not use it, but it seems harder for scenes.

@HackerFoo
Copy link
Contributor

I use this method to initialize a known sizeVec<Entity> using

  let mut entities = vec![Entity::new(0); n];

that is then overwritten by index with valid entities for all indices, but in random order. This pattern is often used for building indices or sorting parallel vectors, where one vector is sorted and others are permuted in the same way.

I'd prefer to use Default::default() if it were implemented for Entity, which would give an always invalid Entity.

@mockersf
Copy link
Member

mockersf commented Nov 11, 2021

Maybe related to #3029 which introduce the notion of an invalid entity

@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Nov 12, 2021
@Davier
Copy link
Contributor

Davier commented Nov 12, 2021

I have not seen any valid external use of this API, but if they exist, we should attempt to create workarounds or focus on the documentation path.

I use it externally when deriving Reflect for any component that has an Entity field (like in impl FromWorld for Parent)

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 12, 2021

Maybe rename new to from_raw?

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Nov 12, 2021
@alice-i-cecile
Copy link
Member Author

Maybe rename new to from_raw?

I like this, and think this is how we should proceed. A better name, plus clear docs should resolve the confusion.

The feedback from the users about how this is used is very useful; @Davier's use case in particular seems very challenging to eliminate.

@alice-i-cecile alice-i-cecile changed the title Make Entity::new(id: u32) private Remame Entity::new(id: u32) to from_raw Dec 12, 2021
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Dec 12, 2021
@bors bors bot closed this as completed in 8a8293b Dec 29, 2021
mockersf pushed a commit to mockersf/bevy that referenced this issue Jan 1, 2022
# Objective

- Rename `Entity::new(id: u32)` to `Entity::from_raw(id: u32)`.
- Add further documentation.
- fixes bevyengine#3108

## Solution

- Renamed `Entity::new(id: u32)` to `Entity::from_raw(id: u32)`.
- Docs extended.

I derived the examples from the discussion of issue bevyengine#3108 .

The [first case](bevyengine#3108 (comment)) mentioned in the linked issue is quite obvious but the [second one](bevyengine#3108 (comment)) probably needs further explanation.


Co-authored-by: r4gus <david@thesugar.de>
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-Docs An addition or correction to our documentation D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants