From c95e5ac7efe29e22aa546d31845e98efdfab4857 Mon Sep 17 00:00:00 2001 From: scawful Date: Mon, 13 Oct 2025 15:14:01 -0400 Subject: [PATCH] refactor(editor): streamline ImGui card management across various editors - Refactored multiple editor classes to ensure that ImGui::End() is always called after ImGui::Begin(), enhancing state management and preventing potential rendering issues. - Updated the DungeonEditor, GraphicsEditor, ScreenEditor, MessageEditor, MusicEditor, and SpriteEditor to follow this pattern, improving code consistency and reliability. Benefits: - Improves the stability of the editor UI by ensuring proper handling of ImGui state. - Enhances code readability and maintainability by standardizing the usage of ImGui functions across different editor components. --- src/app/editor/dungeon/dungeon_editor_v2.cc | 266 ++++++++++---------- src/app/editor/graphics/graphics_editor.cc | 9 +- src/app/editor/graphics/screen_editor.cc | 13 +- src/app/editor/message/message_editor.cc | 8 +- src/app/editor/music/music_editor.cc | 6 +- src/app/editor/sprite/sprite_editor.cc | 5 +- src/app/gui/app/editor_layout.cc | 21 +- src/app/gui/app/editor_layout.h | 5 +- 8 files changed, 168 insertions(+), 165 deletions(-) diff --git a/src/app/editor/dungeon/dungeon_editor_v2.cc b/src/app/editor/dungeon/dungeon_editor_v2.cc index 82302112..39bcb4b3 100644 --- a/src/app/editor/dungeon/dungeon_editor_v2.cc +++ b/src/app/editor/dungeon/dungeon_editor_v2.cc @@ -443,58 +443,56 @@ void DungeonEditorV2::DrawRoomsListCard() { if (selector_card.Begin()) { if (!rom_ || !rom_->is_loaded()) { ImGui::Text("ROM not loaded"); - selector_card.End(); - return; - } - - // Add text filter - static char room_filter[256] = ""; - ImGui::SetNextItemWidth(-1); - if (ImGui::InputTextWithHint("##RoomFilter", ICON_MD_SEARCH " Filter rooms...", - room_filter, sizeof(room_filter))) { - // Filter updated - } - - ImGui::Separator(); - - // Scrollable room list - simple and reliable - if (ImGui::BeginChild("##RoomsList", ImVec2(0, 0), true, - ImGuiWindowFlags_AlwaysVerticalScrollbar)) { - std::string filter_str = room_filter; - std::transform(filter_str.begin(), filter_str.end(), filter_str.begin(), ::tolower); + } else { + // Add text filter + static char room_filter[256] = ""; + ImGui::SetNextItemWidth(-1); + if (ImGui::InputTextWithHint("##RoomFilter", ICON_MD_SEARCH " Filter rooms...", + room_filter, sizeof(room_filter))) { + // Filter updated + } - for (int i = 0; i < zelda3::NumberOfRooms; i++) { - // Get room name - std::string room_name; - if (i < static_cast(std::size(zelda3::kRoomNames))) { - room_name = std::string(zelda3::kRoomNames[i]); - } else { - room_name = absl::StrFormat("Room %03X", i); - } + ImGui::Separator(); + + // Scrollable room list - simple and reliable + if (ImGui::BeginChild("##RoomsList", ImVec2(0, 0), true, + ImGuiWindowFlags_AlwaysVerticalScrollbar)) { + std::string filter_str = room_filter; + std::transform(filter_str.begin(), filter_str.end(), filter_str.begin(), ::tolower); - // Apply filter - if (!filter_str.empty()) { - std::string name_lower = room_name; - std::transform(name_lower.begin(), name_lower.end(), - name_lower.begin(), ::tolower); - if (name_lower.find(filter_str) == std::string::npos) { - continue; + for (int i = 0; i < zelda3::NumberOfRooms; i++) { + // Get room name + std::string room_name; + if (i < static_cast(std::size(zelda3::kRoomNames))) { + room_name = std::string(zelda3::kRoomNames[i]); + } else { + room_name = absl::StrFormat("Room %03X", i); + } + + // Apply filter + if (!filter_str.empty()) { + std::string name_lower = room_name; + std::transform(name_lower.begin(), name_lower.end(), + name_lower.begin(), ::tolower); + if (name_lower.find(filter_str) == std::string::npos) { + continue; + } + } + + // Simple selectable with room ID and name + std::string label = absl::StrFormat("[%03X] %s", i, room_name.c_str()); + bool is_selected = (current_room_id_ == i); + + if (ImGui::Selectable(label.c_str(), is_selected)) { + OnRoomSelected(i); + } + + if (ImGui::IsItemHovered() && ImGui::IsMouseDoubleClicked(0)) { + OnRoomSelected(i); } } - - // Simple selectable with room ID and name - std::string label = absl::StrFormat("[%03X] %s", i, room_name.c_str()); - bool is_selected = (current_room_id_ == i); - - if (ImGui::Selectable(label.c_str(), is_selected)) { - OnRoomSelected(i); - } - - if (ImGui::IsItemHovered() && ImGui::IsMouseDoubleClicked(0)) { - OnRoomSelected(i); - } + ImGui::EndChild(); } - ImGui::EndChild(); } } selector_card.End(); @@ -510,92 +508,90 @@ void DungeonEditorV2::DrawEntrancesListCard() { if (entrances_card.Begin()) { if (!rom_ || !rom_->is_loaded()) { ImGui::Text("ROM not loaded"); - entrances_card.End(); - return; - } - - // Full entrance configuration UI (matching dungeon_room_selector layout) - auto& current_entrance = entrances_[current_entrance_id_]; - - gui::InputHexWord("Entrance ID", ¤t_entrance.entrance_id_); - gui::InputHexWord("Room ID", reinterpret_cast(¤t_entrance.room_)); - ImGui::SameLine(); - gui::InputHexByte("Dungeon ID", ¤t_entrance.dungeon_id_, 50.f, true); - - gui::InputHexByte("Blockset", ¤t_entrance.blockset_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("Music", ¤t_entrance.music_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("Floor", ¤t_entrance.floor_); - - ImGui::Separator(); - - gui::InputHexWord("Player X ", ¤t_entrance.x_position_); - ImGui::SameLine(); - gui::InputHexWord("Player Y ", ¤t_entrance.y_position_); - - gui::InputHexWord("Camera X", ¤t_entrance.camera_trigger_x_); - ImGui::SameLine(); - gui::InputHexWord("Camera Y", ¤t_entrance.camera_trigger_y_); - - gui::InputHexWord("Scroll X ", ¤t_entrance.camera_x_); - ImGui::SameLine(); - gui::InputHexWord("Scroll Y ", ¤t_entrance.camera_y_); - - gui::InputHexWord("Exit", reinterpret_cast(¤t_entrance.exit_), 50.f, true); - - ImGui::Separator(); - ImGui::Text("Camera Boundaries"); - ImGui::Separator(); - ImGui::Text("\t\t\t\t\tNorth East South West"); - - gui::InputHexByte("Quadrant", ¤t_entrance.camera_boundary_qn_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##QE", ¤t_entrance.camera_boundary_qe_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##QS", ¤t_entrance.camera_boundary_qs_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##QW", ¤t_entrance.camera_boundary_qw_, 50.f, true); - - gui::InputHexByte("Full room", ¤t_entrance.camera_boundary_fn_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##FE", ¤t_entrance.camera_boundary_fe_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##FS", ¤t_entrance.camera_boundary_fs_, 50.f, true); - ImGui::SameLine(); - gui::InputHexByte("##FW", ¤t_entrance.camera_boundary_fw_, 50.f, true); - - ImGui::Separator(); - - // Entrance list - simple and reliable - if (ImGui::BeginChild("##EntrancesList", ImVec2(0, 0), true, - ImGuiWindowFlags_AlwaysVerticalScrollbar)) { - for (int i = 0; i < 0x8C; i++) { - // The last seven are spawn points - std::string entrance_name; - if (i < 0x85) { - entrance_name = std::string(zelda3::kEntranceNames[i]); - } else { - entrance_name = absl::StrFormat("Spawn Point %d", i - 0x85); - } - - // Get associated room name - int room_id = entrances_[i].room_; - std::string room_name = "Unknown"; - if (room_id >= 0 && room_id < static_cast(std::size(zelda3::kRoomNames))) { - room_name = std::string(zelda3::kRoomNames[room_id]); - } - - std::string label = absl::StrFormat("[%02X] %s -> %s", - i, entrance_name.c_str(), room_name.c_str()); - - bool is_selected = (current_entrance_id_ == i); - if (ImGui::Selectable(label.c_str(), is_selected)) { - current_entrance_id_ = i; - OnEntranceSelected(i); + } else { + // Full entrance configuration UI (matching dungeon_room_selector layout) + auto& current_entrance = entrances_[current_entrance_id_]; + + gui::InputHexWord("Entrance ID", ¤t_entrance.entrance_id_); + gui::InputHexWord("Room ID", reinterpret_cast(¤t_entrance.room_)); + ImGui::SameLine(); + gui::InputHexByte("Dungeon ID", ¤t_entrance.dungeon_id_, 50.f, true); + + gui::InputHexByte("Blockset", ¤t_entrance.blockset_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("Music", ¤t_entrance.music_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("Floor", ¤t_entrance.floor_); + + ImGui::Separator(); + + gui::InputHexWord("Player X ", ¤t_entrance.x_position_); + ImGui::SameLine(); + gui::InputHexWord("Player Y ", ¤t_entrance.y_position_); + + gui::InputHexWord("Camera X", ¤t_entrance.camera_trigger_x_); + ImGui::SameLine(); + gui::InputHexWord("Camera Y", ¤t_entrance.camera_trigger_y_); + + gui::InputHexWord("Scroll X ", ¤t_entrance.camera_x_); + ImGui::SameLine(); + gui::InputHexWord("Scroll Y ", ¤t_entrance.camera_y_); + + gui::InputHexWord("Exit", reinterpret_cast(¤t_entrance.exit_), 50.f, true); + + ImGui::Separator(); + ImGui::Text("Camera Boundaries"); + ImGui::Separator(); + ImGui::Text("\t\t\t\t\tNorth East South West"); + + gui::InputHexByte("Quadrant", ¤t_entrance.camera_boundary_qn_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##QE", ¤t_entrance.camera_boundary_qe_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##QS", ¤t_entrance.camera_boundary_qs_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##QW", ¤t_entrance.camera_boundary_qw_, 50.f, true); + + gui::InputHexByte("Full room", ¤t_entrance.camera_boundary_fn_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##FE", ¤t_entrance.camera_boundary_fe_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##FS", ¤t_entrance.camera_boundary_fs_, 50.f, true); + ImGui::SameLine(); + gui::InputHexByte("##FW", ¤t_entrance.camera_boundary_fw_, 50.f, true); + + ImGui::Separator(); + + // Entrance list - simple and reliable + if (ImGui::BeginChild("##EntrancesList", ImVec2(0, 0), true, + ImGuiWindowFlags_AlwaysVerticalScrollbar)) { + for (int i = 0; i < 0x8C; i++) { + // The last seven are spawn points + std::string entrance_name; + if (i < 0x85) { + entrance_name = std::string(zelda3::kEntranceNames[i]); + } else { + entrance_name = absl::StrFormat("Spawn Point %d", i - 0x85); + } + + // Get associated room name + int room_id = entrances_[i].room_; + std::string room_name = "Unknown"; + if (room_id >= 0 && room_id < static_cast(std::size(zelda3::kRoomNames))) { + room_name = std::string(zelda3::kRoomNames[room_id]); + } + + std::string label = absl::StrFormat("[%02X] %s -> %s", + i, entrance_name.c_str(), room_name.c_str()); + + bool is_selected = (current_entrance_id_ == i); + if (ImGui::Selectable(label.c_str(), is_selected)) { + current_entrance_id_ = i; + OnEntranceSelected(i); + } } + ImGui::EndChild(); } - ImGui::EndChild(); } } entrances_card.End(); @@ -757,12 +753,8 @@ void DungeonEditorV2::DrawRoomGraphicsCard() { if (graphics_card.Begin()) { if (!rom_ || !rom_->is_loaded()) { ImGui::Text("ROM not loaded"); - graphics_card.End(); - return; - } - - // Show graphics for current room - if (current_room_id_ >= 0 && current_room_id_ < static_cast(rooms_.size())) { + } else if (current_room_id_ >= 0 && current_room_id_ < static_cast(rooms_.size())) { + // Show graphics for current room auto& room = rooms_[current_room_id_]; ImGui::Text("Room %03X Graphics", current_room_id_); diff --git a/src/app/editor/graphics/graphics_editor.cc b/src/app/editor/graphics/graphics_editor.cc index 6a0ed8eb..edfd8e8c 100644 --- a/src/app/editor/graphics/graphics_editor.cc +++ b/src/app/editor/graphics/graphics_editor.cc @@ -112,28 +112,29 @@ absl::Status GraphicsEditor::Update() { prototype_card.SetDefaultSize(600, 500); // Get visibility flags from card manager and pass to Begin() + // Always call End() after Begin() - End() handles ImGui state safely if (sheet_editor_card.Begin(card_manager.GetVisibilityFlag("graphics.sheet_editor"))) { status_ = UpdateGfxEdit(); - sheet_editor_card.End(); } + sheet_editor_card.End(); if (sheet_browser_card.Begin(card_manager.GetVisibilityFlag("graphics.sheet_browser"))) { if (asset_browser_.Initialized == false) { asset_browser_.Initialize(gfx::Arena::Get().gfx_sheets()); } asset_browser_.Draw(gfx::Arena::Get().gfx_sheets()); - sheet_browser_card.End(); } + sheet_browser_card.End(); if (player_anims_card.Begin(card_manager.GetVisibilityFlag("graphics.player_animations"))) { status_ = UpdateLinkGfxView(); - player_anims_card.End(); } + player_anims_card.End(); if (prototype_card.Begin(card_manager.GetVisibilityFlag("graphics.prototype_viewer"))) { status_ = UpdateScadView(); - prototype_card.End(); } + prototype_card.End(); CLEAR_AND_RETURN_STATUS(status_) return absl::OkStatus(); diff --git a/src/app/editor/graphics/screen_editor.cc b/src/app/editor/graphics/screen_editor.cc index 3e435110..4a384cea 100644 --- a/src/app/editor/graphics/screen_editor.cc +++ b/src/app/editor/graphics/screen_editor.cc @@ -109,30 +109,29 @@ absl::Status ScreenEditor::Update() { naming_screen_card.SetDefaultSize(500, 400); // Get visibility flags from card manager and pass to Begin() + // Always call End() after Begin() - End() handles ImGui state safely if (dungeon_maps_card.Begin(card_manager.GetVisibilityFlag("screen.dungeon_maps"))) { DrawDungeonMapsEditor(); } dungeon_maps_card.End(); - + if (inventory_menu_card.Begin(card_manager.GetVisibilityFlag("screen.inventory_menu"))) { DrawInventoryMenuEditor(); - } inventory_menu_card.End(); - + if (overworld_map_card.Begin(card_manager.GetVisibilityFlag("screen.overworld_map"))) { DrawOverworldMapEditor(); } overworld_map_card.End(); - + if (title_screen_card.Begin(card_manager.GetVisibilityFlag("screen.title_screen"))) { DrawTitleScreenEditor(); - } title_screen_card.End(); - + if (naming_screen_card.Begin(card_manager.GetVisibilityFlag("screen.naming_screen"))) { - DrawNamingScreenEditor(); + DrawNamingScreenEditor(); } naming_screen_card.End(); diff --git a/src/app/editor/message/message_editor.cc b/src/app/editor/message/message_editor.cc index 01cb233e..7af2ce40 100644 --- a/src/app/editor/message/message_editor.cc +++ b/src/app/editor/message/message_editor.cc @@ -151,8 +151,8 @@ absl::Status MessageEditor::Update() { list_card.SetDefaultSize(400, 600); if (list_card.Begin()) { DrawMessageList(); - list_card.End(); } + list_card.End(); } // Message Editor Card @@ -161,8 +161,8 @@ absl::Status MessageEditor::Update() { editor_card.SetDefaultSize(500, 600); if (editor_card.Begin()) { DrawCurrentMessage(); - editor_card.End(); } + editor_card.End(); } // Font Atlas Card @@ -172,8 +172,8 @@ absl::Status MessageEditor::Update() { if (font_card.Begin()) { DrawFontAtlas(); DrawExpandedMessageSettings(); - font_card.End(); } + font_card.End(); } // Dictionary Card @@ -184,8 +184,8 @@ absl::Status MessageEditor::Update() { DrawTextCommands(); DrawSpecialCharacters(); DrawDictionary(); - dict_card.End(); } + dict_card.End(); } return absl::OkStatus(); diff --git a/src/app/editor/music/music_editor.cc b/src/app/editor/music/music_editor.cc index adab8a2b..af840250 100644 --- a/src/app/editor/music/music_editor.cc +++ b/src/app/editor/music/music_editor.cc @@ -48,20 +48,20 @@ absl::Status MusicEditor::Update() { // Music Tracker Card if (tracker_card.Begin(card_manager.GetVisibilityFlag("music.tracker"))) { DrawTrackerView(); - tracker_card.End(); } + tracker_card.End(); // Instrument Editor Card if (instrument_card.Begin(card_manager.GetVisibilityFlag("music.instrument_editor"))) { DrawInstrumentEditor(); - instrument_card.End(); } + instrument_card.End(); // Assembly View Card if (assembly_card.Begin(card_manager.GetVisibilityFlag("music.assembly"))) { assembly_editor_.InlineUpdate(); - assembly_card.End(); } + assembly_card.End(); return absl::OkStatus(); } diff --git a/src/app/editor/sprite/sprite_editor.cc b/src/app/editor/sprite/sprite_editor.cc index af82639b..70423c6a 100644 --- a/src/app/editor/sprite/sprite_editor.cc +++ b/src/app/editor/sprite/sprite_editor.cc @@ -57,15 +57,16 @@ absl::Status SpriteEditor::Update() { custom_card.SetDefaultSize(800, 600); // Get visibility flags from card manager and pass to Begin() + // Always call End() after Begin() - End() handles ImGui state safely if (vanilla_card.Begin(card_manager.GetVisibilityFlag("sprite.vanilla_editor"))) { DrawVanillaSpriteEditor(); - vanilla_card.End(); } + vanilla_card.End(); if (custom_card.Begin(card_manager.GetVisibilityFlag("sprite.custom_editor"))) { DrawCustomSprites(); - custom_card.End(); } + custom_card.End(); return status_.ok() ? absl::OkStatus() : status_; } diff --git a/src/app/gui/app/editor_layout.cc b/src/app/gui/app/editor_layout.cc index bf4f4e91..57d41db8 100644 --- a/src/app/gui/app/editor_layout.cc +++ b/src/app/gui/app/editor_layout.cc @@ -250,12 +250,14 @@ void EditorCard::SetPosition(Position pos) { bool EditorCard::Begin(bool* p_open) { // Check visibility flag first - if provided and false, don't show the card if (p_open && !*p_open) { + imgui_begun_ = false; return false; } // Handle icon-collapsed state if (icon_collapsible_ && collapsed_to_icon_) { DrawFloatingIconButton(); + imgui_begun_ = false; return false; } @@ -320,6 +322,9 @@ bool EditorCard::Begin(bool* p_open) { closable_ ? actual_p_open : nullptr, flags); + // Mark that ImGui::Begin() was called - End() must always be called now + imgui_begun_ = true; + // Register card window for test automation if (ImGui::GetCurrentWindow() && ImGui::GetCurrentWindow()->ID != 0) { std::string card_path = absl::StrFormat("EditorCard:%s", title_.c_str()); @@ -332,12 +337,16 @@ bool EditorCard::Begin(bool* p_open) { } void EditorCard::End() { - // Check if window was focused this frame - focused_ = ImGui::IsWindowFocused(ImGuiFocusedFlags_ChildWindows); - - ImGui::End(); - ImGui::PopStyleColor(2); - ImGui::PopStyleVar(2); + // Only call ImGui::End() and pop styles if ImGui::Begin() was called + if (imgui_begun_) { + // Check if window was focused this frame + focused_ = ImGui::IsWindowFocused(ImGuiFocusedFlags_ChildWindows); + + ImGui::End(); + ImGui::PopStyleColor(2); + ImGui::PopStyleVar(2); + imgui_begun_ = false; + } } void EditorCard::Focus() { diff --git a/src/app/gui/app/editor_layout.h b/src/app/gui/app/editor_layout.h index c5362df1..27363fe9 100644 --- a/src/app/gui/app/editor_layout.h +++ b/src/app/gui/app/editor_layout.h @@ -98,9 +98,9 @@ class Toolset { * tile_card.SetPosition(CardPosition::Right); * * if (tile_card.Begin()) { - * // Draw tile selector content - * tile_card.End(); + * // Draw tile selector content when visible * } + * tile_card.End(); // Always call End() after Begin() * ``` */ class EditorCard { @@ -154,6 +154,7 @@ class EditorCard { bool first_draw_ = true; bool focused_ = false; bool* p_open_ = nullptr; + bool imgui_begun_ = false; // Track if ImGui::Begin() was called // UX enhancements bool headless_ = false; // Minimal chrome, no title bar