From 5563b640445e7bd5a94ec28af245fc90b1ef4662 Mon Sep 17 00:00:00 2001 From: Ryan Walters Date: Wed, 10 Sep 2025 00:45:16 -0500 Subject: [PATCH] refactor: replace immutable Hidden component with mutable Visibility component --- src/game.rs | 8 +-- src/systems/animation/blinking.rs | 34 +++-------- src/systems/render.rs | 95 ++++++++++++++++++++++++------- src/systems/state.rs | 26 ++++----- tests/blinking.rs | 63 ++++++++++++-------- 5 files changed, 141 insertions(+), 85 deletions(-) diff --git a/src/game.rs b/src/game.rs index 0d43d07..c5b482a 100644 --- a/src/game.rs +++ b/src/game.rs @@ -16,10 +16,10 @@ use crate::systems::{ hud_render_system, item_system, linear_render_system, player_life_sprite_system, present_system, profile, time_to_live_system, touch_ui_render_system, AudioEvent, AudioResource, AudioState, BackbufferResource, Blinking, BufferedDirection, Collider, DebugState, DebugTextureResource, DeltaTime, DirectionalAnimation, EntityType, Frozen, - GameStage, Ghost, GhostAnimation, GhostAnimations, GhostBundle, GhostCollider, GhostState, GlobalState, Hidden, ItemBundle, + GameStage, Ghost, GhostAnimation, GhostAnimations, GhostBundle, GhostCollider, GhostState, GlobalState, ItemBundle, ItemCollider, LastAnimationState, LinearAnimation, MapTextureResource, MovementModifiers, NodeId, PacmanCollider, PlayerAnimation, PlayerBundle, PlayerControlled, PlayerDeathAnimation, PlayerLives, Position, RenderDirty, Renderable, - ScoreResource, StartupSequence, SystemId, SystemTimings, Timing, TouchState, Velocity, + ScoreResource, StartupSequence, SystemId, SystemTimings, Timing, TouchState, Velocity, Visibility, }; use crate::texture::animated::{DirectionalTiles, TileSequence}; @@ -148,7 +148,7 @@ impl Game { Self::configure_schedule(&mut schedule); debug!("Spawning player entity"); - world.spawn(player_bundle).insert((Frozen, Hidden)); + world.spawn(player_bundle).insert((Frozen, Visibility::hidden())); info!("Spawning game entities"); Self::spawn_ghosts(&mut world)?; @@ -601,7 +601,7 @@ impl Game { } }; - let entity = world.spawn(ghost).insert((Frozen, Hidden)).id(); + let entity = world.spawn(ghost).insert((Frozen, Visibility::hidden())).id(); trace!(ghost = ?ghost_type, entity = ?entity, start_node, "Spawned ghost entity"); } diff --git a/src/systems/animation/blinking.rs b/src/systems/animation/blinking.rs index 5498baa..ddfa0a8 100644 --- a/src/systems/animation/blinking.rs +++ b/src/systems/animation/blinking.rs @@ -1,11 +1,10 @@ use bevy_ecs::{ component::Component, - entity::Entity, query::{Has, With}, - system::{Commands, Query, Res}, + system::{Query, Res}, }; -use crate::systems::{DeltaTime, Frozen, Hidden, Renderable}; +use crate::systems::{DeltaTime, Frozen, Renderable, Visibility}; #[derive(Component, Debug)] pub struct Blinking { @@ -28,18 +27,11 @@ impl Blinking { /// accumulating ticks and toggling visibility when the specified interval is reached. /// Uses integer arithmetic for deterministic behavior. #[allow(clippy::type_complexity)] -pub fn blinking_system( - mut commands: Commands, - time: Res, - mut query: Query<(Entity, &mut Blinking, Has, Has), With>, -) { - for (entity, mut blinking, hidden, frozen) in query.iter_mut() { - // If the entity is frozen, blinking is disabled and the entity is unhidden (if it was hidden) +pub fn blinking_system(time: Res, mut query: Query<(&mut Blinking, &mut Visibility, Has), With>) { + for (mut blinking, mut visibility, frozen) in query.iter_mut() { + // If the entity is frozen, blinking is disabled and the entity is made visible if frozen { - if hidden { - commands.entity(entity).remove::(); - } - + visibility.show(); continue; } @@ -49,11 +41,7 @@ pub fn blinking_system( // Handle zero interval case (immediate toggling) if blinking.interval_ticks == 0 { if time.ticks > 0 { - if hidden { - commands.entity(entity).remove::(); - } else { - commands.entity(entity).insert(Hidden); - } + visibility.toggle(); } continue; } @@ -69,14 +57,10 @@ pub fn blinking_system( // Update the timer to the remainder after complete intervals blinking.tick_timer %= blinking.interval_ticks; - // Toggle the Hidden component for each complete interval + // Toggle the visibility for each complete interval // Since toggling twice is a no-op, we only need to toggle if the count is odd if complete_intervals % 2 == 1 { - if hidden { - commands.entity(entity).remove::(); - } else { - commands.entity(entity).insert(Hidden); - } + visibility.toggle(); } } } diff --git a/src/systems/render.rs b/src/systems/render.rs index 0757f27..b9afb31 100644 --- a/src/systems/render.rs +++ b/src/systems/render.rs @@ -8,7 +8,7 @@ use crate::texture::sprite::{AtlasTile, SpriteAtlas}; use bevy_ecs::component::Component; use bevy_ecs::entity::Entity; use bevy_ecs::event::EventWriter; -use bevy_ecs::query::{Changed, Or, With, Without}; +use bevy_ecs::query::{Changed, Or, With}; use bevy_ecs::removal_detection::RemovedComponents; use bevy_ecs::resource::Resource; use bevy_ecs::system::{NonSendMut, Query, Res, ResMut}; @@ -33,6 +33,53 @@ pub struct RenderDirty(pub bool); #[derive(Component)] pub struct Hidden; +/// A component that controls entity visibility in the render system. +/// +/// Entities without this component are considered visible by default. +/// This allows for efficient rendering where only entities that need +/// visibility control have this component. +#[derive(Component, Debug, Clone, Copy, PartialEq, Eq)] +pub struct Visibility(pub bool); + +impl Default for Visibility { + fn default() -> Self { + Self(true) // Default to visible + } +} + +impl Visibility { + /// Creates a visible Visibility component + pub fn visible() -> Self { + Self(true) + } + + /// Creates a hidden Visibility component + pub fn hidden() -> Self { + Self(false) + } + + /// Returns true if the entity is visible + pub fn is_visible(&self) -> bool { + self.0 + } + + /// Returns true if the entity is hidden + #[allow(dead_code)] // Used in tests + pub fn is_hidden(&self) -> bool { + !self.0 + } + + /// Makes the entity visible + pub fn show(&mut self) { + self.0 = true; + } + + /// Toggles the visibility state + pub fn toggle(&mut self) { + self.0 = !self.0; + } +} + /// Enum to identify which texture is being rendered to in the combined render system #[derive(Debug, Clone, Copy)] enum RenderTarget { @@ -43,15 +90,10 @@ enum RenderTarget { #[allow(clippy::type_complexity)] pub fn dirty_render_system( mut dirty: ResMut, - changed: Query<(), Or<(Changed, Changed)>>, - removed_hidden: RemovedComponents, + changed: Query<(), Or<(Changed, Changed, Changed)>>, removed_renderables: RemovedComponents, ) { - let changed_count = changed.iter().count(); - let removed_hidden_count = removed_hidden.len(); - let removed_renderables_count = removed_renderables.len(); - - if changed_count > 0 || removed_hidden_count > 0 || removed_renderables_count > 0 { + if changed.iter().count() > 0 || !removed_renderables.is_empty() { dirty.0 = true; } } @@ -77,8 +119,14 @@ pub fn render_system( map: &Res, dirty: &Res, renderables: &Query< - (Entity, &Renderable, Option<&Position>, Option<&PixelPosition>), - (Without, Or<(With, With)>), + ( + Entity, + &Renderable, + Option<&Position>, + Option<&PixelPosition>, + Option<&Visibility>, + ), + Or<(With, With)>, >, errors: &mut EventWriter, ) { @@ -95,14 +143,17 @@ pub fn render_system( errors.write(TextureError::RenderFailed(e.to_string()).into()); } - // Render all entities to the backbuffer - for (_entity, renderable, position, pixel_position) in renderables + // Collect and filter visible entities, then sort by layer + let mut visible_entities: Vec<_> = renderables .iter() - .sort_by_key::<(Entity, &Renderable, Option<&Position>, Option<&PixelPosition>), _>(|(_, renderable, _, _)| { - renderable.layer - }) - .rev() - { + .filter(|(_, _, _, _, visibility)| visibility.map(|v| v.is_visible()).unwrap_or(true)) + .collect(); + + visible_entities.sort_by_key(|(_, renderable, _, _, _)| renderable.layer); + visible_entities.reverse(); + + // Render all visible entities to the backbuffer + for (_entity, renderable, position, pixel_position, _visibility) in visible_entities { let pos = if let Some(position) = position { position.get_pixel_position(&map.graph) } else { @@ -150,8 +201,14 @@ pub fn combined_render_system( map: Res, dirty: Res, renderables: Query< - (Entity, &Renderable, Option<&Position>, Option<&PixelPosition>), - (Without, Or<(With, With)>), + ( + Entity, + &Renderable, + Option<&Position>, + Option<&PixelPosition>, + Option<&Visibility>, + ), + Or<(With, With)>, >, colliders: Query<(&Collider, &Position)>, cursor: Res, diff --git a/src/systems/state.rs b/src/systems/state.rs index bfb6074..5f35853 100644 --- a/src/systems/state.rs +++ b/src/systems/state.rs @@ -6,8 +6,8 @@ use crate::systems::SpawnTrigger; use crate::{ map::builder::Map, systems::{ - AudioEvent, Blinking, DirectionalAnimation, Dying, Eaten, Frozen, Ghost, GhostCollider, GhostState, Hidden, - LinearAnimation, Looping, NodeId, PlayerControlled, Position, + AudioEvent, Blinking, DirectionalAnimation, Dying, Eaten, Frozen, Ghost, GhostCollider, GhostState, LinearAnimation, + Looping, NodeId, PlayerControlled, Position, Visibility, }, }; use bevy_ecs::{ @@ -223,8 +223,8 @@ pub fn stage_system( } // Hide the player & eaten ghost - commands.entity(player.0).insert(Hidden); - commands.entity(ghost_entity).insert(Hidden); + commands.entity(player.0).insert(Visibility::hidden()); + commands.entity(ghost_entity).insert(Visibility::hidden()); // Spawn bonus points entity at Pac-Man's position commands.trigger(SpawnTrigger::Bonus { @@ -236,9 +236,9 @@ pub fn stage_system( } (GameStage::GhostEatenPause { ghost_entity, .. }, GameStage::Playing) => { // Unfreeze and reveal the player & all ghosts - commands.entity(player.0).remove::<(Frozen, Hidden)>(); + commands.entity(player.0).remove::().insert(Visibility::visible()); for (entity, _, _) in ghost_query.iter_mut() { - commands.entity(entity).remove::<(Frozen, Hidden)>(); + commands.entity(entity).remove::().insert(Visibility::visible()); } // Reveal the eaten ghost and switch it to Eyes state @@ -254,7 +254,7 @@ pub fn stage_system( (GameStage::PlayerDying(DyingSequence::Frozen { .. }), GameStage::PlayerDying(DyingSequence::Animating { .. })) => { // Hide the ghosts for (entity, _, _) in ghost_query.iter_mut() { - commands.entity(entity).insert(Hidden); + commands.entity(entity).insert(Visibility::hidden()); } // Start Pac-Man's death animation @@ -265,7 +265,7 @@ pub fn stage_system( } (GameStage::PlayerDying(DyingSequence::Animating { .. }), GameStage::PlayerDying(DyingSequence::Hidden { .. })) => { // Hide the player - commands.entity(player.0).insert(Hidden); + commands.entity(player.0).insert(Visibility::hidden()); } (_, GameStage::LevelRestarting) => { let (player_entity, mut pos) = player.into_inner(); @@ -275,7 +275,7 @@ pub fn stage_system( // Freeze the blinking, force them to be visible (if they were hidden by blinking) for entity in blinking_query.iter_mut() { - commands.entity(entity).insert(Frozen).remove::(); + commands.entity(entity).insert(Frozen).insert(Visibility::visible()); } // Reset the player animation @@ -296,15 +296,15 @@ pub fn stage_system( }; commands .entity(ghost_entity) - .remove::<(Frozen, Hidden, Eaten)>() - .insert(GhostState::Normal); + .remove::<(Frozen, Eaten)>() + .insert((Visibility::visible(), GhostState::Normal)); } } (_, GameStage::Starting(StartupSequence::CharactersVisible { .. })) => { // Unhide the player & ghosts - commands.entity(player.0).remove::(); + commands.entity(player.0).insert(Visibility::visible()); for (entity, _, _) in ghost_query.iter_mut() { - commands.entity(entity).remove::(); + commands.entity(entity).insert(Visibility::visible()); } } (GameStage::Starting(StartupSequence::CharactersVisible { .. }), GameStage::Playing) => { diff --git a/tests/blinking.rs b/tests/blinking.rs index f0c9e4b..f7bf8d6 100644 --- a/tests/blinking.rs +++ b/tests/blinking.rs @@ -1,5 +1,5 @@ use bevy_ecs::{entity::Entity, system::RunSystemOnce, world::World}; -use pacman::systems::{blinking_system, Blinking, DeltaTime, Frozen, Hidden, Renderable}; +use pacman::systems::{blinking_system, Blinking, DeltaTime, Frozen, Renderable, Visibility}; use speculoos::prelude::*; mod common; @@ -20,11 +20,12 @@ fn spawn_blinking_entity(world: &mut World, interval_ticks: u32) -> Entity { sprite: common::mock_atlas_tile(1), layer: 0, }, + Visibility::visible(), )) .id() } -/// Spawns a test entity with blinking, renderable, and hidden components +/// Spawns a test entity with blinking, renderable, and hidden visibility fn spawn_hidden_blinking_entity(world: &mut World, interval_ticks: u32) -> Entity { world .spawn(( @@ -33,7 +34,7 @@ fn spawn_hidden_blinking_entity(world: &mut World, interval_ticks: u32) -> Entit sprite: common::mock_atlas_tile(1), layer: 0, }, - Hidden, + Visibility::hidden(), )) .id() } @@ -47,12 +48,13 @@ fn spawn_frozen_blinking_entity(world: &mut World, interval_ticks: u32) -> Entit sprite: common::mock_atlas_tile(1), layer: 0, }, + Visibility::visible(), Frozen, )) .id() } -/// Spawns a test entity with blinking, renderable, hidden, and frozen components +/// Spawns a test entity with blinking, renderable, hidden visibility, and frozen components fn spawn_frozen_hidden_blinking_entity(world: &mut World, interval_ticks: u32) -> Entity { world .spawn(( @@ -61,7 +63,7 @@ fn spawn_frozen_hidden_blinking_entity(world: &mut World, interval_ticks: u32) - sprite: common::mock_atlas_tile(1), layer: 0, }, - Hidden, + Visibility::hidden(), Frozen, )) .id() @@ -73,9 +75,22 @@ fn run_blinking_system(world: &mut World, delta_ticks: u32) { world.run_system_once(blinking_system).unwrap(); } -/// Checks if an entity has the Hidden component -fn has_hidden_component(world: &World, entity: Entity) -> bool { - world.entity(entity).contains::() +/// Checks if an entity is visible +fn is_entity_visible(world: &World, entity: Entity) -> bool { + world + .entity(entity) + .get::() + .map(|v| v.is_visible()) + .unwrap_or(true) // Default to visible if no Visibility component +} + +/// Checks if an entity is hidden +fn is_entity_hidden(world: &World, entity: Entity) -> bool { + world + .entity(entity) + .get::() + .map(|v| v.is_hidden()) + .unwrap_or(false) // Default to visible if no Visibility component } /// Checks if an entity has the Frozen component @@ -100,7 +115,7 @@ fn test_blinking_system_normal_interval_no_toggle() { run_blinking_system(&mut world, 3); // Entity should not be hidden yet - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); // Check that timer was updated let blinking = world.entity(entity).get::().unwrap(); @@ -116,7 +131,7 @@ fn test_blinking_system_normal_interval_first_toggle() { run_blinking_system(&mut world, 5); // Entity should now be hidden - assert_that(&has_hidden_component(&world, entity)).is_true(); + assert_that(&is_entity_hidden(&world, entity)).is_true(); // Check that timer was reset let blinking = world.entity(entity).get::().unwrap(); @@ -130,11 +145,11 @@ fn test_blinking_system_normal_interval_second_toggle() { // First toggle: 5 ticks run_blinking_system(&mut world, 5); - assert_that(&has_hidden_component(&world, entity)).is_true(); + assert_that(&is_entity_hidden(&world, entity)).is_true(); // Second toggle: another 5 ticks run_blinking_system(&mut world, 5); - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); } #[test] @@ -146,7 +161,7 @@ fn test_blinking_system_normal_interval_multiple_intervals() { run_blinking_system(&mut world, 7); // Should toggle twice (even number), so back to original state (not hidden) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); // Check that timer was updated to remainder let blinking = world.entity(entity).get::().unwrap(); @@ -162,7 +177,7 @@ fn test_blinking_system_normal_interval_odd_intervals() { run_blinking_system(&mut world, 5); // Should toggle twice (even number), so back to original state (not hidden) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); // Check that timer was updated to remainder let blinking = world.entity(entity).get::().unwrap(); @@ -178,7 +193,7 @@ fn test_blinking_system_zero_interval_with_ticks() { run_blinking_system(&mut world, 1); // Entity should be hidden immediately - assert_that(&has_hidden_component(&world, entity)).is_true(); + assert_that(&is_entity_hidden(&world, entity)).is_true(); } #[test] @@ -190,7 +205,7 @@ fn test_blinking_system_zero_interval_no_ticks() { run_blinking_system(&mut world, 0); // Entity should not be hidden (no time passed) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); } #[test] @@ -202,7 +217,7 @@ fn test_blinking_system_zero_interval_toggle_back() { run_blinking_system(&mut world, 1); // Entity should be unhidden - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); } #[test] @@ -214,7 +229,7 @@ fn test_blinking_system_frozen_entity_unhidden() { run_blinking_system(&mut world, 10); // Frozen entity should be unhidden and stay unhidden - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); assert_that(&has_frozen_component(&world, entity)).is_true(); } @@ -227,7 +242,7 @@ fn test_blinking_system_frozen_entity_no_blinking() { run_blinking_system(&mut world, 10); // Frozen entity should not be hidden (blinking disabled) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); assert_that(&has_frozen_component(&world, entity)).is_true(); } @@ -255,7 +270,7 @@ fn test_blinking_system_entity_without_renderable_ignored() { run_blinking_system(&mut world, 10); // Entity should not be affected (not in query) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); } #[test] @@ -274,7 +289,7 @@ fn test_blinking_system_entity_without_blinking_ignored() { run_blinking_system(&mut world, 10); // Entity should not be affected (not in query) - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); } #[test] @@ -286,7 +301,7 @@ fn test_blinking_system_large_interval() { run_blinking_system(&mut world, 500); // Entity should not be hidden yet - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); // Check that timer was updated let blinking = world.entity(entity).get::().unwrap(); @@ -302,11 +317,11 @@ fn test_blinking_system_very_small_interval() { run_blinking_system(&mut world, 1); // Entity should be hidden - assert_that(&has_hidden_component(&world, entity)).is_true(); + assert_that(&is_entity_hidden(&world, entity)).is_true(); // Run system with another 1 tick run_blinking_system(&mut world, 1); // Entity should be unhidden - assert_that(&has_hidden_component(&world, entity)).is_false(); + assert_that(&is_entity_visible(&world, entity)).is_true(); }