[Merged by Bors] - Unique WorldId by DJMcNab · Pull Request #2827 · bevyengine/bevy

DJMcNab · 2021-09-14T20:39:49Z

Objective

Fixes these issues:

WorldIds currently aren't necessarily unique
- I want to guarantee that they're unique to safeguard my librarified version of More idiomatic way for this? Using raw pointer in a Resource to read data from a reference coming from another library. #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 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

DJMcNab · 2021-09-14T20:41:15Z

crates/bevy_ecs/src/world/mod.rs

+            .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.");


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

alice-i-cecile

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 🤔

IceSentry · 2021-09-14T23:00:15Z

crates/bevy_ecs/src/schedule/run_criteria.rs

        Self {
            ran: false,
-            system_id: SystemId::new(),
+


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

inodentry · 2021-09-16T10:23:18Z

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 · 2021-09-16T10:27:31Z

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 · 2021-09-16T23:06:53Z

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 · 2021-09-16T23:07:04Z

bors r+

cart · 2021-09-16T23:08:28Z

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.

# 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 · 2021-09-16T23:30:25Z

Build failed (retrying...):

ci

# 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 · 2021-09-16T23:39:18Z

Build failed:

ci

DJMcNab · 2021-09-17T06:31:13Z

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 · 2021-09-30T20:54:31Z

bors r+

# 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 · 2021-09-30T21:19:28Z

Pull request successfully merged into main.

Build succeeded:

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 14, 2021

DJMcNab commented Sep 14, 2021

View reviewed changes

DJMcNab changed the title ~~Unique worldid~~ Unique WorldId Sep 14, 2021

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

alice-i-cecile approved these changes Sep 14, 2021

View reviewed changes

IceSentry reviewed Sep 14, 2021

View reviewed changes

IceSentry approved these changes Sep 14, 2021

View reviewed changes

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

Use an atomic for WorldId

4e84649

Delete SystemId Also remove `rand` as a normal dependency 🎉 Change WorldId to use usize Add tests and a module for `WorldId`

DJMcNab force-pushed the unique-worldid branch from 3f65af8 to 4e84649 Compare September 17, 2021 20:03

DJMcNab requested a review from alice-i-cecile September 23, 2021 19:21

TheRawMeatball approved these changes Sep 30, 2021

View reviewed changes

bors bot changed the title ~~Unique WorldId~~ [Merged by Bors] - Unique WorldId Sep 30, 2021

bors bot closed this Sep 30, 2021

DJMcNab deleted the unique-worldid branch October 1, 2021 06:26

inodentry mentioned this pull request Oct 1, 2021

Atomic counter for render resource ids and Render to Gpu rename #2895

Closed

DJMcNab mentioned this pull request Apr 10, 2022

[Merged by Bors] - bevy_ecs: Use 32-bit entity ID cursor on platforms without AtomicI64 #4452

Closed

Uh oh!

Conversation

DJMcNab commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Todo

Uh oh!

DJMcNab Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

IceSentry Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

DJMcNab Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

inodentry commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DJMcNab commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart commented Sep 16, 2021

Uh oh!

cart commented Sep 16, 2021

Uh oh!

cart commented Sep 16, 2021

Uh oh!

bors bot commented Sep 16, 2021

Uh oh!

bors bot commented Sep 16, 2021

Uh oh!

DJMcNab commented Sep 17, 2021

Uh oh!

cart commented Sep 30, 2021

Uh oh!

bors bot commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DJMcNab commented Sep 14, 2021 •

edited

Loading

inodentry commented Sep 16, 2021 •

edited

Loading

DJMcNab commented Sep 16, 2021 •

edited

Loading