From 0bcad79d06418d820516324bac369c9d9df8313a Mon Sep 17 00:00:00 2001 From: scawful Date: Fri, 10 Oct 2025 02:20:22 -0400 Subject: [PATCH] refactor: Improve Rendering Logic and Debugging in Dungeon Components - Updated DungeonCanvasViewer to disable room layout drawing to reduce visual clutter, enhancing clarity during object placement. - Enhanced BackgroundBuffer to skip empty and floor tiles, preventing overwriting of drawn elements and improving rendering efficiency. - Refined Bitmap palette application to ensure correct transparency handling and marked bitmaps as modified for texture updates. - Streamlined ObjectDrawer by removing unnecessary debug logs and simplifying object drawing logic, improving performance and readability. - Adjusted Room rendering methods to utilize palette indirection for accurate color application, ensuring consistent visual output across rooms. --- .../editor/dungeon/dungeon_canvas_viewer.cc | 10 +-- src/app/gfx/background_buffer.cc | 24 +++++-- src/app/gfx/bitmap.cc | 21 ++++-- src/app/zelda3/dungeon/object_drawer.cc | 64 ++++--------------- src/app/zelda3/dungeon/room.cc | 40 ++++++------ src/app/zelda3/dungeon/room_object.cc | 8 --- 6 files changed, 72 insertions(+), 95 deletions(-) diff --git a/src/app/editor/dungeon/dungeon_canvas_viewer.cc b/src/app/editor/dungeon/dungeon_canvas_viewer.cc index 85c47f08..07fd1eb1 100644 --- a/src/app/editor/dungeon/dungeon_canvas_viewer.cc +++ b/src/app/editor/dungeon/dungeon_canvas_viewer.cc @@ -519,13 +519,15 @@ void DungeonCanvasViewer::DrawDungeonCanvas(int room_id) { } } - // Draw layout overlays on top of background bitmap + // Draw optional overlays on top of background bitmap if (rooms_ && rom_->is_loaded()) { auto& room = (*rooms_)[room_id]; - // Draw room layout (structural elements like walls, pits) - // This provides context for object placement - DrawRoomLayout(room); + // DISABLED: Room layout drawing - causes visual clutter + // Layout tiles (2793) render over everything and obscure objects + // if (show_layout_overlay_) { + // DrawRoomLayout(room); + // } // VISUALIZATION: Draw object position rectangles (for debugging) // This shows where objects are placed regardless of whether graphics render diff --git a/src/app/gfx/background_buffer.cc b/src/app/gfx/background_buffer.cc index f6b91685..28e9239d 100644 --- a/src/app/gfx/background_buffer.cc +++ b/src/app/gfx/background_buffer.cc @@ -106,22 +106,29 @@ void BackgroundBuffer::DrawBackground(std::span gfx16_data) { // For each tile on the tile buffer int drawn_count = 0; int skipped_count = 0; - int non_floor_count = 0; for (int yy = 0; yy < tiles_h; yy++) { for (int xx = 0; xx < tiles_w; xx++) { uint16_t word = buffer_[xx + yy * tiles_w]; - // Prevent draw if tile == 0xFFFF since it's 0 indexed + + // Skip empty tiles (0xFFFF) - these show the floor if (word == 0xFFFF) { skipped_count++; continue; } + + // Skip zero tiles - also show the floor + if (word == 0) { + skipped_count++; + continue; + } + auto tile = gfx::WordToTileInfo(word); - // Count floor tiles vs non-floor - if (tile.id_ >= 0xEE && tile.id_ <= 0xFF) { - // This looks like a floor tile (based on DrawFloor's 14EE pattern) - } else if (word != 0) { - non_floor_count++; + // Skip floor tiles (0xEC-0xFD) - don't overwrite DrawFloor's work + // These are the animated floor tiles, already drawn by DrawFloor + if (tile.id_ >= 0xEC && tile.id_ <= 0xFD) { + skipped_count++; + continue; } // Calculate pixel offset for tile position (xx, yy) in the 512x512 bitmap @@ -176,6 +183,9 @@ void BackgroundBuffer::DrawFloor(const std::vector& rom_data, rom_data[tile_address_floor + f + 5]); gfx::TileInfo floorTile8(rom_data[tile_address_floor + f + 6], rom_data[tile_address_floor + f + 7]); + + // Floor tiles specify which 8-color sub-palette from the 90-color dungeon palette + // e.g., palette 6 = colors 48-55 (6 * 8 = 48) // Draw the floor tiles in a pattern // Convert TileInfo to 16-bit words with palette information diff --git a/src/app/gfx/bitmap.cc b/src/app/gfx/bitmap.cc index c5f50d21..5090db3c 100644 --- a/src/app/gfx/bitmap.cc +++ b/src/app/gfx/bitmap.cc @@ -269,13 +269,23 @@ void Bitmap::ApplyStoredPalette() { } SDL_UnlockSurface(surface_); + + // Apply all palette colors from the SnesPalette for (size_t i = 0; i < palette_.size() && i < 256; ++i) { const auto& pal_color = palette_[i]; - sdl_palette->colors[i].r = pal_color.rgb().x; - sdl_palette->colors[i].g = pal_color.rgb().y; - sdl_palette->colors[i].b = pal_color.rgb().z; - sdl_palette->colors[i].a = pal_color.rgb().w; + // NOTE: rgb() stores 0-255 values directly in ImVec4 (unconventional but intentional) + sdl_palette->colors[i].r = static_cast(pal_color.rgb().x); + sdl_palette->colors[i].g = static_cast(pal_color.rgb().y); + sdl_palette->colors[i].b = static_cast(pal_color.rgb().z); + + // CRITICAL: Transparency for color 0 of each sub-palette + if (pal_color.is_transparent()) { + sdl_palette->colors[i].a = 0; // Fully transparent + } else { + sdl_palette->colors[i].a = 255; // Fully opaque + } } + SDL_LockSurface(surface_); } @@ -285,6 +295,9 @@ void Bitmap::SetPalette(const SnesPalette &palette) { // Apply it immediately if surface is ready ApplyStoredPalette(); + + // Mark as modified to trigger texture update + modified_ = true; } void Bitmap::SetPaletteWithTransparent(const SnesPalette &palette, size_t index, diff --git a/src/app/zelda3/dungeon/object_drawer.cc b/src/app/zelda3/dungeon/object_drawer.cc index d9397f2f..1fdd4702 100644 --- a/src/app/zelda3/dungeon/object_drawer.cc +++ b/src/app/zelda3/dungeon/object_drawer.cc @@ -28,29 +28,17 @@ absl::Status ObjectDrawer::DrawObject(const RoomObject& object, // Ensure object has tiles loaded auto mutable_obj = const_cast(object); - LOG_DEBUG("ObjectDrawer", "Setting ROM for object ID=0x%02X", object.id_); mutable_obj.set_rom(rom_); - LOG_DEBUG("ObjectDrawer", "Calling EnsureTilesLoaded for object ID=0x%02X", object.id_); mutable_obj.EnsureTilesLoaded(); - - // Check if tiles were actually loaded on the mutable object - LOG_DEBUG("ObjectDrawer", "After EnsureTilesLoaded: mutable object has %zu tiles", - mutable_obj.tiles().size()); // Select buffer based on layer auto& target_bg = (object.layer_ == RoomObject::LayerType::BG2) ? bg2 : bg1; - LOG_DEBUG("ObjectDrawer", "Object ID=0x%02X using %s buffer (layer=%d)", - object.id_, (object.layer_ == RoomObject::LayerType::BG2) ? "BG2" : "BG1", object.layer_); // Skip objects that don't have tiles loaded - check mutable object if (mutable_obj.tiles().empty()) { - LOG_DEBUG("ObjectDrawer", "Object ID=0x%02X has no tiles loaded, skipping", object.id_); return absl::OkStatus(); } - LOG_DEBUG("ObjectDrawer", "Object ID=0x%02X has %zu tiles, proceeding with drawing", - object.id_, mutable_obj.tiles().size()); - // Look up draw routine for this object int routine_id = GetDrawRoutineId(object.id_); @@ -75,27 +63,8 @@ absl::Status ObjectDrawer::DrawObjectList( gfx::BackgroundBuffer& bg2, const gfx::PaletteGroup& palette_group) { - LOG_DEBUG("ObjectDrawer", "Drawing %zu objects", objects.size()); - - int drawn_count = 0; - int skipped_count = 0; - for (const auto& object : objects) { - auto status = DrawObject(object, bg1, bg2, palette_group); - if (status.ok()) { - drawn_count++; - } else { - skipped_count++; - // Only print errors that aren't "no tiles" (which is common and expected) - if (status.code() != absl::StatusCode::kOk) { - // Skip silently - many objects don't have tiles loaded yet - } - } - } - - // Only log if there are failures - if (skipped_count > 0) { - LOG_DEBUG("ObjectDrawer", "Drew %d objects, skipped %d", drawn_count, skipped_count); + DrawObject(object, bg1, bg2, palette_group); } return absl::OkStatus(); @@ -663,14 +632,9 @@ void ObjectDrawer::DrawRightwardsDecor2x2spaced12_1to16(const RoomObject& obj, g void ObjectDrawer::WriteTile8(gfx::BackgroundBuffer& bg, int tile_x, int tile_y, const gfx::TileInfo& tile_info) { - LOG_DEBUG("ObjectDrawer", "Writing 8x8 tile at tile pos (%d,%d) to bitmap", tile_x, tile_y); - // Draw directly to bitmap instead of tile buffer to avoid being overwritten auto& bitmap = bg.bitmap(); - LOG_DEBUG("ObjectDrawer", "Bitmap status: active=%d, width=%d, height=%d, surface=%p", - bitmap.is_active(), bitmap.width(), bitmap.height(), bitmap.surface()); if (!bitmap.is_active() || bitmap.width() == 0) { - LOG_DEBUG("ObjectDrawer", "Bitmap not ready: active=%d, width=%d", bitmap.is_active(), bitmap.width()); return; // Bitmap not ready } @@ -714,20 +678,20 @@ void ObjectDrawer::DrawTileToBitmap(gfx::Bitmap& bitmap, const gfx::TileInfo& ti return; } - // Calculate tile position in graphics sheet (128 pixels wide) + // Calculate tile position in graphics sheet (128 pixels wide, 16 tiles per row) int tile_sheet_x = (tile_info.id_ % 16) * 8; // 16 tiles per row int tile_sheet_y = (tile_info.id_ / 16) * 8; // Each row is 16 tiles - // Clamp palette to valid range - uint8_t palette_id = tile_info.palette_ & 0x0F; - if (palette_id > 10) palette_id = palette_id % 11; - uint8_t palette_offset = palette_id * 8; // 3BPP: 8 colors per palette + // CRITICAL: Dungeon palettes are 90 colors organized as: + // - 10 sub-palettes of 8 colors each (0-79) + // - Plus 10 additional colors (80-89) + // Each tile uses palette field (0-10) to select which 8-color sub-palette + uint8_t palette_id = tile_info.palette_ & 0x0F; // 0-15 possible + if (palette_id > 10) palette_id = 10; // Clamp to 0-10 for dungeons - // Force a visible palette for debugging - if (palette_id == 0) { - palette_id = 1; // Use palette 1 instead of 0 - palette_offset = palette_id * 8; - } + // Calculate color offset in dungeon palette + // For dungeon: palette 0 = colors 0-7, palette 1 = colors 8-15, etc. + uint8_t palette_offset = palette_id * 8; // Draw 8x8 pixels for (int py = 0; py < 8; py++) { @@ -749,12 +713,6 @@ void ObjectDrawer::DrawTileToBitmap(gfx::Bitmap& bitmap, const gfx::TileInfo& ti int dest_index = dest_y * bitmap.width() + dest_x; if (dest_index >= 0 && dest_index < static_cast(bitmap.mutable_data().size())) { bitmap.mutable_data()[dest_index] = final_color; - - // Debug first pixel of each tile - if (py == 0 && px == 0) { - LOG_DEBUG("ObjectDrawer", "Tile ID=0x%02X at (%d,%d): palette=%d, pixel_index=%d, final_color=%d", - tile_info.id_, pixel_x, pixel_y, palette_id, pixel_index, final_color); - } } } } diff --git a/src/app/zelda3/dungeon/room.cc b/src/app/zelda3/dungeon/room.cc index 763c8835..b222eefd 100644 --- a/src/app/zelda3/dungeon/room.cc +++ b/src/app/zelda3/dungeon/room.cc @@ -312,36 +312,29 @@ void Room::RenderRoomGraphics() { // Get and apply palette BEFORE rendering objects (so objects use correct colors) auto& dungeon_pal_group = rom()->mutable_palette_group()->dungeon_main; int num_palettes = dungeon_pal_group.size(); - int palette_id = palette; - LOG_DEBUG("[RenderRoomGraphics]", "Room palette byte: %d, num_palettes available: %d", - palette_id, num_palettes); + // Use palette indirection table lookup (same as dungeon_canvas_viewer.cc line 854) + int palette_id = palette; // Default fallback + if (palette < rom()->paletteset_ids.size() && !rom()->paletteset_ids[palette].empty()) { + auto dungeon_palette_ptr = rom()->paletteset_ids[palette][0]; + auto palette_word = rom()->ReadWord(0xDEC4B + dungeon_palette_ptr); + if (palette_word.ok()) { + palette_id = palette_word.value() / 180; // Divide by 180 to get group index + LOG_DEBUG("[RenderRoomGraphics]", "Palette lookup: byte=0x%02X → group_id=%d", palette, palette_id); + } + } + // Clamp to valid range if (palette_id < 0 || palette_id >= num_palettes) { - LOG_DEBUG("[RenderRoomGraphics]", "WARNING: palette_id %d out of bounds, clamping to %d", - palette_id, palette_id % num_palettes); palette_id = palette_id % num_palettes; } auto bg1_palette = dungeon_pal_group[palette_id]; if (bg1_palette.size() > 0) { - LOG_DEBUG("[RenderRoomGraphics]", "Applying palette_id=%d with %d colors to bitmaps", - palette_id, (int)bg1_palette.size()); - - // Debug: Log first few palette colors - for (int i = 0; i < std::min(16, (int)bg1_palette.size()); i++) { - auto rgb = bg1_palette[i].rgb(); - LOG_DEBUG("[RenderRoomGraphics]", " Palette[%d]: R=%.0f G=%.0f B=%.0f A=%.0f %s", - i, rgb.x, rgb.y, rgb.z, rgb.w, - bg1_palette[i].is_transparent() ? "(TRANSPARENT)" : ""); - } - // Apply FULL 90-color dungeon palette bg1_bmp.SetPalette(bg1_palette); bg2_bmp.SetPalette(bg1_palette); - } else { - LOG_DEBUG("[RenderRoomGraphics]", "ERROR: Palette is empty!"); } // Render objects ON TOP of background tiles (AFTER palette is set) @@ -379,10 +372,19 @@ void Room::RenderObjectsToBackground() { return; } - // Get palette group for object rendering + // Get palette group for object rendering (use SAME lookup as RenderRoomGraphics) auto& dungeon_pal_group = rom()->mutable_palette_group()->dungeon_main; int num_palettes = dungeon_pal_group.size(); + + // Use palette indirection table lookup int palette_id = palette; + if (palette < rom()->paletteset_ids.size() && !rom()->paletteset_ids[palette].empty()) { + auto dungeon_palette_ptr = rom()->paletteset_ids[palette][0]; + auto palette_word = rom()->ReadWord(0xDEC4B + dungeon_palette_ptr); + if (palette_word.ok()) { + palette_id = palette_word.value() / 180; + } + } if (palette_id < 0 || palette_id >= num_palettes) { palette_id = 0; diff --git a/src/app/zelda3/dungeon/room_object.cc b/src/app/zelda3/dungeon/room_object.cc index 4406e3c7..6b3cbba1 100644 --- a/src/app/zelda3/dungeon/room_object.cc +++ b/src/app/zelda3/dungeon/room_object.cc @@ -50,28 +50,20 @@ ObjectOption operator~(ObjectOption option) { // Modern rendering uses ObjectDrawer which draws directly to BackgroundBuffer bitmaps. void RoomObject::EnsureTilesLoaded() { - LOG_DEBUG("RoomObject", "Object ID=0x%02X, tiles_loaded=%d", id_, tiles_loaded_); - if (tiles_loaded_) { - LOG_DEBUG("RoomObject", "Tiles already loaded for object 0x%02X", id_); return; } if (rom_ == nullptr) { - LOG_DEBUG("RoomObject", "ERROR: ROM not set for object 0x%02X", id_); return; } // Try the new parser first - this is more efficient and accurate - LOG_DEBUG("RoomObject", "Trying parser for object 0x%02X", id_); auto parser_status = LoadTilesWithParser(); if (parser_status.ok()) { - LOG_DEBUG("RoomObject", "Parser succeeded for object 0x%02X, loaded %zu tiles", id_, tiles_.size()); tiles_loaded_ = true; return; } - - LOG_DEBUG("RoomObject", "Parser failed for object 0x%02X: %s", id_, parser_status.message().data()); // Fallback to legacy method for compatibility with enhanced validation auto rom_data = rom_->data();