Skip to content

Bug: too noisy graph displayed#50

Closed
ThierryBerger wants to merge 3 commits intojakobhellermann:mainfrom
ThierryBerger:incorrect_graph
Closed

Bug: too noisy graph displayed#50
ThierryBerger wants to merge 3 commits intojakobhellermann:mainfrom
ThierryBerger:incorrect_graph

Conversation

@ThierryBerger
Copy link
Contributor

@ThierryBerger ThierryBerger commented Sep 11, 2024

I'm not sure if the bug is originating from this crate or bevy, but it seems weird that system_no_set is linked to system_in_child_set2 (green arrow should not exist).

Screenshot from 2024-09-11 11-13-21

Real project example

dump_with_transitive

  • Expected output (obtained with this PR):

dump_no_transitive

ThierryBerger added a commit to ThierryBerger/bevy_rapier that referenced this pull request Sep 11, 2024
I'm not totally convinced yet, this leads to unfortunate API impacts to users, + bevy_mod_debugdump a bit difficult to parse, see jakobhellermann/bevy_mod_debugdump#50
@ThierryBerger
Copy link
Contributor Author

ThierryBerger commented Sep 17, 2024

as of 49d21ca ; the output looks like that:

dump

Real project example output

dump_no_transitive

More context

I tried to use functions from petgraph but was only greeted by difficulties:

  • The graph we use not implementing IntoNeighbours, maybe avoidable by reconstructing a different graph from that one, not too sure...
  • NodeId not implementing petgraph::IndexType...
    • bevy's NodeId not being Default, which is a problem for petgraph::IndexType, not a total blocker I think, but would need petgraph approval
    • bevy's NodeId being an enum which can't be built from an u32/usize ; that's the most problematic one.

I dropped the idea of using petgraph "correctly" and focused on getting somewhat of a progress

@ThierryBerger ThierryBerger marked this pull request as draft September 18, 2024 07:56
@ThierryBerger ThierryBerger marked this pull request as ready for review September 18, 2024 09:51
let hierarchy = graph.hierarchy().graph();
let dependency = graph.dependency().graph();
let dependency = &mut graph.dependency().graph().clone();
remove_transitive_edges(dependency);
Copy link
Contributor Author

@ThierryBerger ThierryBerger Sep 18, 2024

Choose a reason for hiding this comment

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

This might benefit being an option, or fix within bevy (chain() logic which might be the original culprit, but needs a lot more investigation so I'd make that a follow up only if needed.)

@ThierryBerger ThierryBerger changed the title Bug: incorrect graph displayed Bug: too noisy graph displayed Dec 9, 2024
@jakobhellermann
Copy link
Owner

I've rewritten the code a bit and merged the PR as 7f8c5d3

Now without petgraph since bevy doesn't expose it anymore, but I think my toposort implementation should be right. The bevy schedules all look fine, with two or so of them having clarity improvements.

I've made the transitive pruning configurable, but on by default, as I expect most people would prefer that.

@ThierryBerger
Copy link
Contributor Author

Cool, thanks! as a heads-up, I think bevy is considering re-using petgraph, so there might be a little back and forth churn again, but we'll see when it comes :)

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.

2 participants