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

update to bevy 0.14 #59

Open
RobWalt opened this issue Jun 10, 2024 · 8 comments
Open

update to bevy 0.14 #59

RobWalt opened this issue Jun 10, 2024 · 8 comments

Comments

@RobWalt
Copy link
Contributor

RobWalt commented Jun 10, 2024

As far as I can tell this crate is going to have a hard time to upgrade to bevy 0.14.

When impl-ing WorldQuery we're currently relying on the fact that we get world acces in init_state and get_state to init/get the trait registry internally and get the relevant data. The API of WorldQuery changed to only ComponentInitializer or Components respectively.

I'm not seeing any easy way of resolving this, hence I wanted to open this issue for discussion here. Ideas are welcome. I'll try hacking around a bit now 🔧

@RobWalt
Copy link
Contributor Author

RobWalt commented Jun 10, 2024

Here's some thread I could trace a bit on discord. Let's try to establish a bit of transparency here:

WrongShoe
It's because systems can have active borrows on more than one query. To be clear I don't think this usage would cause UB, but we probably shouldn't violate our own safety constraints internally in the engine. Can you make get_state take Components instead of &World?
That's how I had it in my original PR

Victoron
I see. Looking at all get_state() impls in the engine, it either only accesses Components via component_id(), or doesn't use world at all, so it would be doable. Though since get_state() is a trait method on WorldQuery, I wonder if any user impls use more aspects of World?

Giooschi
bevy-trait-query appears to fetch a Resource with it

WrongShoe
I don't think this is safe. What if you transmute a trait query and have a ResMut<TraitImplRegistry> in the system?
That would have a mutable reference and a shared referece to the reource live at the same time.

Giooschi
Well, from the point of view of bevy-trait-query it gets a &World so it's perfectly safe to access that resource.

WrongShoe
It's ub, so our api's are incorrect.

@brandon-reinhart
Copy link

Looks like they didn't change init_state, but still changed get_state.

@RobWalt
Copy link
Contributor Author

RobWalt commented Jul 7, 2024

I think you can just use the patched version here:

https://github.com/Azorlogh/bevy-trait-query/tree/bevy-0.14

It just doesn't make use of the state. I just ran the benchmarks again and here is the comparison with respect to bevy 0.13.

TLDR: All improves, One regresses

after bevy update:

All<> - 1 match         time:   [57.837 µs 57.853 µs 57.870 µs]
                        change: [-27.393% -27.221% -27.096%] (p = 0.00 < 0.05)
                        Performance has improved.

All<> - 2 matches       time:   [85.118 µs 85.131 µs 85.145 µs]
                        change: [-20.515% -20.122% -19.877%] (p = 0.00 < 0.05)
                        Performance has improved.

All<> - 1-2 matches     time:   [70.975 µs 70.988 µs 71.001 µs]
                        change: [-22.902% -22.789% -22.689%] (p = 0.00 < 0.05)
                        Performance has improved.

concrete - 1 match      time:   [8.4276 µs 8.4304 µs 8.4332 µs]
                        change: [+1.3473% +1.5102% +1.6452%] (p = 0.00 < 0.05)
                        Performance has regressed.

concrete - 2 matches    time:   [7.9414 µs 7.9477 µs 7.9600 µs]
                        change: [-10.657% -10.239% -9.9041%] (p = 0.00 < 0.05)
                        Performance has improved.

concrete - fragmented   time:   [1.3871 µs 1.3910 µs 1.3958 µs]
                        change: [-4.3640% -3.4317% -2.8527%] (p = 0.00 < 0.05)
                        Performance has improved.

One<> - fragmented      time:   [2.5235 ns 2.5237 ns 2.5240 ns]
                        change: [-14.045% -13.461% -12.906%] (p = 0.00 < 0.05)
                        Performance has improved.

All<> - fragmented      time:   [6.1596 µs 6.1926 µs 6.2526 µs]
                        change: [-2.2789% -1.7270% -1.1198%] (p = 0.00 < 0.05)
                        Performance has improved.

One<>                   time:   [32.187 µs 32.189 µs 32.192 µs]
                        change: [+13.082% +13.191% +13.298%] (p = 0.00 < 0.05)
                        Performance has regressed.

One<> - filtering       time:   [16.730 µs 16.740 µs 16.754 µs]
                        change: [+18.165% +18.335% +18.607%] (p = 0.00 < 0.05)
                        Performance has regressed.

@alice-i-cecile
Copy link

Can you submit a PR for that fork? I'm not the maintainer, but it would be nice to have increased visibility.

@RobWalt
Copy link
Contributor Author

RobWalt commented Jul 8, 2024

AFAIK there are still unresolved issues since transmute won't work with this implementation on those queries and will actually panic (?). I'll add a test and some notes somewhere to showcase that. I'm not sure if we can prevent users from trying to transmute those queries in a good way other than through docs :/

But yeah, at least it's working somehow :)

@cuppachino
Copy link

cuppachino commented Jul 25, 2024

Would it be possible to have an rc version with this change for crates that depend on bevy trait query, just so they can migrate to 0.14?

@RobWalt
Copy link
Contributor Author

RobWalt commented Jul 25, 2024

Would it be possible to have an rc version with this change for crates that depend on bevy trait query, just so they can migrate to 0.14?

You can just add

[patch.crates-io]
bevy-trait-query = { git = "https://github.com/RobWalt/bevy-trait-query.git", branch = "bevy-0.14-partial-update" } 

to your Cargo.toml until the PR is merged. Or is that no option for you?

@StarArawn
Copy link

Would it be possible to have an rc version with this change for crates that depend on bevy trait query, just so they can migrate to 0.14?

You can just add

[patch.crates-io]
bevy-trait-query = { git = "https://github.com/RobWalt/bevy-trait-query.git", branch = "bevy-0.14-partial-update" } 

to your Cargo.toml until the PR is merged. Or is that no option for you?

You can't use this if you're releasing your crate on crates.io. I would appreciate a RC release as well as I'd like to get my crate published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants