Skip to content

Add petgraph back to bevy_ecs #18735

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

Closed
wants to merge 3 commits into from
Closed

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Apr 6, 2025

Objective

petgraph usage was removed from bevy_ecs in #15519 in order for bevy_ecs to work in no_std environments, because at that point in time petgraph did not support no_std. However, with the recent 0.8 release of petgraph, it now works in no_std environments. We should replace our specialized in-tree versions with petgraph once again in order to reduce code maintenance and duplication.

Solution

  • Removed Graph structure from bevy_ecs.
  • Changed DiGraph and UnGraph type aliases to instead point to petgraph's DiGraphMap and UnGraphMap, respectively.
  • Replaced TarjanScc structure with petgraph's own algorithm implementation.

Testing

One test was modified to test against petgraph's tarjan-scc instead of our previously existing implementation.

@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Apr 6, 2025
@ItsDoot ItsDoot added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Apr 6, 2025
@ItsDoot ItsDoot requested a review from bushrat011899 April 6, 2025 05:09
@bushrat011899 bushrat011899 added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Apr 6, 2025
@bushrat011899
Copy link
Contributor

Right off the bat, refer to the PR that removed Petgraph for a benchmark that was used at the time. I believe it showed a performance increase switching away from Petgraph that was later attributed to using a better hasher in the specialised solution. It would be good to verify that switching back doesn't drastically reduce performance.

IMO, a small performance decrease is acceptable for the sake of reduced maintenance burden, and I'd rather we contribute upstream fixes for performance.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Nicely done! It's always nice to have a PR that just removes code. Provided benchmarks don't show substantive regressions, LGTM.

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

I'm not super thrilled about the idea of pulling a petgraph dependency again. I don't think it's a particularly good long term fit for Bevy, and I'd rather leave things as they are now until we have a much clearer idea of broader requirements.

@alice-i-cecile alice-i-cecile removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Apr 6, 2025
@ElliottjPierce
Copy link
Contributor

I'd also be curious about how this might interact with the future MultiThreadedExecutor . IIRC, @NthTensor was working on a new design.

Petgraph reminds me of a catch-all python lib. That's not a bad thing, (it's really useful for data science stuff, etc) but I'm hesitant on using a catch-all lib for something so specialized. As an extreme example, the archetype system is a graph, but we don't use a graph library for that because the custom solution offers more flexibility.

If there's no real performance regression, and it doesn't conflict with future plans for schedule executors, I'm not opposed to this. But in principal, I think custom solutions to Bevy's unique use cases will generally offer more flexibility and performance than an additional dependency.

@ItsDoot ItsDoot closed this Apr 21, 2025
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants