-
Notifications
You must be signed in to change notification settings - Fork 36
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
base: main
Are you sure you want to change the base?
Conversation
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). |
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. |
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 I filtered out the I'd like to explore different things in this PR:
Then (as a future Pull Request), it could open up interesting analysis:
|
@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. |
Yea, just copying it and letter checking if it makes sense to extract something shared is what I'd do aswell |
src/event_graph/mod.rs
Outdated
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()), | ||
], | ||
); |
There was a problem hiding this comment.
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 🤔
src/event_graph/settings.rs
Outdated
There was a problem hiding this comment.
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
Updated with subgraphs for schedules: Now is a good time for feedback @jakobhellermann ; what's missing is probably:
|
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. Arguably, 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. |
As an experiment I did display the events outside of their first schedule met, it looked worse : We could only render outside the events being "ambiguous" to different schedules, I think I like it: 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. |
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):