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] - Unique WorldId #2827

Closed
wants to merge 1 commit into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Sep 14, 2021

Objective

Fixes these issues:

Solution

  • Instead of randomly generating WorldIds, just use an incrementing atomic counter, panicing on overflow.
  • Remove SystemId
    • We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own SystemId anyway.
  • Now that these don't depend on rand, move it to a dev-dependency

Todo

Determine if WorldId should be u32 based instead

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 14, 2021
.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |val| {
val.checked_add(1)
})
.expect("More bevy `World`s have been created than is supported. Please tell us what you were doing, because frankly I'm impressed.");
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I'd removed this second sentence before committing. I'm happy to keep it as-is though 😆

@DJMcNab DJMcNab changed the title Unique worldid Unique WorldId Sep 14, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Sep 14, 2021
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Once the error message is cleaned up this LGTM.

I don't have strong feelings about u32 vs u64 for world ids. u32 is probably slightly more reasonable as there's effectively no way to break those limits without going OOM elsewhere. But on the other hand, there's no real cost to storing more data here 🤔

@@ -407,7 +406,7 @@ impl Default for RunOnce {
fn default() -> Self {
Self {
ran: false,
system_id: SystemId::new(),

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary whitespace, I'm surprised rustfmt doesn't check for that

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 15, 2021
@inodentry
Copy link
Contributor

inodentry commented Sep 16, 2021

Regarding u64 vs u32 for WorldId, I do have strong feelings. This is an atomic variable, which means that it relies on appropriate support for atomics from the underlying CPU architecture. There are many platforms that do not support 64-bit atomics, but do support 32-bit atomics. Most 32-bit architectures / compilation targets are like that. If we require 64-bit atomics for bevy, we are restricting portability to such platforms/architectures.

As @alice-i-cecile said, it is unlikely to break the limit of u32. It is very doubtful that someone would want to have more than ~4.2 billion Worlds.

Whereas portability and platform support is an actual problem with actual consequences. Let's not needlessly limit the kinds of devices that bevy can run on.

@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 16, 2021

Hmm, I hadn't considered that - I wonder if there should be a clippy lint for that (beyond just using the more general disallowed function)

We should use AtomicUsize then, since std depends on that being supported.

@cart
Copy link
Member

cart commented Sep 16, 2021

I dig these changes. I do think that ultimately we might need a unique SystemId for "manually triggered systems" and "reactions", but I'm happy to just re-add that if/when its actually needed. SystemId has been dead code since bevy_ecs's inception. Removing it makes sense.

@cart
Copy link
Member

cart commented Sep 16, 2021

bors r+

@cart
Copy link
Member

cart commented Sep 16, 2021

Lol I have a nasty habit of not checking CI before bors-ing. You should just swap out the WorldId::new() constructor for a Default impl.

bors bot pushed a commit that referenced this pull request Sep 16, 2021
# Objective

Fixes these issues:
- `WorldId`s currently aren't necessarily unique
    - I want to guarantee that they're unique to safeguard my librarified version of #2805
    - There probably hasn't been a collision yet, but they could technically collide
- `SystemId` isn't used for anything
  - It's no longer used now that `Locals` are stored within the `System`.
- `bevy_ecs` depends on rand

## Solution

- Instead of randomly generating `WorldId`s, just use an incrementing atomic counter, panicing on overflow.
- Remove `SystemId` 
    - We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own `SystemId` anyway.
- Now that these don't depend on rand, move it to a dev-dependency

## Todo

Determine if `WorldId` should be `u32` based instead
@bors
Copy link
Contributor

bors bot commented Sep 16, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 16, 2021
# Objective

Fixes these issues:
- `WorldId`s currently aren't necessarily unique
    - I want to guarantee that they're unique to safeguard my librarified version of #2805
    - There probably hasn't been a collision yet, but they could technically collide
- `SystemId` isn't used for anything
  - It's no longer used now that `Locals` are stored within the `System`.
- `bevy_ecs` depends on rand

## Solution

- Instead of randomly generating `WorldId`s, just use an incrementing atomic counter, panicing on overflow.
- Remove `SystemId` 
    - We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own `SystemId` anyway.
- Now that these don't depend on rand, move it to a dev-dependency

## Todo

Determine if `WorldId` should be `u32` based instead
@bors
Copy link
Contributor

bors bot commented Sep 16, 2021

Build failed:

@DJMcNab
Copy link
Member Author

DJMcNab commented Sep 17, 2021

That's annoying - I even knew that lint would trigger but forgot to deal with it.

I explicitly chose not to make it a Default impl, because it doesn't really make sense to have a WorldId seperated from a World. Additionally, the default value isn't really a default as such imo

I still expose the constructor function publicly because it could be used e.g. for tests in external crates, although I'm not certain that's really needed.

Delete SystemId

Also remove `rand` as a normal dependency 🎉

Change WorldId to use usize

Add tests and a module for `WorldId`
@cart
Copy link
Member

cart commented Sep 30, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 30, 2021
# Objective

Fixes these issues:
- `WorldId`s currently aren't necessarily unique
    - I want to guarantee that they're unique to safeguard my librarified version of #2805
    - There probably hasn't been a collision yet, but they could technically collide
- `SystemId` isn't used for anything
  - It's no longer used now that `Locals` are stored within the `System`.
- `bevy_ecs` depends on rand

## Solution

- Instead of randomly generating `WorldId`s, just use an incrementing atomic counter, panicing on overflow.
- Remove `SystemId` 
    - We do need to allow Locals for exclusive systems at some point, but exclusive systems couldn't access their own `SystemId` anyway.
- Now that these don't depend on rand, move it to a dev-dependency

## Todo

Determine if `WorldId` should be `u32` based instead
@bors bors bot changed the title Unique WorldId [Merged by Bors] - Unique WorldId Sep 30, 2021
@bors bors bot closed this Sep 30, 2021
@DJMcNab DJMcNab deleted the unique-worldid branch October 1, 2021 06:26
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 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.

6 participants