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

Stageless #47

Merged
merged 8 commits into from May 28, 2022
Merged

Stageless #47

merged 8 commits into from May 28, 2022

Conversation

freiksenet
Copy link
Contributor

Did a stab at implementing stageless. There is some cursed hairiness with ExclusiveSystem traits in bevy/iyes_loopless, so sorry for hairy code at places.

Note that this requires your state to be pre-initialized before AssetLoader, I think it's limitation of how iyes_loopless does State.

@freiksenet
Copy link
Contributor Author

Fixed tests, whew that was fun.

Copy link
Owner

@NiklasEi NiklasEi left a comment

Choose a reason for hiding this comment

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

Slowly the crate has too many feature flags 😆
Maybe we could move some of the now duplicated systems into extra files? Then we have the stageless stuff a bit more separated and need less cfg attributes checking for the feature. E.g. have one mod behind a #[cfg(feature = "stageless")] containing all the stageless systems and one mod behind #[cfg(not(feature = "stageless"))] containing all the old systems.

bevy_asset_loader_derive/Cargo.toml Outdated Show resolved Hide resolved
bevy_asset_loader/Cargo.toml Outdated Show resolved Hide resolved
bevy_asset_loader/examples/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
bevy_asset_loader/src/asset_loader/systems.rs Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Co-authored-by: Niklas Eicker <git@nikl.me>
@freiksenet
Copy link
Contributor Author

Thanks, will fix the comments.

@NiklasEi
Copy link
Owner

NiklasEi commented May 23, 2022

I understood now why the tests are red. Could you split the CI job into one test job for bevy_asset_loader_derive and one for bevy_asset_loader, please?
You can add --package bevy_asset_loader{_derive} to the cargo hack command to only run in that package. The command for bevy_asset_loader_derive does not need any feature grouping.

@freiksenet
Copy link
Contributor Author

Should be good now :)

README.md Outdated Show resolved Hide resolved
@NiklasEi
Copy link
Owner

It looks like a cargo fmt --all run is missing.

If you've had enough of fighting with the CI, I could also merge the PR. The CI issues can still be fixed on the stageless branch. Just say the word 🙂
Thank you for all the work!

@freiksenet
Copy link
Contributor Author

freiksenet commented May 27, 2022

Haha, this is fine, super smooth comparing to CI at my day job.

@freiksenet
Copy link
Contributor Author

I don't understand, it passes on my machine on windows O_O

@NiklasEi
Copy link
Owner

Could you try adding --clean-per-run to the cargo hack command?

@NiklasEi
Copy link
Owner

I also don't understand what's happening here. The test is green, but the process fails. Locally everything seems fine, too.
I will merge and take a look at the CI setup separately.

Thanks again for the implementation 👍

@NiklasEi NiklasEi merged commit 59ae198 into NiklasEi:stageless May 28, 2022
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

Successfully merging this pull request may close these issues.

None yet

2 participants