ForesightMiningSoftwareCorporation / bevy_transform_gizmo

A 3d gizmo for transforming entities in Bevy.
Apache License 2.0
160 stars 40 forks source link

Avoid spamming errors in unexpected situations #36

Closed nicopap closed 1 year ago

nicopap commented 2 years ago

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.

aevyrie commented 2 years ago

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?

nicopap commented 1 year ago

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.

aevyrie commented 1 year ago

My position on this is:

nicopap commented 1 year ago

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:

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

aevyrie commented 1 year ago

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.