Skip to content

Add an opt-in warning on missed events #7092

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

Closed
wants to merge 16 commits into from

Conversation

CatThingy
Copy link
Contributor

Objective

Add an opt-in warning for events that get missed. Addresses #7089. Based on (and requires) #7086.

Solution

Add an associated const on the Event trait, which is set with an additional attribute macro. Only emit the warning of that associated const is true.

I believe this should have no performance impact for events that do not enable the warning, as the branch should be trivially optimized out.

Notes

Should this be inverted i.e. warn by default, add #[event(ignore_missed)]? Should the attribute be formatted in a different way? I did not add the warning to any engine events, although some should probably have them (AssetEvent comes to mind).

This implementation only allows for the warning to be enabled/disabled when the event is declared. Events sent by third-party plugins that are intended to be handled by the user are required to make a call on whether or an event must be handled, which can differ depending on the application.

I added the documentation for the attribute to Events, as that is where most of the documentation around events currently resides. Should this go on Event instead?


Changelog

Added

Users can add the #[event(warn_missed)] attribute to emit a warning if that event is not handled.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 6, 2023
@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jan 8, 2023
@alice-i-cecile
Copy link
Member

I like this quite a bit. A few nits, and this is blocked, but I think that this is a good strategy. I think we either:

a) need to show how to enable this only in debug mode
b) enable this by default for all events in debug mode and provide both opt-in and opt-out behavior

I prefer b), so you can debug third party events more easily.

We also need to benchmark this with the flag turned off (or compare the assembly) to verify there's no perf regression.

@CatThingy CatThingy force-pushed the warn-missed-events branch from 1fdfee4 to 2adff99 Compare March 6, 2023 19:46
@CatThingy CatThingy force-pushed the warn-missed-events branch from 2adff99 to 9635465 Compare March 6, 2023 19:51
@bas-ie
Copy link
Contributor

bas-ie commented Oct 13, 2024

Backlog cleanup: closing due to inactivity and ongoing WIP status, but obviously could be adopted/re-opened. Noting that the dependent PR was merged, and a tracking issue (#7089) already exists.

@bas-ie bas-ie closed this Oct 13, 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 C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants