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] - bevy_ecs: Use 32-bit entity ID cursor on platforms without AtomicI64 #4452

Closed

Conversation

ian-h-chamberlain
Copy link
Contributor

Objective

Solution

  • Conditionally compile entity ID cursor as AtomicI32 when compiling on a platform that does not support 64-bit atomics.

  • This effectively raises the MSRV to 1.60 as it uses a #[cfg] that was only just stabilized there. (should this be noted in changelog?)


Changelog

  • Added bevy_ecs support for platforms without 64-bit atomic ints

Migration Guide

N/A

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 10, 2022
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

It's also worth noting that (as far as I know) we don't plan to make any guarantees about weird platforms at this point in time. That is, if you want it to keep working, you'll likely need to keep patching yourself.

Additionally, I wouldn't be super surprised if this didn't land. I'd say that this probably shouldn't be merged without cart's sign off, since it is definitely a weird hack.

That being said, I'm reasonably happy with the implementation of the change, barring the few corner cases which need additional thought applied.

crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
@ian-h-chamberlain
Copy link
Contributor Author

It's also worth noting that (as far as I know) we don't plan to make any guarantees about weird platforms at this point in time. That is, if you want it to keep working, you'll likely need to keep patching yourself.

Additionally, I wouldn't be super surprised if this didn't land. I'd say that this probably shouldn't be merged without cart's sign off, since it is definitely a weird hack.

Yeah, I figured this might be the case, but the change was relatively small in scope so I thought I'd see if it would be accepted into mainline. I realize it's not a stated goal of the project to maintain compatibility for unusual platforms like this, but if there is a commit hash I can pin to and use Bevy with I will be pretty happy with that :)

I will look to see if I can at least address your initial concerns / comments and we can go from there (if/when cart has a chance to look over it as well).

@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 and removed S-Needs-Triage This issue needs to be labelled labels Apr 11, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Apr 11, 2022

This effectively raises the MSRV to 1.60 as it uses a #[cfg] that was only just stabilized there. (should this be noted in changelog?)

Our stated latest supported release is "latest stable", so no worries there :)

This is an interesting and not very intrusive change 🤔 Needs some cleanup, but I personally don't mind including small tweaks like this for compatibility if they're well-documented and unobtrusive. Well down the road, I think we should consider something like Rust's platform tiers.

You could obviously maintain a fork of Bevy with these minimal changes, but that becomes rather frustrating in the context of the ecosystem, so it would be nice to be able to upstream this.

@ian-h-chamberlain
Copy link
Contributor Author

ian-h-chamberlain commented Apr 11, 2022

Ok, thanks for the helpful feedback! Just to clarify, am I missing anything requested for changes here?

  • Rename type alias to something more descriptive (probably IdCursor)
  • Document the fallback type alias as a compatibility shim
  • Use AtomicIsize instead of AtomicI32 in the non-64-bit case?
  • Use try_from().unwrap() in cases where casting may overflow, and add unwrap comment explaining how it can fail

Is that everything? It sounded like maybe the unwrap could go into a different PR, but maybe it it belongs in this one since the risk of overflow is greater on these kinds of platforms?

@alice-i-cecile
Copy link
Member

Yep, that sounds correct :) Final call on whether we want this sort of compatibility change at all belongs to @cart as a matter of controversial engine-level policy, but I think that those changes should make this PR mergeable if he decides to pry open that particular can of worms.

Just to double-check @ian-h-chamberlain: this was the only change you needed to get bevy_ecs compiling on a 32 bit platform?

@ian-h-chamberlain
Copy link
Contributor Author

Just to double-check @ian-h-chamberlain: this was the only change you needed to get bevy_ecs compiling on a 32 bit platform?

@alice-i-cecile yes, actually with this change and a custom nightly I was able to compile the whole bevy crate with default-features = false! My plan was to try enabling relevant features and see what else might be worth upstreaming if I found issues, but this seemed like a good starting point.

I have only tested on the armv6k-nintendo-3ds target, which has a (WIP + unstable) std implementation, so I'm not sure how well it would work for other 32-bit Rust-tier-3 targets, but if they have std then probably it would work?


I'll try to update the PR later this evening with the requested changes.

@ian-h-chamberlain
Copy link
Contributor Author

Updated with the requested changes, let me know if there's anything else needed!

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.

Code LGTM now. Still going to punt on the "should we do this" call though :)

@ian-h-chamberlain
Copy link
Contributor Author

@alice-i-cecile Based on the recent Discord announcement regarding "controversial" PRs (and your previous comments), should this be labeled with S-Controversial? I would presume that you'd still want cart's approval on this, even if other maintainers signed off on this change?

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 25, 2022
@alice-i-cecile
Copy link
Member

Good call, thanks.

- Rename to [Atomic]IdCursor
- Add comments documentating behavior on non-64-bit-atomic platforms
- try_from().unwrap() in cases that can only fail with 32-bit AtomicIsize
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

The side effects are a bit controversial, but the code looks good to me.

@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 May 10, 2022
@ian-h-chamberlain
Copy link
Contributor Author

@alice-i-cecile I think this has passed the 45-day mark since you marked it as ready for review, is there anything else (a final pass of reviews maybe) needed to get it merged?

@alice-i-cecile
Copy link
Member

For the 45 day mark, I needed a second approval from either @mockersf or @robswain. I think this is a good candidate for that process though; this is controversial but low stakes and will basically never be the "most important thing to review" :)

@robswain
Copy link

robswain commented Jul 1, 2022

@alice-i-cecile You've got the wrong Rob Swain I'm afraid. I definitely don't have anything to do with this project 😄

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 1, 2022

Indeed. cc @superdump

@mockersf
Copy link
Member

mockersf commented Jul 1, 2022

This PR looks good, but there is no way to guarantee it's still working. Another PR could break the support and we would never know until someone complains.
Is there a reason to not use isize/AtomicIsize everywhere?

What kind of guarantee do we want to provide? @ian-h-chamberlain would you be happy with having to fix it occasionally? Do we want to setup a CI job that would cross compile to a target without 64 bits atomics?

@ian-h-chamberlain
Copy link
Contributor Author

ian-h-chamberlain commented Jul 1, 2022

In terms of support – for the armv6k-nintendo-3ds target (or I suppose any other tier 3 targets that happen to benefit from this change), I wouldn't expect Bevy to maintain compatibility continuously, and I'd be fine with fixing it whenever I experienced breakage.

However, it might be reasonable to add CI for a tier 1 target like i686-unknown-linux-gnu. @mockersf would you envision something like .github/workflows/ios.yml that runs periodically, or something as part of the main CI pipeline (like build-wasm and build-android)? I can look into this, I think ubuntu runners should be able to cross compile to a target like this without too much difficulty.

Edit: I need to check that i686-unknown-linux-gnu actually doesn't support 64-bit atomics or find a different platform if it actually does.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 1, 2022

Is there a reason to not use isize/AtomicIsize everywhere?

We want the smallest signed integer big enough to contain u32::MAX.

That is i64. It just so happens that some platforms don't support 64 bit atomics, so for those rare cases, it's better to fall back on isize, which (since we use at least 4 bytes of overhead per entity) is definitely big enough to contain as many entities as can be spawned (u32::MAX, but capped at usize::MAX/4).

The reason not to use isize everywhere is that it's wasteful on 128 bit platforms. Admittedly, I don't know of any 128 bit platforms, but I'd rather keep it as the condition explained in my first paragraph.

And I think the consensus is that we don't guarantee it works. We just don't intentionally stand in the way of it not working (without good reason, anyway). In rustc parlance, these are tier 3 targets, i.e. it might be supported, and you can bring PRs to fix the support, but we don't try and ensure it is.

@mockersf
Copy link
Member

mockersf commented Jul 1, 2022

Thanks for the explanation!

@mockersf would you envision something like .github/workflows/ios.yml that runs periodically, or something as part of the main CI pipeline (like build-wasm and build-android)?

If GitHub had better support for actions on cron, maybe, but as it is for the moment it's pretty useless. And I would rather not add it to the main CI jobs, they are already long enough.
The best from my point of view is if you had a project somewhere that is tested regularly on your target architecture and would report issues, but that put the burden on you.

@ian-h-chamberlain
Copy link
Contributor Author

The best from my point of view is if you had a project somewhere that is tested regularly on your target architecture and would report issues, but that put the burden on you.

It is my intention to create a bevy_3ds project which will provide plugins for working with Bevy on the platform, and I would like to have some CI there that would catch something like this (and put the burden on me to PR here for any fixes). It's very early stages so I don't have anything set up currently, but if that would meet your requirements then maybe we are all set?

@alice-i-cecile
Copy link
Member

Yep, we're on board with that setup for now. This is still controversial though, so I've started the timer to merge :)

@superdump
Copy link
Contributor

@alice-i-cecile You've got the wrong Rob Swain I'm afraid. I definitely don't have anything to do with this project 😄

Hello name-sharer! Much love to you!

@ian-h-chamberlain
Copy link
Contributor Author

It looks like this has reached the 45-day mark, based on the project page. Is there anything else needed to merge now that we've hit the timer?

@alice-i-cecile
Copy link
Member

I have no further objections :) I'm on vacation though, so I'll merge this at the start of September if no one else beats me to it.

@cart
Copy link
Member

cart commented Aug 20, 2022

If this change was any more invasive than it is, I'd probably block it. But its just scoped enough that I'm willing to squeeze it in. It makes me just a little bit uncomfortable and we're catering to a pretty niche crowd here (3ds is effectively dead at this point, so this is enabling homebrew stuff? And I haven't seen this issue come up on any other platforms). Getting bevy working on a 3ds is definitely fun and cool though, and we should encourage doing fun and cool things, within reason.

But for the person (or AI) years in the future reading this thread and writing future Bevy ECS code: if you need to drop this support for any legitimate reason ... I think you should do it.

@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 21, 2022
…4452)

# Objective
- Fixes #4451

## Solution
- Conditionally compile entity ID cursor as `AtomicI32` when compiling on a platform that does not support 64-bit atomics.

- This effectively raises the MSRV to 1.60 as it uses a `#[cfg]` that was only just stabilized there. (should this be noted in changelog?)

---

## Changelog
- Added `bevy_ecs` support for platforms without 64-bit atomic ints


## Migration Guide
N/A
@bors bors bot changed the title bevy_ecs: Use 32-bit entity ID cursor on platforms without AtomicI64 [Merged by Bors] - bevy_ecs: Use 32-bit entity ID cursor on platforms without AtomicI64 Aug 21, 2022
@bors bors bot closed this Aug 21, 2022
@ian-h-chamberlain ian-h-chamberlain deleted the entity-32bit-cursor branch August 21, 2022 01:06
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Sep 28, 2022
…evyengine#4452)

# Objective
- Fixes bevyengine#4451

## Solution
- Conditionally compile entity ID cursor as `AtomicI32` when compiling on a platform that does not support 64-bit atomics.

- This effectively raises the MSRV to 1.60 as it uses a `#[cfg]` that was only just stabilized there. (should this be noted in changelog?)

---

## Changelog
- Added `bevy_ecs` support for platforms without 64-bit atomic ints


## Migration Guide
N/A
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…evyengine#4452)

# Objective
- Fixes bevyengine#4451

## Solution
- Conditionally compile entity ID cursor as `AtomicI32` when compiling on a platform that does not support 64-bit atomics.

- This effectively raises the MSRV to 1.60 as it uses a `#[cfg]` that was only just stabilized there. (should this be noted in changelog?)

---

## Changelog
- Added `bevy_ecs` support for platforms without 64-bit atomic ints


## Migration Guide
N/A
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#4452)

# Objective
- Fixes bevyengine#4451

## Solution
- Conditionally compile entity ID cursor as `AtomicI32` when compiling on a platform that does not support 64-bit atomics.

- This effectively raises the MSRV to 1.60 as it uses a `#[cfg]` that was only just stabilized there. (should this be noted in changelog?)

---

## Changelog
- Added `bevy_ecs` support for platforms without 64-bit atomic ints


## Migration Guide
N/A
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Development

Successfully merging this pull request may close these issues.

bevy_ecs: Allow using i32 for entity ID cursor on platforms without AtomicI64 support
10 participants