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

Draft puffin_tracing support #4730

Closed
wants to merge 1 commit into from
Closed

Conversation

mvlabat
Copy link
Contributor

@mvlabat mvlabat commented May 12, 2022

Objective

The purpose of this PR is to demonstrate how puffin support can be integrated into Bevy. Currently, this PR is based on my fork of the puffin library which implements a tracing layer for puffin: EmbarkStudios/puffin#79. But regardless of whether the tracing support lands into puffin itself or it'll be an independent crate, it's possible for Bevy to integrate puffin as a tracing layer without replacing tracing with an alternative crate such as profiling or puffin itself (but it might still be worth to discuss those as well).

Solution

The current approach is to add the puffin_tracing crate (which isn't published atm) as an optional dependency and introduce a feature which passes PuffinLayer as a tracing-subscriber layer. This implies that users will have to add puffin, bevy_egui, puffin_egui crates to their apps themselves so that Bevy can avoid depending on those directly.

It would be fair to note that such integration can also exist outside of Bevy as a plugin (today, I also published one: https://github.com/mvlabat/bevy_puffin). But it doesn't offer an ideal UX, as it's incompatible with Bevy's LogPlugin, which renders it impossible to use the DefaultPlugins plugin group and makes users add those plugins manually.

Alternatively, we can also allow users to customize LogPlugin by passing their own layers.

To discuss

I noted that this change doesn't make Bevy depend on puffin itself (only on puffin_tracing, which still transitively depends on puffin though), but there still might be a valid reason to do that. For example, we can add a system which would call puffin::GlobalProfiler::lock().new_frame();. As a counter-argument, users may want to control marking a new frame themselves, but I can't imagine the use-cases for that yet.

@alice-i-cecile alice-i-cecile added A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels May 12, 2022
@mockersf
Copy link
Member

It would be fair to note that such integration can also exist outside of Bevy as a plugin (today, I also published one: https://github.com/mvlabat/bevy_puffin). But it doesn't offer an ideal UX, as it's incompatible with Bevy's LogPlugin, which renders it impossible to use the DefaultPlugins plugin group and makes users add those plugins manually.

You can use add_plugins_with to still be able to use DefaultPlugins: mvlabat/bevy_puffin#1

@mvlabat
Copy link
Contributor Author

mvlabat commented May 12, 2022

@mockersf great point, thank you! I don't know how I avoided reading the LogPlugin docs for so long, which explain exactly that.

@richchurcher
Copy link
Contributor

Backlog cleanup: closing as an inactive WIP PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Feature A new feature, making something new possible 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