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 @@ -29,6 +29,7 @@ bevy_input = { path = "../bevy_input", version = "0.14.0-dev" }
# other
serde = { version = "1.0", features = ["derive"], optional = true }
ron = { version = "0.8.0", optional = true }
uuid = { version = "1.0", features = ["v4"] }

[lints]
workspace = true
Expand Down
35 changes: 34 additions & 1 deletion crates/bevy_dev_tools/src/fps_overlay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@ use bevy_ecs::{
schedule::{common_conditions::resource_changed, IntoSystemConfigs},
system::{Commands, Query, Res, Resource},
};
use bevy_render::view::Visibility;
use bevy_text::{Font, Text, TextSection, TextStyle};
use bevy_ui::node_bundles::TextBundle;

use crate::{DevTool, DevToolId, DevToolsStore, RegisterDevTool};

/// A plugin that adds an FPS overlay to the Bevy application.
///
/// This plugin will add the [`FrameTimeDiagnosticsPlugin`] if it wasn't added before.
Expand All @@ -26,19 +29,31 @@ pub struct FpsOverlayPlugin {
pub config: FpsOverlayConfig,
}

impl FpsOverlayPlugin {
/// [`DevToolId`] of the `FpsOverlayPlugin`.
pub const FPS_OVERLAY_ID: DevToolId =
DevToolId::from_u128(66095722480681667677084752066997432964);
}

impl Plugin for FpsOverlayPlugin {
fn build(&self, app: &mut bevy_app::App) {
// TODO: Use plugin dependencies, see https://github.com/bevyengine/bevy/issues/69
if !app.is_plugin_added::<FrameTimeDiagnosticsPlugin>() {
app.add_plugins(FrameTimeDiagnosticsPlugin);
}
app.insert_resource(self.config.clone())
.register_dev_tool(DevTool::new(Self::FPS_OVERLAY_ID))
.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.

update_text.run_if(|dev_tools: Res<DevToolsStore>| {
dev_tools
.get(&Self::FPS_OVERLAY_ID)
.is_some_and(|dev_tool| dev_tool.is_enabled)
}),
),
);
}
Expand Down Expand Up @@ -96,3 +111,21 @@ fn customize_text(
}
}
}

fn change_visibility(
mut query: Query<&mut Visibility, With<FpsText>>,
dev_tools: Res<DevToolsStore>,
) {
if dev_tools
.get(&FpsOverlayPlugin::FPS_OVERLAY_ID)
.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;
}
}
}
111 changes: 111 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,9 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]

use bevy_app::prelude::*;
use bevy_ecs::system::Resource;
use bevy_utils::HashMap;
use uuid::Uuid;

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

/// Unique identifier for [`DevTool`].
#[derive(Debug, Hash, Eq, PartialEq, Clone)]
pub struct DevToolId(pub Uuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering: Why not use a TypeId map instead of Uuid? I think its much more simple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to identify each dev tool with a different type? We could use plugin struct for it, but what if plugin adds more than one dev tool?

Copy link
Contributor

@pablo-lua pablo-lua Mar 17, 2024

Choose a reason for hiding this comment

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

I was expecting to identify every tool with a single config type registered to it, but I see that's a similar approach taken by the gizmos multiple configs, and we aren't storing configs in this map by now, so, with what we have now on this PR, the TypeId doesn't make sense.
I can see number of reasons why offering a config type though. For example, we might want to do more than enable and disable things, we might want this DevToolsStore to store entire configurations in the future, like storing the FpsOverlayConfig here, and let the user refer to it from the TypeId (which can be easily created and so).


impl DevToolId {
/// Creates a [`DevToolId`] from u128.
pub const fn from_u128(value: u128) -> DevToolId {
DevToolId(Uuid::from_u128(value))
}
}

/// Information about dev tool.
#[derive(Debug)]
pub struct DevTool {
/// Identifier of a dev tool.
pub id: DevToolId,
is_enabled: bool,
}

impl DevTool {
/// 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 DevTool {
/// Creates a new [`DevTool`] from a specified [`DevToolId`].
/// New tool is enabled by default.
pub fn new(id: DevToolId) -> DevTool {
DevTool {
id,
is_enabled: true,
}
}
}

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

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

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

/// Returns a [`DevTool`].
pub fn get(&self, id: &DevToolId) -> Option<&DevTool> {
self.dev_tools.get(id)
}

/// Returns a mutable [`DevTool`].
pub fn get_mut(&mut self, id: &DevToolId) -> Option<&mut DevTool> {
self.dev_tools.get_mut(id)
}

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

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

/// Extend [`App`] with new `register_dev_tool` function.
pub trait RegisterDevTool {
/// Registers a new [`DevTool`].
fn register_dev_tool(&mut self, dev_tool: DevTool) -> &mut Self;
}

impl RegisterDevTool for App {
fn register_dev_tool(&mut self, dev_tool: DevTool) -> &mut Self {
let mut dev_tools = self
.world
.get_resource_or_insert_with::<DevToolsStore>(Default::default);
dev_tools.add(dev_tool);
self
}
}