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

Abstraction dev tools #12427

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions crates/bevy_dev_tools/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ bevy_text = { path = "../bevy_text", version = "0.14.0-dev" }
bevy_diagnostic = { path = "../bevy_diagnostic", version = "0.14.0-dev" }
bevy_color = { path = "../bevy_color", version = "0.14.0-dev" }
bevy_input = { path = "../bevy_input", version = "0.14.0-dev" }
bevy_reflect = { path = "../bevy_reflect", version = "0.14.0-dev" }

# other
serde = { version = "1.0", features = ["derive"], optional = true }
Expand Down
72 changes: 58 additions & 14 deletions crates/bevy_dev_tools/src/fps_overlay.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Module containing logic for FPS overlay.

use std::any::TypeId;

use bevy_app::{Plugin, Startup, Update};
use bevy_asset::Handle;
use bevy_color::Color;
Expand All @@ -8,10 +10,15 @@ use bevy_ecs::{
component::Component,
query::With,
schedule::{common_conditions::resource_changed, IntoSystemConfigs},
system::{Commands, Query, Res, Resource},
system::{Commands, Query, Res},
};
use bevy_reflect::Reflect;
use bevy_render::view::Visibility;
use bevy_text::{Font, Text, TextSection, TextStyle};
use bevy_ui::node_bundles::TextBundle;
use bevy_utils::warn_once;

use crate::{DevTool, DevToolApp, DevToolsStore};

/// A plugin that adds an FPS overlay to the Bevy application.
///
Expand All @@ -22,7 +29,7 @@ use bevy_ui::node_bundles::TextBundle;
/// - **Metal**: setting env variable `MTL_HUD_ENABLED=1`
#[derive(Default)]
pub struct FpsOverlayPlugin {
/// Starting configuration of overlay, this can be later be changed through [`FpsOverlayConfig`] resource.
/// Starting configuration of overlay, this can be later be changed through [`DevToolsStore`] resource.
pub config: FpsOverlayConfig,
}

Expand All @@ -32,25 +39,31 @@ impl Plugin for FpsOverlayPlugin {
if !app.is_plugin_added::<FrameTimeDiagnosticsPlugin>() {
app.add_plugins(FrameTimeDiagnosticsPlugin);
}
app.insert_resource(self.config.clone())
app.insert_dev_tool(self.config.clone())
.add_systems(Startup, setup)
.add_systems(
Update,
(
customize_text.run_if(resource_changed::<FpsOverlayConfig>),
update_text,
change_visibility.run_if(resource_changed::<DevToolsStore>),
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like this. This will run every time any tool has been changed, but I can't think of a solution to only run if a specific tool has been changed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can relax on that somehow by just making a global system that manages this, based on one-shot systems, I'm not sure how effective this can be do though. At the same time, that would be useless without some kind o cache to store what was changed and what was not.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we had some kind of cache storing what was changed, I don't think there would be any reason for complicating it with the global system. I'd just let the user interact with the cache and use it in a run_if condition.

Copy link
Member

@alice-i-cecile alice-i-cecile Mar 17, 2024

Choose a reason for hiding this comment

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

I think this is a fine level of granularity for now: I wouldn't expect DevToolsStore to change regularly at all.

If it becomes a problem we can also cache a Local within the system to short-circuit.

(customize_text, update_text).run_if(|dev_tools: Res<DevToolsStore>| {
dev_tools
.get(&TypeId::of::<FpsOverlayConfig>())
.is_some_and(|dev_tool| dev_tool.is_enabled)
}),
),
);
}
}

/// Configuration options for the FPS overlay.
#[derive(Resource, Clone)]
#[derive(Clone, Debug, Reflect)]
pub struct FpsOverlayConfig {
/// Configuration of text in the overlay.
pub text_config: TextStyle,
}

impl DevTool for FpsOverlayConfig {}

impl Default for FpsOverlayConfig {
fn default() -> Self {
FpsOverlayConfig {
Expand All @@ -66,11 +79,19 @@ impl Default for FpsOverlayConfig {
#[derive(Component)]
struct FpsText;

fn setup(mut commands: Commands, overlay_config: Res<FpsOverlayConfig>) {
fn setup(mut commands: Commands, dev_tools: Res<DevToolsStore>) {
let Some(dev_tool) = dev_tools.get(&TypeId::of::<FpsOverlayConfig>()) else {
warn_once!("Dev tool with TypeId of FpsOverlayConfig does not exist in DevToolsStore. Fps overlay won't be created");
return;
};
let Some(tool_config) = dev_tool.get_tool_config::<FpsOverlayConfig>() else {
warn_once!("Failed to get tool config from dev tool. Fps overlay won't be created");
return;
};
commands.spawn((
TextBundle::from_sections([
TextSection::new("FPS: ", overlay_config.text_config.clone()),
TextSection::from_style(overlay_config.text_config.clone()),
TextSection::new("FPS: ", tool_config.text_config.clone()),
TextSection::from_style(tool_config.text_config.clone()),
]),
FpsText,
));
Expand All @@ -86,13 +107,36 @@ fn update_text(diagnostic: Res<DiagnosticsStore>, mut query: Query<&mut Text, Wi
}
}

fn customize_text(
overlay_config: Res<FpsOverlayConfig>,
mut query: Query<&mut Text, With<FpsText>>,
) {
fn customize_text(dev_tools: Res<DevToolsStore>, mut query: Query<&mut Text, With<FpsText>>) {
for mut text in &mut query {
for section in text.sections.iter_mut() {
section.style = overlay_config.text_config.clone();
let Some(dev_tool) = dev_tools.get(&TypeId::of::<FpsOverlayConfig>()) else {
warn_once!("Dev tool with TypeId of FpsOverlayConfig does not exist in DevToolsStore. You won't be able to customize the overlay");
return;
};
let Some(tool_config) = dev_tool.get_tool_config::<FpsOverlayConfig>() else {
warn_once!("Failed to get tool config from dev tool. Fps overlay won't be created. You won't be able to customize the overlay");
return;
};
section.style = tool_config.text_config.clone();
}
}
}

fn change_visibility(
mut query: Query<&mut Visibility, With<FpsText>>,
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
dev_tools: Res<DevToolsStore>,
) {
if dev_tools
.get(&TypeId::of::<FpsOverlayConfig>())
.is_some_and(|dev_tool| dev_tool.is_enabled)
{
for mut visibility in query.iter_mut() {
*visibility = Visibility::Visible;
}
} else {
for mut visibility in query.iter_mut() {
*visibility = Visibility::Hidden;
}
}
}
129 changes: 129 additions & 0 deletions crates/bevy_dev_tools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]

use bevy_app::prelude::*;
use bevy_ecs::system::Resource;
use bevy_reflect::Reflect;
use bevy_utils::HashMap;
use std::{any::TypeId, fmt::Debug};

#[cfg(feature = "bevy_ci_testing")]
pub mod ci_testing;
Expand Down Expand Up @@ -45,3 +49,128 @@ impl Plugin for DevToolsPlugin {
}
}
}

/// Trait implemented for every dev tool.
pub trait DevTool: Sync + Send + Debug + Reflect + 'static {}

/// Information about dev tool.
#[derive(Debug)]
pub struct DevToolConfig {
/// Identifier of a dev tool.
pub id: TypeId,
/// Tool specific configuration.
tool_config: Box<dyn DevTool>,
is_enabled: bool,
}

impl DevToolConfig {
/// Returns true if [`DevTool`] is enabled.
pub fn is_enabled(&self) -> bool {
self.is_enabled
}

/// Enables [`DevTool`].
pub fn enable(&mut self) {
self.is_enabled = true;
}

/// Disables
pub fn disable(&mut self) {
self.is_enabled = false;
}

/// Toggles [`DevTool`].
pub fn toggle(&mut self) {
self.is_enabled = !self.is_enabled;
}
}

impl DevToolConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why those two impl Blocks aren't together?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think we might want helper methods to return the inner config as &dyn Reflect and &mut dyn Reflect.

/// Creates a new [`DevTool`] from a specified [`TypeId`].
/// New tool is enabled by default.
pub fn new(id: TypeId, tool_config: impl DevTool) -> DevToolConfig {
DevToolConfig {
id,
tool_config: Box::new(tool_config),
is_enabled: true,
}
}

/// Returns a tool specific configuration.
pub fn get_tool_config<D: DevTool + 'static>(&self) -> Option<&D> {
self.tool_config.as_any().downcast_ref::<D>()
}

/// Returns a mutable tool specific configuration.
pub fn get_tool_config_mut<D: DevTool + 'static>(&mut self) -> Option<&mut D> {
self.tool_config.as_any_mut().downcast_mut::<D>()
}
}

/// A collection of [`DevTool`]s.
#[derive(Resource, Default, Debug)]
pub struct DevToolsStore {
dev_tools: HashMap<TypeId, DevToolConfig>,
}

impl DevToolsStore {
/// Adds a new [`DevTool`].
///
/// If possible, prefer calling [`App::init_dev_tool`] or [`App::insert_dev_tool`].
pub fn add(&mut self, dev_tool: DevToolConfig) {
self.dev_tools.insert(dev_tool.id, dev_tool);
}

/// Removes a [`DevTool`].
pub fn remove(&mut self, id: &TypeId) {
self.dev_tools.remove(id);
}

/// Returns a reference to the given [`DevTool`] if present.
pub fn get(&self, id: &TypeId) -> Option<&DevToolConfig> {
self.dev_tools.get(id)
}

/// Returns a mutable reference to the given [`DevTool`] if present.
pub fn get_mut(&mut self, id: &TypeId) -> Option<&mut DevToolConfig> {
self.dev_tools.get_mut(id)
}

/// Returns an iterator over all [`DevTool`]s, by reference.
pub fn iter(&self) -> impl Iterator<Item = &DevToolConfig> {
self.dev_tools.values()
}

/// Returns an iterator over all [`DevTool`]s, by mutable reference.
pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut DevToolConfig> {
self.dev_tools.values_mut()
}
}

/// Extends [`App`] with new `init_dev_tool` and `insert_dev_tool` functions.
pub trait DevToolApp {
/// Initialize a new [`DevTool`].
fn init_dev_tool<D: DevTool + Default>(&mut self) -> &mut Self;
/// Insert a new [`DevTool`] with configuration.
fn insert_dev_tool<D: DevTool>(&mut self, value: D) -> &mut Self;
}

impl DevToolApp for App {
fn init_dev_tool<D: DevTool + Default>(&mut self) -> &mut Self {
let dev_tool = DevToolConfig::new(TypeId::of::<D>(), D::default());
let mut dev_tools = self
.world
.get_resource_or_insert_with::<DevToolsStore>(Default::default);
dev_tools.add(dev_tool);
self
}

fn insert_dev_tool<D: DevTool>(&mut self, value: D) -> &mut Self {
let dev_tool = DevToolConfig::new(TypeId::of::<D>(), value);
Comment on lines +168 to +169
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn insert_dev_tool<D: DevTool>(&mut self, value: D) -> &mut Self {
let dev_tool = DevToolConfig::new(TypeId::of::<D>(), value);
fn insert_dev_tool<D: DevTool>(&mut self, tool_config: D) -> &mut Self {
let dev_tool = DevToolConfig::new(TypeId::of::<D>(), tool_config);

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a better naming here, but can be a opinion question.

let mut dev_tools = self
.world
.get_resource_or_insert_with::<DevToolsStore>(Default::default);
dev_tools.add(dev_tool);
self
}
}
25 changes: 20 additions & 5 deletions examples/dev_tools/fps_overlay.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
//! Showcase how to use and configure FPS overlay.

use std::any::TypeId;

use bevy::{
dev_tools::fps_overlay::{FpsOverlayConfig, FpsOverlayPlugin},
dev_tools::{
fps_overlay::{FpsOverlayConfig, FpsOverlayPlugin},
DevToolsStore,
},
prelude::*,
};

Expand Down Expand Up @@ -54,12 +59,22 @@ fn setup(mut commands: Commands) {
);
}

fn customize_config(input: Res<ButtonInput<KeyCode>>, mut overlay: ResMut<FpsOverlayConfig>) {
fn customize_config(input: Res<ButtonInput<KeyCode>>, mut dev_tools: ResMut<DevToolsStore>) {
// We try to get mutable reference to fps overlay dev tool. Otherwise we don't do anything
let Some(dev_tool) = dev_tools.get_mut(&TypeId::of::<FpsOverlayConfig>()) else {
return;
};

// We try to access configuration struct that is specific to this dev tool.
let Some(tool_config) = dev_tool.get_tool_config_mut::<FpsOverlayConfig>() else {
return;
};

if input.just_pressed(KeyCode::Digit1) {
// Changing resource will affect overlay
overlay.text_config.color = Color::srgb(1.0, 0.0, 0.0);
// Changing tool_config will affect overlay
tool_config.text_config.color = Color::srgb(1.0, 0.0, 0.0);
}
if input.just_pressed(KeyCode::Digit2) {
overlay.text_config.font_size -= 2.0;
tool_config.text_config.font_size -= 2.0;
}
}