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

Add helper macro's for logging only once #10808

Merged
merged 10 commits into from
Dec 5, 2023

Conversation

josfeenstra
Copy link
Contributor

@josfeenstra josfeenstra commented Nov 29, 2023

Objective

Fixes #10291

This adds a way to easily log messages once within system which are called every frame.

Solution

Opted for a macro-based approach. The fact that the 'once' call is tracked per call site makes the log_once!() macro very versatile and easy-to-use. I suspect it will be very handy for all of us, but especially beginners, to get some initial feedback from systems without spamming up the place!

I've made the macro's return its internal has_fired state, for situations in which that might be useful to know (trigger something else alongside the log, for example).

Please let me know if I placed the macro's in the right location, and if you would like me to do something more clever with the macro's themselves, since its looking quite copy-pastey at the moment. I've tried ways to replace 5 with 1 macro's, but no success yet.

One downside of this approach is: Say you wish to warn the user if a resource is invalid. In this situation, the
resource.is_valid() check would still be performed every frame:

fn my_system(my_res: Res<MyResource>) {
   if !my_res.is_valid() {
      warn_once!("resource is invalid!");
   }
}

If you want to prevent that, you would still need to introduce a local boolean. I don't think this is a very big deal, as expensive checks shouldn't be called every frame in any case.

Changelog

Added: trace_once!(), debug_once!(), info_once!(), warn_once!(), and error_once!() log macros which fire only once per call site.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

crates/bevy_log/src/once.rs Outdated Show resolved Hide resolved
crates/bevy_log/src/once.rs Outdated Show resolved Hide resolved
@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Utils Utility functions and types A-ECS Entities, components, systems, and events labels Nov 29, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Nice! I like the simplicity of this rather than more explicit options (like a Local<bool> or some other method of tracking).

crates/bevy_log/src/once.rs Outdated Show resolved Hide resolved
@josfeenstra
Copy link
Contributor Author

josfeenstra commented Nov 30, 2023

Nice! I like the simplicity of this rather than more explicit options (like a Local<bool> or some other method of tracking).

Me too @MrGVSV, and thanks for the review! What do you think of this latest change, in which the once!() mechanic is abstracted away in yet another macro?

@josfeenstra
Copy link
Contributor Author

Some remaining questions:

  • Should once!() be moved to bevy_util?
  • AtomicBool is from std. Does it work on all platforms, in particular webassembly?

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Really simple and clean. Nice stuff!

@alice-i-cecile
Copy link
Member

Should once!() be moved to bevy_util?

I'm content with where it is for now. I don't particularly want to promote this as a general-purpose tool, or add things to bevy_utils if we have another plausible choice.

AtomicBool is from std. Does it work on all platforms, in particular webassembly?

https://github.com/rust-lang/rust/issues/77839suggests that Rust can't access WASM atomic primitives, but I'm not sure if that's the same as std's atomic types simply failing to compile. It feels like falling back to a simple static in the context of a purely single-threaded program should be straightforward.

A quick test before merging would be great.

@josfeenstra
Copy link
Contributor Author

A quick test before merging would be great.

@alice-i-cecile

image

Seems to work just fine!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 4, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 5, 2023
Merged via the queue into bevyengine:main with commit 18ac125 Dec 5, 2023
23 checks passed
@josfeenstra josfeenstra deleted the log-once branch December 5, 2023 10:23
This was referenced Jan 18, 2024
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 A-Utils Utility functions and types C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a helper for logging only once
3 participants