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

remove ParallelIterator #2093

Closed
wants to merge 3 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented May 3, 2021

ParallelIterator was only mentioned in a few docs, I tried removing it and... everything still works! 😌

While not used by Bevy, it may still be useful as a public trait for third party to implement?
I think it should have been removed with ECSv2.

@mockersf mockersf added the C-Code-Quality A section of code that is hard to understand or change label May 3, 2021
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@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 3, 2021
@fopsdev
Copy link

fopsdev commented May 4, 2021

this has nothing to do with:
#2088
right?
(par_iter != ParrallelIterator as mentioned here)

@cart
Copy link
Member

cart commented May 4, 2021

Nope this is definitely the ParallelIterator implemented in #2088. We should probably have a discussion that weighs the complexity cost of maintaining ParallelIterator + another query iterator variant (but with way more flexibility) vs committing to only supporting par_for_each / par_for_each_mut at the cost of disallowing other interesting iterator patterns.

We have a lot of iterator variants on the table now (QueryIter, BatchIter, QueryCombinationIter, for_each, par_for_each). Given the complexity of bevy's query iteration logic, we should sort out the best way to manage complexity across so many variants (ideally without compromising performance or api quality).

Pulling in @TheRawMeatball, @Frizi who are both writing iterator code and @BoxyUwU who is adding iterator features like Relations.

@mockersf
Copy link
Member Author

mockersf commented May 4, 2021

oh it had nothing to do with the first impl of #2088, but it's used by the rewrite

If we keep it, par_for_each* should probably be rewritten to use it instead of having their own logic

@cart
Copy link
Member

cart commented May 5, 2021

If we keep it, par_for_each* should probably be rewritten to use it instead of having their own logic

The downside of adapting par_for_each is that we need to do the "dense check" branching per-next-call instead of once at the beginning. We should bench that to see what the cost is.

@alice-i-cecile alice-i-cecile removed 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 Jul 12, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Dec 12, 2021
@CGMossa
Copy link
Contributor

CGMossa commented Dec 22, 2021

I'm very much interested in more parallel capabilities than for_each and for_each_mut. I've looked in the PRs, discord and I see that there is a discussion that needs to be had, before this progresses.
Let me know if I can aid it in any way.

@mockersf mockersf marked this pull request as draft February 1, 2022 22:17
@mockersf mockersf closed this Feb 24, 2022
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants