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

[ian-h-chamberlain]

Objective

• Fixes bevy_ecs: Allow using i32 for entity ID cursor on platforms without AtomicI64 support #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

[DJMcNab]

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.

[ian-h-chamberlain]

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]

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]

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]

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]

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]

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

[alice-i-cecile]

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

[ian-h-chamberlain]

@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]

Good call, thanks.

[IceSentry]

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

[ian-h-chamberlain]

@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]

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]

@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]

Indeed. cc @superdump

[mockersf]

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]

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]

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]

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]

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]

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

[superdump]

@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]

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]

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]

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]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12