Skip to content

Conversation

@afonsolage
Copy link
Contributor

Objective

Solution

  • Added FpsOverlayPositionConfig into FpsOverlayConfig to enable fine configuration of overlay root node position.
  • Added min_color and max_color to FrameTimeGraphConfig to enable frame graph color configuration.
  • Moved text config to FpsOverlayTextConfig to enable/disable only the text part of overlay.
  • Added an "overlay-wise" enable option which would disable the entire overlay,

This PR is kinda blocked by #21022, since there is an intent to convert the fps overlay to use bevy_sprite, but I decided to do it anyways because I don't think the benefits of converting fps_overlay to bevy_sprite worth it.

The fix for #21021 may not be ideal, since it can be confusing to have 3 enable options on a simple component like fps_overlay, but I think it would be nice to be able to only use the graph of the overlay, without the text itself:

image

Testing

  • I used the fps_overlay example as testing and its working fine.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Dev-Tools Tools used to debug Bevy applications. S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 1, 2025
@alice-i-cecile alice-i-cecile added this to the 0.18 milestone Oct 1, 2025
@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 1, 2025
// Enable or disable the entire fps overlay
enabled: true,
frame_time_graph_config: FrameTimeGraphConfig {
enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of enabled , I suggest rename

  • in FrameTimeGraphConfig change enabled to graph_enabled
  • in FpsOverlayTextConfig change enabled to text_enabled

And then for FpsOverlayConfig remove enabled and replace any usage with frame_time_graph_config.graph_enabled || text_config.text_enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

Might as well rename frame_time_graph_config to graph_config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with removing enabled from FpsOverlayConfig is that if the user wanna hide the overlay, like using some toggle key, it needs to work with two nested enabled properties, like described in #21201. Ideally to hide/show there should be only one property config for the entire overlay.

I'm ok with the other suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe set_enabled(bool) could be a method that sets both text_enabled and graph_enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still keeps the problem of "verbosity", because Bevy uses a lot of "declaration config". Imagine that I wanna it disabled so I can enable it with hotkey later on:

app.add_plugins(FpsOverlayPlugin {
    config: FpsOverlayConfig {
       text_config: FpsOverlayTextConfig {
           enabled: false,
           ..default()
       },
       frame_time_graph_config: FrameTimeGraphConfig {
            enabled: false,
            ..default()
        },
        ..default()
    },
});

vs

app.add_plugins(FpsOverlayPlugin {
    config: FpsOverlayConfig {
       enabled: false,
        ..default()
    },
});

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

ran the example and LGTM

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I think it would be better if updating the position of the overlay in runtime would also update the position of the overlay. As it is now, the position is only used on spawn.
If we add this, can we also add something like changing position with arrows in the examples?

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 14, 2025
}

/// Configuration options for the FPS overlay.
#[derive(Resource, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a resource, if it's also nested inside of FpsOverlayConfig?

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.

This needs a simple migration guide, and I agree with @pablo-lua about the desirability of making this dynamically update. It's fine if we decide that's not worth the hassle, but it at least needs to be called out in the docs.

I've also noticed an errant resource derive to fix up.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 14, 2025
@alice-i-cecile alice-i-cecile removed this from the 0.18 milestone Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Dev-Tools Tools used to debug Bevy applications. C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow users to customise the colors of FrameTimeGraph The frame time graph enabling is *weird* FPS overlay: Allow users to specify positioning

6 participants