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

WIP: events graph #31

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

WIP: events graph #31

wants to merge 11 commits into from

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Mar 22, 2024

to understand the flow of events, a graph of systems writing to events and reading which events might be useful.

example render on bevy_mod_picking (example minimal):

events_ambiguous_outside_schedules

@Vrixyz Vrixyz marked this pull request as draft March 22, 2024 18:05
@jakobhellermann
Copy link
Owner

That's pretty cool, thanks!

Is there anything missing here functionality-wise that is still WIP or just polish? (making the function names consistent with the other graphs, and making the colors configurable).

@jakobhellermann
Copy link
Owner

Also is the example render filtered, or are there some systems not counted yet? This looks like it's missing a good amount of systems.

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Mar 28, 2024

Indeed my first screenshot was a bit underwhelming, my initial implementation did only render 1 schedule, but in practice events are used throughout the schedules, so I updated the code to be able to display multiple schedules at once.

That's the latest state (as of commit 5615dad), running on bevy_mod_picking (example minimal):

image

I filtered out the event_update_system which is clearing the events, I guess we'd want that configurable.

I'd like to explore different things in this PR:

  • consistency with other features from debugdump (you mentioned color style...)
  • showing which schedule a system is from (by using the color I think). It can be challenging because a system could be in different schedules, and then how to display it ? :/
  • test this a bit more to make sure displayed info is correct and relevant, I'm lacking expert knowledge on the analyzed crates to be too confident.

Then (as a future Pull Request), it could open up interesting analysis:

  • detect when an event fired wouldn't be handled until next frame, a common source on "one frame delay", this should be possible by reconciliating schedule order and event order...
  • Following almost the same code, we also could offer the same kind of graph for components or resources, to understand how a data is modified, where, when.
  • parameters to filter for a specific type would be very useful to reduce the noise (in the screenshot, logs are very noisy)

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Mar 28, 2024

@jakobhellermann any preference on how to tackle styling ? My plan (started with d6ab989) is to mostly copy from the schedule graph render, adapting/removing things as I discover them.

Once we're happy with the features we'll probably be able to consider merging some data/functions, or keep them independent.

@jakobhellermann
Copy link
Owner

Yea, just copying it and letter checking if it makes sense to extract something shared is what I'd do aswell

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 1, 2024

Applying same system style as schedule graph, depending on their crate source :

graphviz(3)

Comment on lines 210 to 221
dot.add_edge(
&event_id,
&system_name,
&[
/*
("lhead", &self.lref(to)),
("ltail", &self.lref(from)),
("tooltip", &self.edge_tooltip(from, to)),
*/
("color", ctx.next_edge_color()),
],
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could use a util function in the context 🤔

Copy link
Contributor Author

@Vrixyz Vrixyz Apr 1, 2024

Choose a reason for hiding this comment

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

This whole file is copied from schedule_graph, I removed what was not necessary and added a few knobs to tweak.

This could be a future PR to evaluate shared behaviours with schedule_graph ? system_style.rs definitely ; and more controversially a few things from settings.rs

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 4, 2024

Updated with subgraphs for schedules:

mod_picking_minimal_subgraph_schedules

Now is a good time for feedback @jakobhellermann ; what's missing is probably:

  • support for systems in multiple schedules (if that's a thing 🤔)
  • some code cleanup, not too sure where, I tried to fit in the library's architecture
  • I don't quite understand what's the benefit of those edges : WIP: events graph #31 (comment) ; there's a cluster which I assume is useful to avoid "spaghetti" effect with arrows but I'm not too familiar with graphviz so your guidance is welcome.
  • update readme

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Apr 4, 2024

I like that subgraph with schedules, but there might be a problem with where to put the events: at the moment they belong to the first schedule they are encountered (from the graph list).

This leads to them appearing at different places if we change the filtering of systems or schedules.

See this unfiltered events graph

mod_picking_minimal_nofilter

Arguably, event_update_system is a special case which we could make better ; my solution to address that was to filter it out by default and expose the filter so users can build up on that.

That said, there's probably more complex projects which might be bitten by that.

I'd like to consider that future improvements as I don't see any obvious fixes right now.

@Vrixyz Vrixyz marked this pull request as ready for review April 4, 2024 13:14
@Vrixyz
Copy link
Contributor Author

Vrixyz commented May 14, 2024

As an experiment I did display the events outside of their first schedule met, it looked worse :

events outside schedules (messy)

events_outside_schedules

We could only render outside the events being "ambiguous" to different schedules, I think I like it:

events found in different schedules rendered outside

events_ambiguous_outside_schedules

I think I'll need help to take a decision here @jakobhellermann ; or add it as an option ? I'll push what I have.

We still should order the way our systems/events are iterated on to avoid having different renders on similar runs, but I'd like to keep that for another issue.

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