bevy_transform_gizmo icon indicating copy to clipboard operation
bevy_transform_gizmo copied to clipboard

Avoid spamming errors in unexpected situations

Open nicopap opened this issue 2 years ago • 3 comments

Spamming errors greatly reduces the usability of a library, if every second, up to 500 errors are printed in the console, it is as good as if the library panicked.

With this PR, bevy_transform_gizmo will only log stuff if it didn't already emit a similar message within the last second. (note that I downgraded the errors into warnings, since they fundamentally do not break stuff)

I also use system chaining for logging, because of separation of concerns.

nicopap avatar Aug 31 '22 16:08 nicopap

Thanks for the PR.

Spamming errors greatly reduces the usability of a library, if every second, up to 500 errors are printed in the console, it is as good as if the library panicked.

Is it possible that the better solution, then, is to panic?

aevyrie avatar Sep 16 '22 21:09 aevyrie

I specifically need to NOT panic. There is a few cases where I do things like remove the cast source, change the camera etc, which bevy_transform_gizmo doesn't handle all that well. My use case is a fork of bevy_editor_pls where the gizmo only shows up in editing mode. Panicking in unexpected situations would simply make such a use-case impossible.

nicopap avatar Sep 24 '22 12:09 nicopap

My position on this is:

  • Batching warnings can make debugging more difficult. It's not obvious or expected that errors should get swallowed up like this, nor is it commonly used elsewhere in the bevy ecosystem. I would prefer predictability over any convenience this adds, in part because:
  • This should not be the happy path. If you are getting so many errors that this is becoming a problem, logging isn't the issue, the root cause needs to be solved. These errors are being reported because the plugin can't function as expected while in this state.
  • If you know that you are going to be in these states, you should be disabling the plugin or removing the gizmo entity entirely. If this process is enough of a burden that you prefer not doing it, does that point to an issue with the public API of this plugin? Is there something else we need to make it easier to handle the use cases you mention?

aevyrie avatar Sep 26 '22 20:09 aevyrie

Sorry for letting this rot. I intended to reply with specific examples.

Batching warnings can make debugging more difficult. It's not obvious or expected that errors should get swallowed up like this, nor is it commonly used elsewhere in the bevy ecosystem

There is a few counter-examples:

  • The hierarchy error added in 0.9 does batch the warning message to avoid spamming it
  • Same with the "UI node has parent without Style component" warning
  • I wrote a plugin that does batching just this way https://lib.rs/crates/bevy_mod_sysfail (in fact it was inspired by this PR)

To be fair, two of those three were written by me.

you should be disabling the plugin

This might have been the thing I was missing! Well, I'll try migrating to use the disable method. I expect I won't need to silence messages with disabling. Thanks

nicopap avatar Nov 22 '22 09:11 nicopap

If this process is enough of a burden that you prefer not doing it, does that point to an issue with the public API of this plugin? Is there something else we need to make it easier to handle the use cases you mention?

If you feel that the above is true, let's reopen and see if we can improve the docs or API around disabling the plugin.

aevyrie avatar Nov 22 '22 22:11 aevyrie