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.
This commit is contained in:
scawful
2025-10-13 15:14:01 -04:00
parent 27aba01864
commit c95e5ac7ef
8 changed files with 168 additions and 165 deletions

View File

@@ -443,58 +443,56 @@ void DungeonEditorV2::DrawRoomsListCard() {
if (selector_card.Begin()) { if (selector_card.Begin()) {
if (!rom_ || !rom_->is_loaded()) { if (!rom_ || !rom_->is_loaded()) {
ImGui::Text("ROM not loaded"); ImGui::Text("ROM not loaded");
selector_card.End(); } else {
return; // Add text filter
} static char room_filter[256] = "";
ImGui::SetNextItemWidth(-1);
// Add text filter if (ImGui::InputTextWithHint("##RoomFilter", ICON_MD_SEARCH " Filter rooms...",
static char room_filter[256] = ""; room_filter, sizeof(room_filter))) {
ImGui::SetNextItemWidth(-1); // Filter updated
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);
for (int i = 0; i < zelda3::NumberOfRooms; i++) { ImGui::Separator();
// Get room name
std::string room_name; // Scrollable room list - simple and reliable
if (i < static_cast<int>(std::size(zelda3::kRoomNames))) { if (ImGui::BeginChild("##RoomsList", ImVec2(0, 0), true,
room_name = std::string(zelda3::kRoomNames[i]); ImGuiWindowFlags_AlwaysVerticalScrollbar)) {
} else { std::string filter_str = room_filter;
room_name = absl::StrFormat("Room %03X", i); std::transform(filter_str.begin(), filter_str.end(), filter_str.begin(), ::tolower);
}
// Apply filter for (int i = 0; i < zelda3::NumberOfRooms; i++) {
if (!filter_str.empty()) { // Get room name
std::string name_lower = room_name; std::string room_name;
std::transform(name_lower.begin(), name_lower.end(), if (i < static_cast<int>(std::size(zelda3::kRoomNames))) {
name_lower.begin(), ::tolower); room_name = std::string(zelda3::kRoomNames[i]);
if (name_lower.find(filter_str) == std::string::npos) { } else {
continue; 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);
} }
} }
ImGui::EndChild();
// 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();
} }
} }
selector_card.End(); selector_card.End();
@@ -510,92 +508,90 @@ void DungeonEditorV2::DrawEntrancesListCard() {
if (entrances_card.Begin()) { if (entrances_card.Begin()) {
if (!rom_ || !rom_->is_loaded()) { if (!rom_ || !rom_->is_loaded()) {
ImGui::Text("ROM not loaded"); ImGui::Text("ROM not loaded");
entrances_card.End(); } else {
return; // Full entrance configuration UI (matching dungeon_room_selector layout)
} auto& current_entrance = entrances_[current_entrance_id_];
// Full entrance configuration UI (matching dungeon_room_selector layout) gui::InputHexWord("Entrance ID", &current_entrance.entrance_id_);
auto& current_entrance = entrances_[current_entrance_id_]; gui::InputHexWord("Room ID", reinterpret_cast<uint16_t*>(&current_entrance.room_));
ImGui::SameLine();
gui::InputHexWord("Entrance ID", &current_entrance.entrance_id_); gui::InputHexByte("Dungeon ID", &current_entrance.dungeon_id_, 50.f, true);
gui::InputHexWord("Room ID", reinterpret_cast<uint16_t*>(&current_entrance.room_));
ImGui::SameLine(); gui::InputHexByte("Blockset", &current_entrance.blockset_, 50.f, true);
gui::InputHexByte("Dungeon ID", &current_entrance.dungeon_id_, 50.f, true); ImGui::SameLine();
gui::InputHexByte("Music", &current_entrance.music_, 50.f, true);
gui::InputHexByte("Blockset", &current_entrance.blockset_, 50.f, true); ImGui::SameLine();
ImGui::SameLine(); gui::InputHexByte("Floor", &current_entrance.floor_);
gui::InputHexByte("Music", &current_entrance.music_, 50.f, true);
ImGui::SameLine(); ImGui::Separator();
gui::InputHexByte("Floor", &current_entrance.floor_);
gui::InputHexWord("Player X ", &current_entrance.x_position_);
ImGui::Separator(); ImGui::SameLine();
gui::InputHexWord("Player Y ", &current_entrance.y_position_);
gui::InputHexWord("Player X ", &current_entrance.x_position_);
ImGui::SameLine(); gui::InputHexWord("Camera X", &current_entrance.camera_trigger_x_);
gui::InputHexWord("Player Y ", &current_entrance.y_position_); ImGui::SameLine();
gui::InputHexWord("Camera Y", &current_entrance.camera_trigger_y_);
gui::InputHexWord("Camera X", &current_entrance.camera_trigger_x_);
ImGui::SameLine(); gui::InputHexWord("Scroll X ", &current_entrance.camera_x_);
gui::InputHexWord("Camera Y", &current_entrance.camera_trigger_y_); ImGui::SameLine();
gui::InputHexWord("Scroll Y ", &current_entrance.camera_y_);
gui::InputHexWord("Scroll X ", &current_entrance.camera_x_);
ImGui::SameLine(); gui::InputHexWord("Exit", reinterpret_cast<uint16_t*>(&current_entrance.exit_), 50.f, true);
gui::InputHexWord("Scroll Y ", &current_entrance.camera_y_);
ImGui::Separator();
gui::InputHexWord("Exit", reinterpret_cast<uint16_t*>(&current_entrance.exit_), 50.f, true); ImGui::Text("Camera Boundaries");
ImGui::Separator();
ImGui::Separator(); ImGui::Text("\t\t\t\t\tNorth East South West");
ImGui::Text("Camera Boundaries");
ImGui::Separator(); gui::InputHexByte("Quadrant", &current_entrance.camera_boundary_qn_, 50.f, true);
ImGui::Text("\t\t\t\t\tNorth East South West"); ImGui::SameLine();
gui::InputHexByte("##QE", &current_entrance.camera_boundary_qe_, 50.f, true);
gui::InputHexByte("Quadrant", &current_entrance.camera_boundary_qn_, 50.f, true); ImGui::SameLine();
ImGui::SameLine(); gui::InputHexByte("##QS", &current_entrance.camera_boundary_qs_, 50.f, true);
gui::InputHexByte("##QE", &current_entrance.camera_boundary_qe_, 50.f, true); ImGui::SameLine();
ImGui::SameLine(); gui::InputHexByte("##QW", &current_entrance.camera_boundary_qw_, 50.f, true);
gui::InputHexByte("##QS", &current_entrance.camera_boundary_qs_, 50.f, true);
ImGui::SameLine(); gui::InputHexByte("Full room", &current_entrance.camera_boundary_fn_, 50.f, true);
gui::InputHexByte("##QW", &current_entrance.camera_boundary_qw_, 50.f, true); ImGui::SameLine();
gui::InputHexByte("##FE", &current_entrance.camera_boundary_fe_, 50.f, true);
gui::InputHexByte("Full room", &current_entrance.camera_boundary_fn_, 50.f, true); ImGui::SameLine();
ImGui::SameLine(); gui::InputHexByte("##FS", &current_entrance.camera_boundary_fs_, 50.f, true);
gui::InputHexByte("##FE", &current_entrance.camera_boundary_fe_, 50.f, true); ImGui::SameLine();
ImGui::SameLine(); gui::InputHexByte("##FW", &current_entrance.camera_boundary_fw_, 50.f, true);
gui::InputHexByte("##FS", &current_entrance.camera_boundary_fs_, 50.f, true);
ImGui::SameLine(); ImGui::Separator();
gui::InputHexByte("##FW", &current_entrance.camera_boundary_fw_, 50.f, true);
// Entrance list - simple and reliable
ImGui::Separator(); if (ImGui::BeginChild("##EntrancesList", ImVec2(0, 0), true,
ImGuiWindowFlags_AlwaysVerticalScrollbar)) {
// Entrance list - simple and reliable for (int i = 0; i < 0x8C; i++) {
if (ImGui::BeginChild("##EntrancesList", ImVec2(0, 0), true, // The last seven are spawn points
ImGuiWindowFlags_AlwaysVerticalScrollbar)) { std::string entrance_name;
for (int i = 0; i < 0x8C; i++) { if (i < 0x85) {
// The last seven are spawn points entrance_name = std::string(zelda3::kEntranceNames[i]);
std::string entrance_name; } else {
if (i < 0x85) { entrance_name = absl::StrFormat("Spawn Point %d", 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";
// Get associated room name if (room_id >= 0 && room_id < static_cast<int>(std::size(zelda3::kRoomNames))) {
int room_id = entrances_[i].room_; room_name = std::string(zelda3::kRoomNames[room_id]);
std::string room_name = "Unknown"; }
if (room_id >= 0 && room_id < static_cast<int>(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());
std::string label = absl::StrFormat("[%02X] %s -> %s", bool is_selected = (current_entrance_id_ == i);
i, entrance_name.c_str(), room_name.c_str()); if (ImGui::Selectable(label.c_str(), is_selected)) {
current_entrance_id_ = i;
bool is_selected = (current_entrance_id_ == i); OnEntranceSelected(i);
if (ImGui::Selectable(label.c_str(), is_selected)) { }
current_entrance_id_ = i;
OnEntranceSelected(i);
} }
ImGui::EndChild();
} }
ImGui::EndChild();
} }
} }
entrances_card.End(); entrances_card.End();
@@ -757,12 +753,8 @@ void DungeonEditorV2::DrawRoomGraphicsCard() {
if (graphics_card.Begin()) { if (graphics_card.Begin()) {
if (!rom_ || !rom_->is_loaded()) { if (!rom_ || !rom_->is_loaded()) {
ImGui::Text("ROM not loaded"); ImGui::Text("ROM not loaded");
graphics_card.End(); } else if (current_room_id_ >= 0 && current_room_id_ < static_cast<int>(rooms_.size())) {
return; // Show graphics for current room
}
// Show graphics for current room
if (current_room_id_ >= 0 && current_room_id_ < static_cast<int>(rooms_.size())) {
auto& room = rooms_[current_room_id_]; auto& room = rooms_[current_room_id_];
ImGui::Text("Room %03X Graphics", current_room_id_); ImGui::Text("Room %03X Graphics", current_room_id_);

View File

@@ -112,28 +112,29 @@ absl::Status GraphicsEditor::Update() {
prototype_card.SetDefaultSize(600, 500); prototype_card.SetDefaultSize(600, 500);
// Get visibility flags from card manager and pass to Begin() // 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"))) { if (sheet_editor_card.Begin(card_manager.GetVisibilityFlag("graphics.sheet_editor"))) {
status_ = UpdateGfxEdit(); status_ = UpdateGfxEdit();
sheet_editor_card.End();
} }
sheet_editor_card.End();
if (sheet_browser_card.Begin(card_manager.GetVisibilityFlag("graphics.sheet_browser"))) { if (sheet_browser_card.Begin(card_manager.GetVisibilityFlag("graphics.sheet_browser"))) {
if (asset_browser_.Initialized == false) { if (asset_browser_.Initialized == false) {
asset_browser_.Initialize(gfx::Arena::Get().gfx_sheets()); asset_browser_.Initialize(gfx::Arena::Get().gfx_sheets());
} }
asset_browser_.Draw(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"))) { if (player_anims_card.Begin(card_manager.GetVisibilityFlag("graphics.player_animations"))) {
status_ = UpdateLinkGfxView(); status_ = UpdateLinkGfxView();
player_anims_card.End();
} }
player_anims_card.End();
if (prototype_card.Begin(card_manager.GetVisibilityFlag("graphics.prototype_viewer"))) { if (prototype_card.Begin(card_manager.GetVisibilityFlag("graphics.prototype_viewer"))) {
status_ = UpdateScadView(); status_ = UpdateScadView();
prototype_card.End();
} }
prototype_card.End();
CLEAR_AND_RETURN_STATUS(status_) CLEAR_AND_RETURN_STATUS(status_)
return absl::OkStatus(); return absl::OkStatus();

View File

@@ -109,30 +109,29 @@ absl::Status ScreenEditor::Update() {
naming_screen_card.SetDefaultSize(500, 400); naming_screen_card.SetDefaultSize(500, 400);
// Get visibility flags from card manager and pass to Begin() // 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"))) { if (dungeon_maps_card.Begin(card_manager.GetVisibilityFlag("screen.dungeon_maps"))) {
DrawDungeonMapsEditor(); DrawDungeonMapsEditor();
} }
dungeon_maps_card.End(); dungeon_maps_card.End();
if (inventory_menu_card.Begin(card_manager.GetVisibilityFlag("screen.inventory_menu"))) { if (inventory_menu_card.Begin(card_manager.GetVisibilityFlag("screen.inventory_menu"))) {
DrawInventoryMenuEditor(); DrawInventoryMenuEditor();
} }
inventory_menu_card.End(); inventory_menu_card.End();
if (overworld_map_card.Begin(card_manager.GetVisibilityFlag("screen.overworld_map"))) { if (overworld_map_card.Begin(card_manager.GetVisibilityFlag("screen.overworld_map"))) {
DrawOverworldMapEditor(); DrawOverworldMapEditor();
} }
overworld_map_card.End(); overworld_map_card.End();
if (title_screen_card.Begin(card_manager.GetVisibilityFlag("screen.title_screen"))) { if (title_screen_card.Begin(card_manager.GetVisibilityFlag("screen.title_screen"))) {
DrawTitleScreenEditor(); DrawTitleScreenEditor();
} }
title_screen_card.End(); title_screen_card.End();
if (naming_screen_card.Begin(card_manager.GetVisibilityFlag("screen.naming_screen"))) { if (naming_screen_card.Begin(card_manager.GetVisibilityFlag("screen.naming_screen"))) {
DrawNamingScreenEditor(); DrawNamingScreenEditor();
} }
naming_screen_card.End(); naming_screen_card.End();

View File

@@ -151,8 +151,8 @@ absl::Status MessageEditor::Update() {
list_card.SetDefaultSize(400, 600); list_card.SetDefaultSize(400, 600);
if (list_card.Begin()) { if (list_card.Begin()) {
DrawMessageList(); DrawMessageList();
list_card.End();
} }
list_card.End();
} }
// Message Editor Card // Message Editor Card
@@ -161,8 +161,8 @@ absl::Status MessageEditor::Update() {
editor_card.SetDefaultSize(500, 600); editor_card.SetDefaultSize(500, 600);
if (editor_card.Begin()) { if (editor_card.Begin()) {
DrawCurrentMessage(); DrawCurrentMessage();
editor_card.End();
} }
editor_card.End();
} }
// Font Atlas Card // Font Atlas Card
@@ -172,8 +172,8 @@ absl::Status MessageEditor::Update() {
if (font_card.Begin()) { if (font_card.Begin()) {
DrawFontAtlas(); DrawFontAtlas();
DrawExpandedMessageSettings(); DrawExpandedMessageSettings();
font_card.End();
} }
font_card.End();
} }
// Dictionary Card // Dictionary Card
@@ -184,8 +184,8 @@ absl::Status MessageEditor::Update() {
DrawTextCommands(); DrawTextCommands();
DrawSpecialCharacters(); DrawSpecialCharacters();
DrawDictionary(); DrawDictionary();
dict_card.End();
} }
dict_card.End();
} }
return absl::OkStatus(); return absl::OkStatus();

View File

@@ -48,20 +48,20 @@ absl::Status MusicEditor::Update() {
// Music Tracker Card // Music Tracker Card
if (tracker_card.Begin(card_manager.GetVisibilityFlag("music.tracker"))) { if (tracker_card.Begin(card_manager.GetVisibilityFlag("music.tracker"))) {
DrawTrackerView(); DrawTrackerView();
tracker_card.End();
} }
tracker_card.End();
// Instrument Editor Card // Instrument Editor Card
if (instrument_card.Begin(card_manager.GetVisibilityFlag("music.instrument_editor"))) { if (instrument_card.Begin(card_manager.GetVisibilityFlag("music.instrument_editor"))) {
DrawInstrumentEditor(); DrawInstrumentEditor();
instrument_card.End();
} }
instrument_card.End();
// Assembly View Card // Assembly View Card
if (assembly_card.Begin(card_manager.GetVisibilityFlag("music.assembly"))) { if (assembly_card.Begin(card_manager.GetVisibilityFlag("music.assembly"))) {
assembly_editor_.InlineUpdate(); assembly_editor_.InlineUpdate();
assembly_card.End();
} }
assembly_card.End();
return absl::OkStatus(); return absl::OkStatus();
} }

View File

@@ -57,15 +57,16 @@ absl::Status SpriteEditor::Update() {
custom_card.SetDefaultSize(800, 600); custom_card.SetDefaultSize(800, 600);
// Get visibility flags from card manager and pass to Begin() // 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"))) { if (vanilla_card.Begin(card_manager.GetVisibilityFlag("sprite.vanilla_editor"))) {
DrawVanillaSpriteEditor(); DrawVanillaSpriteEditor();
vanilla_card.End();
} }
vanilla_card.End();
if (custom_card.Begin(card_manager.GetVisibilityFlag("sprite.custom_editor"))) { if (custom_card.Begin(card_manager.GetVisibilityFlag("sprite.custom_editor"))) {
DrawCustomSprites(); DrawCustomSprites();
custom_card.End();
} }
custom_card.End();
return status_.ok() ? absl::OkStatus() : status_; return status_.ok() ? absl::OkStatus() : status_;
} }

View File

@@ -250,12 +250,14 @@ void EditorCard::SetPosition(Position pos) {
bool EditorCard::Begin(bool* p_open) { bool EditorCard::Begin(bool* p_open) {
// Check visibility flag first - if provided and false, don't show the card // Check visibility flag first - if provided and false, don't show the card
if (p_open && !*p_open) { if (p_open && !*p_open) {
imgui_begun_ = false;
return false; return false;
} }
// Handle icon-collapsed state // Handle icon-collapsed state
if (icon_collapsible_ && collapsed_to_icon_) { if (icon_collapsible_ && collapsed_to_icon_) {
DrawFloatingIconButton(); DrawFloatingIconButton();
imgui_begun_ = false;
return false; return false;
} }
@@ -320,6 +322,9 @@ bool EditorCard::Begin(bool* p_open) {
closable_ ? actual_p_open : nullptr, closable_ ? actual_p_open : nullptr,
flags); flags);
// Mark that ImGui::Begin() was called - End() must always be called now
imgui_begun_ = true;
// Register card window for test automation // Register card window for test automation
if (ImGui::GetCurrentWindow() && ImGui::GetCurrentWindow()->ID != 0) { if (ImGui::GetCurrentWindow() && ImGui::GetCurrentWindow()->ID != 0) {
std::string card_path = absl::StrFormat("EditorCard:%s", title_.c_str()); std::string card_path = absl::StrFormat("EditorCard:%s", title_.c_str());
@@ -332,12 +337,16 @@ bool EditorCard::Begin(bool* p_open) {
} }
void EditorCard::End() { void EditorCard::End() {
// Check if window was focused this frame // Only call ImGui::End() and pop styles if ImGui::Begin() was called
focused_ = ImGui::IsWindowFocused(ImGuiFocusedFlags_ChildWindows); if (imgui_begun_) {
// Check if window was focused this frame
ImGui::End(); focused_ = ImGui::IsWindowFocused(ImGuiFocusedFlags_ChildWindows);
ImGui::PopStyleColor(2);
ImGui::PopStyleVar(2); ImGui::End();
ImGui::PopStyleColor(2);
ImGui::PopStyleVar(2);
imgui_begun_ = false;
}
} }
void EditorCard::Focus() { void EditorCard::Focus() {

View File

@@ -98,9 +98,9 @@ class Toolset {
* tile_card.SetPosition(CardPosition::Right); * tile_card.SetPosition(CardPosition::Right);
* *
* if (tile_card.Begin()) { * if (tile_card.Begin()) {
* // Draw tile selector content * // Draw tile selector content when visible
* tile_card.End();
* } * }
* tile_card.End(); // Always call End() after Begin()
* ``` * ```
*/ */
class EditorCard { class EditorCard {
@@ -154,6 +154,7 @@ class EditorCard {
bool first_draw_ = true; bool first_draw_ = true;
bool focused_ = false; bool focused_ = false;
bool* p_open_ = nullptr; bool* p_open_ = nullptr;
bool imgui_begun_ = false; // Track if ImGui::Begin() was called
// UX enhancements // UX enhancements
bool headless_ = false; // Minimal chrome, no title bar bool headless_ = false; // Minimal chrome, no title bar