yaze/docs/CANVAS_REFACTORING_STATUS.md

Canvas Refactoring - Current Status & Future Work

✅ Successfully Completed

1. Modern ImGui-Style Interface (Working)

Added Methods:

void Canvas::Begin(ImVec2 size = {0, 0});  // Replaces DrawBackground + DrawContextMenu
void Canvas::End();                        // Replaces DrawGrid + DrawOverlay

RAII Wrapper:

class ScopedCanvas {
  ScopedCanvas(const std::string& id, ImVec2 size = {});
  ~ScopedCanvas();  // Automatic End()
};

Usage:

// Modern pattern (cleaner, exception-safe)
gui::ScopedCanvas canvas("Editor", ImVec2(512, 512));
canvas->DrawBitmap(bitmap);
canvas->DrawTilePainter(tile, 16);

// Legacy pattern (still works - zero breaking changes)
canvas.DrawBackground();
canvas.DrawContextMenu();
canvas.DrawBitmap(bitmap);
canvas.DrawGrid();
canvas.DrawOverlay();

Status: ✅ Implemented, builds successfully, ready for adoption

2. Context Menu Improvements (Working)

Helper Constructors:

// Simple item
canvas.AddContextMenuItem({"Label", callback});

// With shortcut
canvas.AddContextMenuItem({"Label", callback, "Ctrl+X"});

// Conditional (enabled based on state)
canvas.AddContextMenuItem(
  Canvas::ContextMenuItem::Conditional("Action", callback, condition)
);

Benefits: More concise, clearer intent

Status: ✅ Implemented and working

3. Optional CanvasInteractionHandler Component (Available)

Created: canvas/canvas_interaction_handler.{h,cc} (579 lines total)

Purpose: Alternative API for tile interaction (NOT integrated into main Canvas)

Status: ✅ Built and available for future custom interaction logic

4. Code Cleanup

• ✅ Removed unused constants (kBlackColor, kOutlineRect)
• ✅ Improved inline documentation
• ✅ Better code organization

⚠️ Outstanding Issue: Rectangle Selection Wrapping

The Problem

When dragging a multi-tile rectangle selection near 512x512 local map boundaries in large maps, tiles still paint in the wrong location (wrap to left side of map).

What Was Attempted

Attempt 1: Clamp preview position in DrawBitmapGroup()

• ✅ Prevents visual wrapping in preview
• ❌ Doesn't fix actual painting location

Attempt 2: Pre-compute tile IDs in selected_tile16_ids_

• ✅ Tile IDs stored correctly
• ❌ Still paints in wrong location

Attempt 3: Clamp mouse position before grid alignment

• ✅ Smoother preview dragging
• ❌ Painting still wraps

Root Cause Analysis

The issue involves complex interaction between:

1. Original Selection (DrawSelectRect):

   • Right-click drag creates selection
   • selected_tiles_ = coordinates from original location
   • selected_points_ = rectangle bounds

2. Preview While Dragging (DrawBitmapGroup):

   • Repositions rectangle to follow mouse
   • Clamps to stay within 512x512 boundary
   • Updates selected_points_ to clamped position
   • Shows tile IDs from selected_tile16_ids_

3. Painting (CheckForOverworldEdits):

   • Uses selected_points_ for NEW location
   • Uses selected_tile16_ids_ for tile data
   • Loops through NEW coordinates
   • Index i increments through loop

The Mismatch:

• If clamped preview has fewer tiles than original selection
• Loop creates fewer positions than selected_tile16_ids_.size()
• Index goes out of sync
• OR: Loop positions don't match the coordinate calculation

Suspected Issue

The problem likely lies in how index_x and index_y are calculated in the painting loop:

// Current calculation (line 961-970 in overworld_editor.cc):
int local_map_x = x / local_map_size;
int local_map_y = y / local_map_size;
int tile16_x = (x % local_map_size) / kTile16Size;
int tile16_y = (y % local_map_size) / kTile16Size;
int index_x = local_map_x * tiles_per_local_map + tile16_x;
int index_y = local_map_y * tiles_per_local_map + tile16_y;

// This calculation assumes x,y are in WORLD coordinates (0-4096)
// But if the clamped rectangle spans boundaries differently...

What Needs Investigation

1. Coordinate space mismatch: Are x,y in the painting loop using the right coordinate system?
2. Index calculation: Does index_x/index_y correctly map to the world array?
3. Boundary conditions: What happens when clamped rectangle is smaller than original?
4. World array structure: Is selected_world[index_x][index_y] the right indexing?

Debugging Steps for Future Agent

// Add logging to CheckForOverworldEdits painting loop:
util::logf("Painting: i=%d, x=%d, y=%d, local_map=(%d,%d), tile16=(%d,%d), index=(%d,%d), tile_id=%d",
           i, x, y, local_map_x, local_map_y, tile16_x, tile16_y, 
           index_x, index_y, selected_tile16_ids_[i]);

// Compare with original selection:
util::logf("Original: selected_tiles_[%d] = (%.0f, %.0f)", 
           i, selected_tiles_[i].x, selected_tiles_[i].y);

// Check array bounds:
util::logf("World array: selected_world[%d][%d], bounds: 0x200 x 0x200", 
           index_x, index_y);

Possible Fixes to Try

Option A: Don't allow dragging across local map boundaries at all

// In DrawBitmapGroup, keep rectangle at original position if it would cross
if (would_cross_boundary) {
  return;  // Don't update selected_points_
}

Option B: Recalculate selected_tiles_ when clamped

// When clamping occurs, regenerate selected_tiles_ for new position
// This keeps original selection data synchronized with new position

Option C: Use different approach for rectangle painting

// Instead of iterating x,y coordinates and indexing array,
// Iterate through selected_tile16_ids_ and calculate x,y from index
for (int i = 0; i < selected_tile16_ids_.size(); ++i) {
  int rel_x = i % rect_width;
  int rel_y = i / rect_width;
  int abs_x = start_x + (rel_x * kTile16Size);
  int abs_y = start_y + (rel_y * kTile16Size);
  // Paint selected_tile16_ids_[i] at (abs_x, abs_y)
}

🔧 Files Modified

Core Canvas

• src/app/gui/canvas.h - Begin/End, ScopedCanvas, context menu helpers, clamping control
• src/app/gui/canvas.cc - Implementation, preview clamping logic
• src/app/gui/canvas_utils.h - Added clamp_rect_to_local_maps config
• src/app/gui/gui.cmake - Added canvas_interaction_handler.cc

Overworld Editor

• src/app/editor/overworld/overworld_editor.h - Added selected_tile16_ids_ member
• src/app/editor/overworld/overworld_editor.cc - Use member variable for tile IDs

Components Created

• src/app/gui/canvas/canvas_interaction_handler.h (209 lines)
• src/app/gui/canvas/canvas_interaction_handler.cc (370 lines)

📚 Documentation (Consolidated)

Final Structure (3 files):

1. CANVAS_GUIDE.md - Complete reference guide
2. canvas_refactoring_summary.md - Phase 1 background
3. CANVAS_REFACTORING_STATUS.md - This file

Removed: 10+ outdated/duplicate/incorrect planning documents

🎯 Future Refactoring Steps

Priority 1: Fix Rectangle Wrapping (High)

Issue: Rectangle selection still wraps when dragged to boundaries

Investigation needed:

1. Add detailed logging to painting loop
2. Verify coordinate space (canvas vs world vs tile)
3. Check world array indexing logic
4. Compare clamped vs original rectangle sizes

Possible approach: See "Possible Fixes to Try" section above

Priority 2: Extract Coordinate Conversion Helpers (Low Impact)

Pattern found: Repeated coordinate calculations across overworld editor

// Extract to helpers:
int GetMapIdFromPosition(ImVec2 world_pos, int current_world) const;
ImVec2 WorldToCanvasCoords(ImVec2 world_pos) const;
ImVec2 CanvasToWorldCoords(ImVec2 canvas_pos) const;
ImVec2 WorldToTileCoords(ImVec2 world_pos) const;

Benefit: Clearer code, less duplication, easier to debug

Priority 3: Move Components to canvas/ Namespace (Organizational)

Files to move:

• gui/canvas_utils.{h,cc} → gui/canvas/canvas_utils.{h,cc}
• gui/enhanced_palette_editor.{h,cc} → gui/canvas/palette_editor.{h,cc}
• gui/bpp_format_ui.{h,cc} → gui/canvas/bpp_format_ui.{h,cc}

Add compatibility shims for old paths

Benefit: Better organization, clear namespace structure

Priority 4: Complete Scratch Space Feature (Feature)

Current state:

• Data structures exist (ScratchSpaceSlot)
• No UI implementation

Needed:

• Draw scratch canvas
• Copy/paste between scratch and main map
• Save/load scratch layouts
• UI for managing 4 scratch slots

Priority 5: Simplify Canvas State (Refactoring)

Issue: Dual state management still exists

// Both config_ and legacy variables:
CanvasConfig config_;          // Modern
bool enable_grid_;             // Legacy (duplicate)
float global_scale_;           // Legacy (duplicate)

Goal: Eliminate legacy variables, use config_ only with property accessors

Requires: Careful migration to avoid breaking changes

🔍 Known Working Patterns

Overworld Tile Painting (Working)

void CheckForOverworldEdits() {
  CheckForSelectRectangle();
  
  // Single tile painting - WORKS
  if (!blockset_canvas_.points().empty() &&
      !ow_map_canvas_.select_rect_active() &&
      ow_map_canvas_.DrawTilemapPainter(tile16_blockset_, current_tile16_)) {
    DrawOverworldEdits();  // Paint single tile
  }
  
  // Rectangle painting - BROKEN at boundaries
  if (ow_map_canvas_.select_rect_active() &&
      ImGui::IsMouseClicked(ImGuiMouseButton_Left)) {
    // Paint rectangle - wrapping issue here
  }
}

Blockset Selection (Working)

blockset_canvas_.DrawTileSelector(32);

if (!blockset_canvas_.points().empty() && 
    ImGui::IsMouseClicked(ImGuiMouseButton_Left)) {
  // Get tile from blockset - WORKS
  current_tile16_ = CalculateTileId();
}

Manual Overlay Highlighting (Working)

// Set custom highlight box
blockset_canvas_.mutable_points()->clear();
blockset_canvas_.mutable_points()->push_back(ImVec2(x, y));
blockset_canvas_.mutable_points()->push_back(ImVec2(x + w, y + h));
// Renders as white outline in DrawOverlay()

🎓 Lessons Learned

What Worked

1. ✅ Additive changes - Begin/End alongside legacy (no breakage)
2. ✅ Optional components - CanvasInteractionHandler available when needed
3. ✅ Configurable behavior - Easy revert options
4. ✅ Helper constructors - Simpler API without breaking changes

What Didn't Work

1. ❌ Delegating tile methods - Broke subtle state management
2. ❌ Replacing points management - points_ manipulation is intentional
3. ❌ Simple clamping - Rectangle painting has complex coordinate logic

Key Insights

1. Test runtime behavior - Build success ≠ correct behavior
2. Understand before refactoring - Complex interactions need investigation
3. Preserve working code - If it works, keep original implementation
4. Add, don't replace - New patterns alongside old

📋 For Future Agent

Immediate Task: Fix Rectangle Wrapping

Symptoms:

• Single tile painting: ✅ Works at all boundaries
• Rectangle selection preview: ✅ Clamps correctly
• Rectangle painting: ❌ Paints in wrong location near boundaries

Debugging approach:

1. Add logging to CheckForOverworldEdits() painting loop
2. Log: i, x, y, local_map_x/y, tile16_x/y, index_x/y, tile16_id
3. Compare with expected values
4. Check if world array indexing is correct
5. Verify clamped rectangle size matches original selection size

Files to investigate:

• overworld_editor.cc::CheckForOverworldEdits() (lines 917-1013)
• overworld_editor.cc::CheckForSelectRectangle() (lines 1016-1046)
• canvas.cc::DrawBitmapGroup() (lines 1155-1314)
• canvas.cc::DrawSelectRect() (lines 957-1064)

Key question: Why does single tile painting work but rectangle doesn't?

Medium Term: Namespace Organization

Move all canvas components to canvas/ namespace:

gui/canvas/
├── canvas_utils.{h,cc}              // Move from gui/
├── palette_editor.{h,cc}            // Rename from enhanced_palette_editor
├── bpp_format_ui.{h,cc}            // Move from gui/
├── canvas_interaction_handler.{h,cc} // Already here
├── canvas_modals.{h,cc}             // Already here
├── canvas_context_menu.{h,cc}       // Already here
├── canvas_usage_tracker.{h,cc}      // Already here
└── canvas_performance_integration.{h,cc} // Already here

Add compatibility shims for old paths.

Long Term: State Management Simplification

Current issue: Dual state management

CanvasConfig config_;    // Modern
bool enable_grid_;       // Legacy (duplicate)
float global_scale_;     // Legacy (duplicate)
// ... more duplicates

Goal: Single source of truth

CanvasConfig config_;                          // Only source
bool enable_grid() const { return config_.enable_grid; }  // Accessor
void SetEnableGrid(bool v) { config_.enable_grid = v; }

Requires: Careful migration, test all editors

Stretch Goals: Enhanced Features

1. Scratch Space UI - Complete the scratch canvas implementation
2. Undo/Redo - Integrate with canvas operations
3. Keyboard shortcuts - Add to context menu items
4. Multi-layer rendering - Support sprite overlays

📊 Current Metrics

Metric	Value
Canvas.h	579 lines
Canvas.cc	1873 lines
Components	6 in canvas/ namespace
Documentation	3 focused files
Build status	✅ Compiles
Breaking changes	0
Modern patterns	Available but optional

🔑 Key Files Reference

Core Canvas

• src/app/gui/canvas.h - Main class definition
• src/app/gui/canvas.cc - Implementation

Canvas Components

• src/app/gui/canvas/canvas_interaction_handler.{h,cc} - Optional interaction API
• src/app/gui/canvas/canvas_modals.{h,cc} - Modal dialogs
• src/app/gui/canvas/canvas_context_menu.{h,cc} - Context menu system
• src/app/gui/canvas/canvas_usage_tracker.{h,cc} - Usage analytics
• src/app/gui/canvas/canvas_performance_integration.{h,cc} - Performance monitoring

Utilities

• src/app/gui/canvas_utils.{h,cc} - Helper functions (TODO: move to canvas/)

Major Consumers

• src/app/editor/overworld/overworld_editor.{h,cc} - Primary user, complex interactions
• src/app/editor/overworld/tile16_editor.{h,cc} - Blockset editing
• src/app/editor/graphics/graphics_editor.{h,cc} - Graphics sheet editing
• src/app/editor/dungeon/dungeon_editor.{h,cc} - Dungeon room editing

🎯 Recommended Next Steps

Step 1: Fix Rectangle Wrapping Bug (Critical)

Action: Debug the coordinate calculation in painting loop Time: 2-4 hours
Risk: Medium (affects core functionality)

Approach:

1. Add comprehensive logging
2. Test with specific scenario (e.g., select at x=300-700, drag to x=400-800 in large map)
3. Compare logged values with expected
4. Identify where coordinate calculation goes wrong
5. Apply surgical fix

Step 2: Test All Editors (Verification)

Action: Manual testing of all Canvas usage Time: 1-2 hours Risk: Low (just testing)

Test cases:

• Overworld: Tile painting, rectangle selection, large maps
• Tile16: Blockset selection, tile editing
• Graphics: Sheet display, tile selection
• Dungeon: Room canvas

Step 3: Adopt Modern Patterns (Optional)

Action: Use Begin/End or ScopedCanvas in new features Time: Ongoing Risk: Zero (additive only)

Benefits: Cleaner code, exception safety

📖 Documentation

Read This

• CANVAS_GUIDE.md - Complete feature reference and API documentation
• CANVAS_REFACTORING_STATUS.md - This file (current status)

Background (Optional)

• canvas_refactoring_summary.md - Phase 1 (sizing improvements)
• canvas_refactoring_summary_phase2.md - What we tried and learned

💡 Quick Reference

Modern Usage

canvas.Begin();
canvas.DrawBitmap(bitmap);
canvas.End();

Legacy Usage (Still Works)

canvas.DrawBackground();
canvas.DrawBitmap(bitmap);
canvas.DrawGrid();
canvas.DrawOverlay();

Revert Clamping

canvas.SetClampRectToLocalMaps(false);

Add Context Menu

canvas.AddContextMenuItem({"Action", callback, "Shortcut"});

✅ Current Status

Build: ✅ Compiles without errors
Functionality: ✅ Most features working Known issue: ⚠️ Rectangle wrapping at boundaries
Modern API: ✅ Available and working Documentation: ✅ Consolidated and clear

Ready for: Debugging the rectangle wrapping issue

---

For Future Agent: Start by investigating the coordinate calculation in the painting loop. Add logging, test specific scenarios, and compare actual vs expected values. The fix is likely a small coordinate space conversion issue.