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

[Merged by Bors] - Make most Entity methods const #5688

Closed
wants to merge 2 commits into from

Conversation

djeedai
Copy link
Contributor

@djeedai djeedai commented Aug 14, 2022

Objective

Fixes #5687

Solution

Update the methods on the Entity struct to be const, so we can
define compile-time constants and more generally use them in a const
context.


Changelog

Added

  • Most Entity methods are now const fn.

Update the methods on the `Entity` struct to be `const`, so we can
define compile-time constants and more generally use them in a const
context.

Fixes bevyengine#5687
@MinerSebas
Copy link
Contributor

MinerSebas commented Aug 14, 2022

In principle, I agree with this change, the more code that can be executed at compile time, the better.

In practice, I don't see a (current) benefit for these fn's to be const already, as Entity is an unstable runtime identifier.
Only once you can run the (whole) ecs at compile-time, do I see any benefits here.

@djeedai
Copy link
Contributor Author

djeedai commented Aug 14, 2022

In principle, I agree with this change, the more code that can be executed at compile time, the better.

In practice, I don't see a (current) benefit for these fn's to be const already, as Entity is an unstable runtime identifier. Only once you can run the (whole) ecs at compile-time, do I see any benefits here.

I want to make an invalid Entity to initialize my batching, like this:

const INVALID: Entity = Entity::from_bits(u64::MAX);

You can argue that this will break when the 4th billionth entity reaches its 4th billionth generation; I'm willing to take that risk as a limitation of my batching algorithm. I'm also willing to bet other stuffs will break before anyone reaches that limit.

So I appreciate defining an invalid Entity is controversial, I didn't include it in this PR, but at least I'd like to be able to do it in my crate.

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 14, 2022

I think a better alternative would be defining an invalid entity (with eg index 0) in bevy as const and making sure that it doesn't get allocated, which I think is possible by "allocating" it with EntityMeta::EMPTY.

@djeedai
Copy link
Contributor Author

djeedai commented Aug 14, 2022

I think a better alternative would be defining an invalid entity (with eg index 0) in bevy as const and making sure that it doesn't get allocated, which I think is possible by "allocating" it with EntityMeta::EMPTY.

This requires the current PR though, doesn't it? 😬

@bjorn3
Copy link
Contributor

bjorn3 commented Aug 14, 2022

No, you can have impl Entity { const INVALID: Entity = Entity { id: 0, generation: 0 } } in bevy_ecs without this PR.

@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 Aug 14, 2022
@cart
Copy link
Member

cart commented Aug 18, 2022

I like @bjorn3's suggestion. Letting users define custom const entities feels like a foot gun.

@djeedai
Copy link
Contributor Author

djeedai commented Aug 18, 2022

I like @bjorn3's suggestion. Letting users define custom const entities feels like a foot gun.

Sure, but that's a much larger and more complex change, even maybe due for an RFC if now all systems need to account for this.

We could use ideally some NonZero<u32> so we can benefit from the compiler optimization of Option when we want to store an Entity possibly invalid.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I guess in fairness we do already support creating these identifiers from their "pieces" at runtime (which is also generally a foot gun for the same reasons).

Supporting specific entities at compile time isn't particularly different, and I do like the idea of not needing Options or unsafe for scenarios like this, despite there being user-facing risk.

Short term, I'm cool with merging this. But long term, we should consider closing up these apis entirely (while still supporting the scenarios we need to support). I do like the idea of Entity:INVALID. Seems worth exploring, but I do agree thats a big-ish change.

@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 30, 2022
# Objective

Fixes #5687

## Solution

Update the methods on the `Entity` struct to be `const`, so we can
define compile-time constants and more generally use them in a const
context.

---

## Changelog

### Added

- Most `Entity` methods are now `const fn`.
@bors bors bot changed the title Make most Entity methods const [Merged by Bors] - Make most Entity methods const Aug 30, 2022
@bors bors bot closed this Aug 30, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Fixes bevyengine#5687

## Solution

Update the methods on the `Entity` struct to be `const`, so we can
define compile-time constants and more generally use them in a const
context.

---

## Changelog

### Added

- Most `Entity` methods are now `const fn`.
bors bot pushed a commit that referenced this pull request Nov 12, 2022
# Objective
Fix #6548. Most of these methods were already made `const` in #5688. `Entity::to_bits` is the only one that remained.

## Solution
Make it const.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fixes bevyengine#5687

## Solution

Update the methods on the `Entity` struct to be `const`, so we can
define compile-time constants and more generally use them in a const
context.

---

## Changelog

### Added

- Most `Entity` methods are now `const fn`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Fix bevyengine#6548. Most of these methods were already made `const` in bevyengine#5688. `Entity::to_bits` is the only one that remained.

## Solution
Make it const.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider making some Entity methods const
5 participants