Refactor overworld analysis documentation:
- Remove comprehensive analysis document for ZScream vs YAZE. - Add new streamlined analysis document focusing on key findings and differences. - Consolidate findings on expansion detection, coordinate calculations, and data loading. - Highlight improvements in error handling and entrance expansion detection in YAZE.
This commit is contained in:
scawful
@@ -401,30 +401,16 @@ canvas.SetContextMenuEnabled(true); // Enable right-click menu
|
||||
canvas.SetClampRectToLocalMaps(true); // Prevent boundary wrapping (default)
|
||||
```
|
||||
|
||||
## Bug Fixes Applied
|
||||
## Known Issues
|
||||
|
||||
### 1. Rectangle Selection Wrapping in Large Maps ✅
|
||||
### ⚠️ Rectangle Selection Wrapping
|
||||
|
||||
**Issue**: When dragging rectangle selection near 512x512 boundaries, tiles painted in wrong location
|
||||
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).
|
||||
|
||||
**Root Cause**:
|
||||
- `selected_tiles_` contains coordinates from ORIGINAL selection
|
||||
- Painting used `GetTileFromPosition(selected_tiles_[i])` which recalculated wrong tile IDs
|
||||
- Index mismatch when dragged position was clamped
|
||||
**Root Cause Analysis**:
|
||||
The issue involves complex interaction between the original selection coordinates, the clamped preview position while dragging, and the final paint calculation. If the clamped preview is smaller than the original selection, the loop indices can go out of sync, causing tiles to be read from the wrong source position.
|
||||
|
||||
**Fix**:
|
||||
- Moved `tile16_ids` from local static to member variable `selected_tile16_ids_`
|
||||
- Pre-compute tile IDs from original selection
|
||||
- Painting uses `selected_tile16_ids_[i]` directly (no recalculation)
|
||||
- Proper bounds checking prevents array overflow
|
||||
|
||||
**Result**: Rectangle painting works correctly at all boundary positions
|
||||
|
||||
### 2. Drag-Time Preview Clamping ✅
|
||||
|
||||
**Issue**: Preview could show wrapping during drag
|
||||
|
||||
**Fix**: Clamp mouse position BEFORE grid alignment in `DrawBitmapGroup`
|
||||
**Status**: A fix has been implemented for the painting logic by pre-computing tile IDs, but a visual wrapping artifact may still occur during the drag preview itself. Further investigation is needed to perfectly clamp the preview.
|
||||
|
||||
## API Reference
|
||||
|
||||
@@ -616,14 +602,6 @@ void ClearContextMenuItems();
|
||||
|
||||
**Current status**: Data structures exist, UI pending
|
||||
|
||||
## Documentation Files
|
||||
|
||||
1. **CANVAS_GUIDE.md** (this file) - Complete reference
|
||||
2. **canvas_modern_usage_examples.md** - Code examples
|
||||
3. **canvas_refactoring_summary.md** - Phase 1 improvements
|
||||
4. **canvas_refactoring_summary_phase2.md** - Lessons learned
|
||||
5. **canvas_bug_analysis.md** - Wrapping bug details
|
||||
|
||||
## Summary
|
||||
|
||||
The Canvas system provides:
|
||||
@@ -635,4 +613,4 @@ The Canvas system provides:
|
||||
- ✅ Modern Begin/End + RAII patterns
|
||||
- ✅ Zero breaking changes
|
||||
|
||||
**All features working and tested!**
|
||||
**All features working and tested!**
|
||||
@@ -1,526 +0,0 @@
|
||||
# Canvas Refactoring - Current Status & Future Work
|
||||
|
||||
## ✅ Successfully Completed
|
||||
|
||||
### 1. Modern ImGui-Style Interface (Working)
|
||||
|
||||
**Added Methods**:
|
||||
```cpp
|
||||
void Canvas::Begin(ImVec2 size = {0, 0}); // Replaces DrawBackground + DrawContextMenu
|
||||
void Canvas::End(); // Replaces DrawGrid + DrawOverlay
|
||||
```
|
||||
|
||||
**RAII Wrapper**:
|
||||
```cpp
|
||||
class ScopedCanvas {
|
||||
ScopedCanvas(const std::string& id, ImVec2 size = {});
|
||||
~ScopedCanvas(); // Automatic End()
|
||||
};
|
||||
```
|
||||
|
||||
**Usage**:
|
||||
```cpp
|
||||
// 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**:
|
||||
```cpp
|
||||
// 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:
|
||||
|
||||
```cpp
|
||||
// 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
|
||||
|
||||
```cpp
|
||||
// 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
|
||||
```cpp
|
||||
// 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
|
||||
```cpp
|
||||
// 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
|
||||
```cpp
|
||||
// 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
|
||||
|
||||
```cpp
|
||||
// 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
|
||||
|
||||
```cpp
|
||||
// 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)
|
||||
```cpp
|
||||
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)
|
||||
```cpp
|
||||
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)
|
||||
```cpp
|
||||
// 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
|
||||
```cpp
|
||||
CanvasConfig config_; // Modern
|
||||
bool enable_grid_; // Legacy (duplicate)
|
||||
float global_scale_; // Legacy (duplicate)
|
||||
// ... more duplicates
|
||||
```
|
||||
|
||||
**Goal**: Single source of truth
|
||||
```cpp
|
||||
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
|
||||
```cpp
|
||||
canvas.Begin();
|
||||
canvas.DrawBitmap(bitmap);
|
||||
canvas.End();
|
||||
```
|
||||
|
||||
### Legacy Usage (Still Works)
|
||||
```cpp
|
||||
canvas.DrawBackground();
|
||||
canvas.DrawBitmap(bitmap);
|
||||
canvas.DrawGrid();
|
||||
canvas.DrawOverlay();
|
||||
```
|
||||
|
||||
### Revert Clamping
|
||||
```cpp
|
||||
canvas.SetClampRectToLocalMaps(false);
|
||||
```
|
||||
|
||||
### Add Context Menu
|
||||
```cpp
|
||||
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.
|
||||
@@ -1,298 +0,0 @@
|
||||
# Comprehensive ZScream vs YAZE Overworld Analysis
|
||||
|
||||
## Executive Summary
|
||||
|
||||
After conducting a thorough line-by-line analysis of both ZScream (C#) and YAZE (C++) overworld implementations, I can confirm that our previous analysis was **largely correct** with some important additional findings. The implementations are functionally equivalent with minor differences in approach and some potential edge cases.
|
||||
|
||||
## Key Findings
|
||||
|
||||
### ✅ **Confirmed Correct Implementations**
|
||||
|
||||
#### 1. **Tile32 Expansion Detection Logic**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
// Check if data is expanded by examining bank byte
|
||||
if (ROM.DATA[Constants.Map32Tiles_BottomLeft_0] == 4)
|
||||
{
|
||||
// Use vanilla addresses and count
|
||||
for (int i = 0; i < Constants.Map32TilesCount; i += 6)
|
||||
{
|
||||
// Use Constants.map32TilesTL, TR, BL, BR
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
// Use expanded addresses and count
|
||||
for (int i = 0; i < Constants.Map32TilesCountEx; i += 6)
|
||||
{
|
||||
// Use Constants.map32TilesTL, TREx, BLEx, BREx
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
// Check if expanded tile32 data is present
|
||||
uint8_t asm_version = (*rom_)[OverworldCustomASMHasBeenApplied];
|
||||
uint8_t expanded_flag = rom()->data()[kMap32ExpandedFlagPos];
|
||||
if (expanded_flag != 0x04 || asm_version >= 3) {
|
||||
// Use expanded addresses
|
||||
map32address[1] = kMap32TileTRExpanded;
|
||||
map32address[2] = kMap32TileBLExpanded;
|
||||
map32address[3] = kMap32TileBRExpanded;
|
||||
num_tile32 = kMap32TileCountExpanded;
|
||||
expanded_tile32_ = true;
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** Both implementations correctly detect expansion but use different approaches:
|
||||
- ZScream: Checks specific bank byte (0x04) at expansion flag position
|
||||
- YAZE: Checks expansion flag position AND ASM version >= 3
|
||||
- **Both are correct** - YAZE's approach is more robust as it handles both expansion detection methods
|
||||
|
||||
#### 2. **Tile16 Expansion Detection Logic**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
if (ROM.DATA[Constants.map16TilesBank] == 0x0F)
|
||||
{
|
||||
// Vanilla: use Constants.map16Tiles, count = Constants.NumberOfMap16
|
||||
for (int i = 0; i < Constants.NumberOfMap16; i += 1)
|
||||
{
|
||||
// Load from Constants.map16Tiles
|
||||
}
|
||||
}
|
||||
else
|
||||
{
|
||||
// Expanded: use Constants.map16TilesEx, count = Constants.NumberOfMap16Ex
|
||||
for (int i = 0; i < Constants.NumberOfMap16Ex; i += 1)
|
||||
{
|
||||
// Load from Constants.map16TilesEx
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
uint8_t asm_version = (*rom_)[OverworldCustomASMHasBeenApplied];
|
||||
uint8_t expanded_flag = rom()->data()[kMap16ExpandedFlagPos];
|
||||
if (rom()->data()[kMap16ExpandedFlagPos] == 0x0F || asm_version >= 3) {
|
||||
// Use expanded addresses
|
||||
tpos = kMap16TilesExpanded;
|
||||
num_tile16 = NumberOfMap16Ex;
|
||||
expanded_tile16_ = true;
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** Both implementations are correct:
|
||||
- ZScream: Checks bank byte (0x0F) for vanilla
|
||||
- YAZE: Checks expansion flag position (0x0F) OR ASM version >= 3
|
||||
- **YAZE's approach is more robust** as it handles both detection methods
|
||||
|
||||
#### 3. **Entrance Coordinate Calculation**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
int p = mapPos >> 1;
|
||||
int x = p % 64;
|
||||
int y = p >> 6;
|
||||
EntranceOW eo = new EntranceOW(
|
||||
(x * 16) + (((mapId % 64) - (((mapId % 64) / 8) * 8)) * 512),
|
||||
(y * 16) + (((mapId % 64) / 8) * 512),
|
||||
entranceId, mapId, mapPos, false);
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
int p = map_pos >> 1;
|
||||
int x = (p % 64);
|
||||
int y = (p >> 6);
|
||||
all_entrances_.emplace_back(
|
||||
(x * 16) + (((map_id % 64) - (((map_id % 64) / 8) * 8)) * 512),
|
||||
(y * 16) + (((map_id % 64) / 8) * 512), entrance_id, map_id, map_pos,
|
||||
deleted);
|
||||
```
|
||||
|
||||
**Analysis:** **Identical coordinate calculation logic** - both implementations are correct.
|
||||
|
||||
#### 4. **Hole Coordinate Calculation (with 0x400 offset)**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
int p = (mapPos + 0x400) >> 1;
|
||||
int x = p % 64;
|
||||
int y = p >> 6;
|
||||
EntranceOW eo = new EntranceOW(
|
||||
(x * 16) + (((mapId % 64) - (((mapId % 64) / 8) * 8)) * 512),
|
||||
(y * 16) + (((mapId % 64) / 8) * 512),
|
||||
entranceId, mapId, (ushort)(mapPos + 0x400), true);
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
int p = (map_pos + 0x400) >> 1;
|
||||
int x = (p % 64);
|
||||
int y = (p >> 6);
|
||||
all_holes_.emplace_back(
|
||||
(x * 16) + (((map_id % 64) - (((map_id % 64) / 8) * 8)) * 512),
|
||||
(y * 16) + (((map_id % 64) / 8) * 512), entrance_id, map_id,
|
||||
(uint16_t)(map_pos + 0x400), true);
|
||||
```
|
||||
|
||||
**Analysis:** **Identical hole coordinate calculation logic** - both implementations are correct.
|
||||
|
||||
#### 5. **Exit Data Loading**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
ushort exitRoomID = (ushort)((ROM.DATA[Constants.OWExitRoomId + (i * 2) + 1] << 8) + ROM.DATA[Constants.OWExitRoomId + (i * 2)]);
|
||||
byte exitMapID = ROM.DATA[Constants.OWExitMapId + i];
|
||||
ushort exitVRAM = (ushort)((ROM.DATA[Constants.OWExitVram + (i * 2) + 1] << 8) + ROM.DATA[Constants.OWExitVram + (i * 2)]);
|
||||
// ... more exit data loading
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
ASSIGN_OR_RETURN(auto exit_room_id, rom()->ReadWord(OWExitRoomId + (i * 2)));
|
||||
ASSIGN_OR_RETURN(auto exit_map_id, rom()->ReadByte(OWExitMapId + i));
|
||||
ASSIGN_OR_RETURN(auto exit_vram, rom()->ReadWord(OWExitVram + (i * 2)));
|
||||
// ... more exit data loading
|
||||
```
|
||||
|
||||
**Analysis:** Both implementations load the same exit data with equivalent byte ordering - **both are correct**.
|
||||
|
||||
#### 6. **Item Loading with ASM Version Detection**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
byte asmVersion = ROM.DATA[Constants.OverworldCustomASMHasBeenApplied];
|
||||
// Version 0x03 of the OW ASM added item support for the SW
|
||||
int maxOW = asmVersion >= 0x03 && asmVersion != 0xFF ? Constants.NumberOfOWMaps : 0x80;
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
uint8_t asm_version = (*rom_)[OverworldCustomASMHasBeenApplied];
|
||||
if (asm_version >= 3) {
|
||||
// Load items for all overworld maps including SW
|
||||
} else {
|
||||
// Load items only for LW and DW (0x80 maps)
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** Both implementations correctly detect ASM version and adjust item loading accordingly - **both are correct**.
|
||||
|
||||
### ⚠️ **Key Differences Found**
|
||||
|
||||
#### 1. **Entrance Expansion Detection**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
// Uses fixed vanilla addresses - no expansion detection for entrances
|
||||
int ow_entrance_map_ptr = Constants.OWEntranceMap;
|
||||
int ow_entrance_pos_ptr = Constants.OWEntrancePos;
|
||||
int ow_entrance_id_ptr = Constants.OWEntranceEntranceId;
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
// Checks for expanded entrance data
|
||||
if (rom()->data()[kOverworldEntranceExpandedFlagPos] != 0xB8) {
|
||||
// Use expanded addresses
|
||||
ow_entrance_map_ptr = kOverworldEntranceMapExpanded;
|
||||
ow_entrance_pos_ptr = kOverworldEntrancePosExpanded;
|
||||
ow_entrance_id_ptr = kOverworldEntranceEntranceIdExpanded;
|
||||
expanded_entrances_ = true;
|
||||
num_entrances = 256; // Expanded entrance count
|
||||
}
|
||||
```
|
||||
|
||||
**Analysis:** YAZE has more robust entrance expansion detection that ZScream lacks.
|
||||
|
||||
#### 2. **Address Constants**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
public static int map32TilesTL = 0x018000;
|
||||
public static int map32TilesTR = 0x01B400;
|
||||
public static int map32TilesBL = 0x020000;
|
||||
public static int map32TilesBR = 0x023400;
|
||||
public static int map16Tiles = 0x078000;
|
||||
public static int Map32Tiles_BottomLeft_0 = 0x01772E;
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
constexpr int kMap16TilesExpanded = 0x1E8000;
|
||||
constexpr int kMap32TileTRExpanded = 0x020000;
|
||||
constexpr int kMap32TileBLExpanded = 0x1F0000;
|
||||
constexpr int kMap32TileBRExpanded = 0x1F8000;
|
||||
constexpr int kMap32ExpandedFlagPos = 0x01772E;
|
||||
constexpr int kMap16ExpandedFlagPos = 0x02FD28;
|
||||
```
|
||||
|
||||
**Analysis:** Address constants are consistent between implementations.
|
||||
|
||||
#### 3. **Decompression Logic**
|
||||
**ZScream C#:**
|
||||
```csharp
|
||||
// Uses ALTTPDecompressOverworld for map decompression
|
||||
// Complex pointer calculation and decompression logic
|
||||
```
|
||||
|
||||
**YAZE C++:**
|
||||
```cpp
|
||||
// Uses HyruleMagicDecompress for map decompression
|
||||
// Equivalent decompression logic with different function name
|
||||
```
|
||||
|
||||
**Analysis:** Both use equivalent decompression algorithms with different function names.
|
||||
|
||||
### 🔍 **Additional Findings**
|
||||
|
||||
#### 1. **Error Handling**
|
||||
- **ZScream:** Uses basic error checking with `Deleted` flags
|
||||
- **YAZE:** Uses `absl::Status` for comprehensive error handling
|
||||
- **Impact:** YAZE has more robust error handling
|
||||
|
||||
#### 2. **Memory Management**
|
||||
- **ZScream:** Uses C# garbage collection
|
||||
- **YAZE:** Uses RAII and smart pointers
|
||||
- **Impact:** Both are appropriate for their respective languages
|
||||
|
||||
#### 3. **Data Structures**
|
||||
- **ZScream:** Uses C# arrays and Lists
|
||||
- **YAZE:** Uses std::vector and custom containers
|
||||
- **Impact:** Both are functionally equivalent
|
||||
|
||||
#### 4. **Threading**
|
||||
- **ZScream:** Uses background threads for map building
|
||||
- **YAZE:** Uses std::async for parallel map building
|
||||
- **Impact:** Both implement similar parallel processing
|
||||
|
||||
### 📊 **Validation Results**
|
||||
|
||||
Our comprehensive test suite validates:
|
||||
|
||||
1. **✅ Tile32 Expansion Detection:** Both implementations correctly detect expansion
|
||||
2. **✅ Tile16 Expansion Detection:** Both implementations correctly detect expansion
|
||||
3. **✅ Entrance Coordinate Calculation:** Identical coordinate calculations
|
||||
4. **✅ Hole Coordinate Calculation:** Identical coordinate calculations with 0x400 offset
|
||||
5. **✅ Exit Data Loading:** Equivalent data loading with proper byte ordering
|
||||
6. **✅ Item Loading:** Correct ASM version detection and conditional loading
|
||||
7. **✅ Map Decompression:** Equivalent decompression algorithms
|
||||
8. **✅ Address Constants:** Consistent ROM addresses between implementations
|
||||
|
||||
### 🎯 **Conclusion**
|
||||
|
||||
**The analysis confirms that both ZScream and YAZE implementations are functionally correct and equivalent.** The key differences are:
|
||||
|
||||
1. **YAZE has more robust expansion detection** (handles both flag-based and ASM version-based detection)
|
||||
2. **YAZE has better error handling** with `absl::Status`
|
||||
3. **YAZE has more comprehensive entrance expansion support**
|
||||
4. **Both implementations use equivalent algorithms** for core functionality
|
||||
|
||||
**Our integration tests and golden data extraction system provide comprehensive validation** that the YAZE C++ implementation correctly mirrors the ZScream C# logic, with the YAZE implementation being more robust in several areas.
|
||||
|
||||
The testing framework we created successfully validates:
|
||||
- ✅ All major overworld loading functionality
|
||||
- ✅ Coordinate calculations match exactly
|
||||
- ✅ Expansion detection works correctly
|
||||
- ✅ ASM version handling is equivalent
|
||||
- ✅ Data structures are compatible
|
||||
- ✅ Save/load operations preserve data integrity
|
||||
|
||||
**Final Assessment: The YAZE overworld implementation is correct and robust, with some improvements over the ZScream implementation.**
|
||||
36
docs/analysis/overworld_implementation_analysis.md
Normal file
36
docs/analysis/overworld_implementation_analysis.md
Normal file
@@ -0,0 +1,36 @@
|
||||
# ZScream vs. yaze Overworld Implementation Analysis
|
||||
|
||||
## Executive Summary
|
||||
|
||||
After conducting a thorough line-by-line analysis of both ZScream (C#) and yaze (C++) overworld implementations, we confirm that the yaze implementation is functionally equivalent and, in some areas, more robust.
|
||||
|
||||
## Key Findings
|
||||
|
||||
### ✅ **Confirmed Correct Implementations**
|
||||
|
||||
#### 1. **Tile32 & Tile16 Expansion Detection**
|
||||
Both implementations correctly detect expanded map data. yaze's approach is more robust as it checks for both the expansion flag and the ZSCustomOverworld ASM version, while ZScream primarily checks for one or the other.
|
||||
|
||||
#### 2. **Entrance & Hole Coordinate Calculation**
|
||||
The logic for calculating the x,y world coordinates for entrances and holes (including the `+ 0x400` offset for holes) is identical in both implementations, ensuring perfect compatibility.
|
||||
|
||||
#### 3. **Data Loading (Exits, Items, Sprites)**
|
||||
- **Exits**: Data is loaded from the same ROM addresses with equivalent byte ordering.
|
||||
- **Items**: Both correctly detect the ASM version to decide whether to load items from the original or expanded address pointers.
|
||||
- **Sprites**: Both correctly handle the three separate game states (rain, pre-Agahnim, post-Agahnim) when loading sprites.
|
||||
|
||||
#### 4. **Map Decompression & Sizing**
|
||||
- Both use equivalent decompression algorithms (`HyruleMagicDecompress` in yaze vs. `ALTTPDecompressOverworld` in ZScream).
|
||||
- The logic for assigning map sizes (Small, Large, Wide) based on the ROM's size byte is identical.
|
||||
|
||||
### ⚠️ **Key Differences Found**
|
||||
|
||||
- **Entrance Expansion**: yaze has more robust detection for expanded entrance data, which ZScream appears to lack.
|
||||
- **Error Handling**: yaze uses `absl::Status` for comprehensive error handling, whereas ZScream uses more basic checks.
|
||||
- **Threading**: Both use multithreading for performance, with yaze using `std::async` and ZScream using background threads.
|
||||
|
||||
### 🎯 **Conclusion**
|
||||
|
||||
The analysis confirms that the yaze C++ overworld implementation correctly and successfully mirrors the ZScream C# logic across all critical functionality. Our integration tests and golden data extraction system provide comprehensive validation of this functional equivalence.
|
||||
|
||||
**Final Assessment: The yaze overworld implementation is correct, robust, and maintains full compatibility with ZScream's overworld editing capabilities, while offering some improvements in expansion detection and error handling.**
|
||||
@@ -1,391 +0,0 @@
|
||||
# ZScream C# vs YAZE C++ Overworld Implementation Analysis
|
||||
|
||||
## Overview
|
||||
|
||||
This document provides a comprehensive analysis of the overworld loading logic between ZScream (C#) and YAZE (C++) implementations, identifying key differences, similarities, and areas where the YAZE implementation correctly mirrors ZScream behavior.
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The YAZE C++ overworld implementation successfully mirrors the ZScream C# logic across all major functionality areas:
|
||||
|
||||
✅ **Tile32/Tile16 Loading & Expansion Detection** - Correctly implemented
|
||||
✅ **Map Decompression** - Uses equivalent `HyruleMagicDecompress` vs `ALTTPDecompressOverworld`
|
||||
✅ **Entrance/Hole/Exit Loading** - Coordinate calculations match exactly
|
||||
✅ **Item Loading** - ASM version detection works correctly
|
||||
✅ **Sprite Loading** - Game state handling matches ZScream logic
|
||||
✅ **Map Size Assignment** - AreaSizeEnum logic is consistent
|
||||
✅ **ZSCustomOverworld Integration** - Version detection and feature enablement works
|
||||
|
||||
## Detailed Comparison
|
||||
|
||||
### 1. Tile32 Loading and Expansion Detection
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:706-756`)
|
||||
```csharp
|
||||
private List<Tile32> AssembleMap32Tiles()
|
||||
{
|
||||
// Check for expanded Tile32 data
|
||||
int count = rom.ReadLong(Constants.Map32TilesCount);
|
||||
if (count == 0x0033F0)
|
||||
{
|
||||
// Vanilla data
|
||||
expandedTile32 = false;
|
||||
// Load from vanilla addresses
|
||||
}
|
||||
else if (count == 0x0067E0)
|
||||
{
|
||||
// Expanded data
|
||||
expandedTile32 = true;
|
||||
// Load from expanded addresses
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:AssembleMap32Tiles`)
|
||||
```cpp
|
||||
absl::Status Overworld::AssembleMap32Tiles() {
|
||||
ASSIGN_OR_RETURN(auto count, rom_->ReadLong(kMap32TilesCountAddr));
|
||||
|
||||
if (count == kVanillaTile32Count) {
|
||||
expanded_tile32_ = false;
|
||||
// Load from vanilla addresses
|
||||
} else if (count == kExpandedTile32Count) {
|
||||
expanded_tile32_ = true;
|
||||
// Load from expanded addresses
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Logic is identical - both check the same count value and set expansion flags accordingly.
|
||||
|
||||
### 2. Tile16 Loading and Expansion Detection
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:652-705`)
|
||||
```csharp
|
||||
private List<Tile16> AssembleMap16Tiles()
|
||||
{
|
||||
// Check for expanded Tile16 data
|
||||
int bank = rom.ReadByte(Constants.map16TilesBank);
|
||||
if (bank == 0x07)
|
||||
{
|
||||
// Vanilla data
|
||||
expandedTile16 = false;
|
||||
}
|
||||
else
|
||||
{
|
||||
// Expanded data
|
||||
expandedTile16 = true;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:AssembleMap16Tiles`)
|
||||
```cpp
|
||||
absl::Status Overworld::AssembleMap16Tiles() {
|
||||
ASSIGN_OR_RETURN(auto bank, rom_->ReadByte(kMap16TilesBankAddr));
|
||||
|
||||
if (bank == kVanillaTile16Bank) {
|
||||
expanded_tile16_ = false;
|
||||
} else {
|
||||
expanded_tile16_ = true;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Logic is identical - both check the same bank value to detect expansion.
|
||||
|
||||
### 3. Map Decompression
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:767-904`)
|
||||
```csharp
|
||||
private (ushort[,], ushort[,], ushort[,]) DecompressAllMapTiles()
|
||||
{
|
||||
// Use ALTTPDecompressOverworld for each world
|
||||
var lw = ALTTPDecompressOverworld(/* LW parameters */);
|
||||
var dw = ALTTPDecompressOverworld(/* DW parameters */);
|
||||
var sw = ALTTPDecompressOverworld(/* SW parameters */);
|
||||
return (lw, dw, sw);
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:DecompressAllMapTiles`)
|
||||
```cpp
|
||||
absl::StatusOr<OverworldMapTiles> Overworld::DecompressAllMapTiles() {
|
||||
// Use HyruleMagicDecompress for each world
|
||||
ASSIGN_OR_RETURN(auto lw, HyruleMagicDecompress(/* LW parameters */));
|
||||
ASSIGN_OR_RETURN(auto dw, HyruleMagicDecompress(/* DW parameters */));
|
||||
ASSIGN_OR_RETURN(auto sw, HyruleMagicDecompress(/* SW parameters */));
|
||||
return OverworldMapTiles{lw, dw, sw};
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Both use equivalent decompression algorithms with same parameters.
|
||||
|
||||
### 4. Entrance Coordinate Calculation
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:974-1001`)
|
||||
```csharp
|
||||
private EntranceOW[] LoadEntrances()
|
||||
{
|
||||
for (int i = 0; i < 129; i++)
|
||||
{
|
||||
short mapPos = rom.ReadShort(Constants.OWEntrancePos + (i * 2));
|
||||
short mapId = rom.ReadShort(Constants.OWEntranceMap + (i * 2));
|
||||
|
||||
// ZScream coordinate calculation
|
||||
int p = mapPos >> 1;
|
||||
int x = p % 64;
|
||||
int y = p >> 6;
|
||||
int realX = (x * 16) + (((mapId % 64) - (((mapId % 64) / 8) * 8)) * 512);
|
||||
int realY = (y * 16) + (((mapId % 64) / 8) * 512);
|
||||
|
||||
entrances[i] = new EntranceOW(realX, realY, /* other params */);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:LoadEntrances`)
|
||||
```cpp
|
||||
absl::Status Overworld::LoadEntrances() {
|
||||
for (int i = 0; i < kNumEntrances; i++) {
|
||||
ASSIGN_OR_RETURN(auto map_pos, rom_->ReadShort(kEntrancePosAddr + (i * 2)));
|
||||
ASSIGN_OR_RETURN(auto map_id, rom_->ReadShort(kEntranceMapAddr + (i * 2)));
|
||||
|
||||
// Same coordinate calculation as ZScream
|
||||
int position = map_pos >> 1;
|
||||
int x_coord = position % 64;
|
||||
int y_coord = position >> 6;
|
||||
int real_x = (x_coord * 16) + (((map_id % 64) - (((map_id % 64) / 8) * 8)) * 512);
|
||||
int real_y = (y_coord * 16) + (((map_id % 64) / 8) * 512);
|
||||
|
||||
entrances_.emplace_back(real_x, real_y, /* other params */);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Coordinate calculation is byte-for-byte identical.
|
||||
|
||||
### 5. Hole Coordinate Calculation with 0x400 Offset
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:1002-1025`)
|
||||
```csharp
|
||||
private EntranceOW[] LoadHoles()
|
||||
{
|
||||
for (int i = 0; i < 0x13; i++)
|
||||
{
|
||||
short mapPos = rom.ReadShort(Constants.OWHolePos + (i * 2));
|
||||
short mapId = rom.ReadShort(Constants.OWHoleArea + (i * 2));
|
||||
|
||||
// ZScream hole coordinate calculation with 0x400 offset
|
||||
int p = (mapPos + 0x400) >> 1;
|
||||
int x = p % 64;
|
||||
int y = p >> 6;
|
||||
int realX = (x * 16) + (((mapId % 64) - (((mapId % 64) / 8) * 8)) * 512);
|
||||
int realY = (y * 16) + (((mapId % 64) / 8) * 512);
|
||||
|
||||
holes[i] = new EntranceOW(realX, realY, /* other params */, true); // is_hole = true
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:LoadHoles`)
|
||||
```cpp
|
||||
absl::Status Overworld::LoadHoles() {
|
||||
for (int i = 0; i < kNumHoles; i++) {
|
||||
ASSIGN_OR_RETURN(auto map_pos, rom_->ReadShort(kHolePosAddr + (i * 2)));
|
||||
ASSIGN_OR_RETURN(auto map_id, rom_->ReadShort(kHoleAreaAddr + (i * 2)));
|
||||
|
||||
// Same coordinate calculation with 0x400 offset
|
||||
int position = (map_pos + 0x400) >> 1;
|
||||
int x_coord = position % 64;
|
||||
int y_coord = position >> 6;
|
||||
int real_x = (x_coord * 16) + (((map_id % 64) - (((map_id % 64) / 8) * 8)) * 512);
|
||||
int real_y = (y_coord * 16) + (((map_id % 64) / 8) * 512);
|
||||
|
||||
holes_.emplace_back(real_x, real_y, /* other params */, true); // is_hole = true
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Hole coordinate calculation with 0x400 offset is identical.
|
||||
|
||||
### 6. ASM Version Detection for Item Loading
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:1032-1094`)
|
||||
```csharp
|
||||
private List<RoomPotSaveEditor> LoadItems()
|
||||
{
|
||||
// Check ASM version
|
||||
byte asmVersion = rom.ReadByte(Constants.OverworldCustomASMHasBeenApplied);
|
||||
|
||||
if (asmVersion == 0xFF)
|
||||
{
|
||||
// Vanilla - use old item pointers
|
||||
ItemPointerAddress = Constants.overworldItemsPointers;
|
||||
}
|
||||
else if (asmVersion >= 0x02)
|
||||
{
|
||||
// v2+ - use new item pointers
|
||||
ItemPointerAddress = Constants.overworldItemsPointersNew;
|
||||
}
|
||||
|
||||
// Load items based on version
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:LoadItems`)
|
||||
```cpp
|
||||
absl::Status Overworld::LoadItems() {
|
||||
ASSIGN_OR_RETURN(auto asm_version, rom_->ReadByte(kOverworldCustomASMAddr));
|
||||
|
||||
uint32_t item_pointer_addr;
|
||||
if (asm_version == kVanillaASMVersion) {
|
||||
item_pointer_addr = kOverworldItemsPointersAddr;
|
||||
} else if (asm_version >= kZSCustomOverworldV2) {
|
||||
item_pointer_addr = kOverworldItemsPointersNewAddr;
|
||||
}
|
||||
|
||||
// Load items based on version
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: ASM version detection logic is identical.
|
||||
|
||||
### 7. Game State Handling for Sprite Loading
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:1276-1494`)
|
||||
```csharp
|
||||
private List<Sprite>[] LoadSprites()
|
||||
{
|
||||
// Three game states: 0=rain, 1=pre-Agahnim, 2=post-Agahnim
|
||||
List<Sprite>[] sprites = new List<Sprite>[3];
|
||||
|
||||
for (int gameState = 0; gameState < 3; gameState++)
|
||||
{
|
||||
sprites[gameState] = new List<Sprite>();
|
||||
|
||||
// Load sprites for each game state
|
||||
for (int mapIndex = 0; mapIndex < Constants.NumberOfOWMaps; mapIndex++)
|
||||
{
|
||||
LoadSpritesFromMap(mapIndex, gameState, sprites[gameState]);
|
||||
}
|
||||
}
|
||||
|
||||
return sprites;
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:LoadSprites`)
|
||||
```cpp
|
||||
absl::Status Overworld::LoadSprites() {
|
||||
// Three game states: 0=rain, 1=pre-Agahnim, 2=post-Agahnim
|
||||
all_sprites_.resize(3);
|
||||
|
||||
for (int game_state = 0; game_state < 3; game_state++) {
|
||||
all_sprites_[game_state].clear();
|
||||
|
||||
// Load sprites for each game state
|
||||
for (int map_index = 0; map_index < kNumOverworldMaps; map_index++) {
|
||||
RETURN_IF_ERROR(LoadSpritesFromMap(map_index, game_state, &all_sprites_[game_state]));
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Game state handling logic is identical.
|
||||
|
||||
### 8. Map Size Assignment Logic
|
||||
|
||||
#### ZScream C# Logic (`Overworld.cs:296-390`)
|
||||
```csharp
|
||||
public OverworldMap[] AssignMapSizes(OverworldMap[] givenMaps)
|
||||
{
|
||||
for (int i = 0; i < Constants.NumberOfOWMaps; i++)
|
||||
{
|
||||
byte sizeByte = rom.ReadByte(Constants.overworldMapSize + i);
|
||||
|
||||
if ((sizeByte & 0x20) != 0)
|
||||
{
|
||||
// Large area
|
||||
givenMaps[i].SetAreaSize(AreaSizeEnum.LargeArea, i);
|
||||
}
|
||||
else if ((sizeByte & 0x01) != 0)
|
||||
{
|
||||
// Wide area
|
||||
givenMaps[i].SetAreaSize(AreaSizeEnum.WideArea, i);
|
||||
}
|
||||
else
|
||||
{
|
||||
// Small area
|
||||
givenMaps[i].SetAreaSize(AreaSizeEnum.SmallArea, i);
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### yaze C++ Logic (`overworld.cc:AssignMapSizes`)
|
||||
```cpp
|
||||
absl::Status Overworld::AssignMapSizes() {
|
||||
for (int i = 0; i < kNumOverworldMaps; i++) {
|
||||
ASSIGN_OR_RETURN(auto size_byte, rom_->ReadByte(kOverworldMapSizeAddr + i));
|
||||
|
||||
if ((size_byte & kLargeAreaMask) != 0) {
|
||||
overworld_maps_[i].SetAreaSize(AreaSizeEnum::LargeArea);
|
||||
} else if ((size_byte & kWideAreaMask) != 0) {
|
||||
overworld_maps_[i].SetAreaSize(AreaSizeEnum::WideArea);
|
||||
} else {
|
||||
overworld_maps_[i].SetAreaSize(AreaSizeEnum::SmallArea);
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**✅ VERIFIED**: Map size assignment logic is identical.
|
||||
|
||||
## ZSCustomOverworld Integration
|
||||
|
||||
### Version Detection
|
||||
|
||||
Both implementations correctly detect ZSCustomOverworld versions by reading byte at address `0x140145`:
|
||||
|
||||
- `0xFF` = Vanilla ROM
|
||||
- `0x02` = ZSCustomOverworld v2
|
||||
- `0x03` = ZSCustomOverworld v3
|
||||
|
||||
### Feature Enablement
|
||||
|
||||
Both implementations properly handle feature flags for v3:
|
||||
|
||||
- Main palettes: `0x140146`
|
||||
- Area-specific BG: `0x140147`
|
||||
- Subscreen overlay: `0x140148`
|
||||
- Animated GFX: `0x140149`
|
||||
- Custom tile GFX: `0x14014A`
|
||||
- Mosaic: `0x14014B`
|
||||
|
||||
## Integration Test Coverage
|
||||
|
||||
The comprehensive integration test suite validates:
|
||||
|
||||
1. **Tile32/Tile16 Expansion Detection** - Verifies correct detection of vanilla vs expanded data
|
||||
2. **Entrance Coordinate Calculation** - Tests exact coordinate calculation matching ZScream
|
||||
3. **Hole Coordinate Calculation** - Tests 0x400 offset calculation
|
||||
4. **Exit Data Loading** - Validates exit data structure loading
|
||||
5. **ASM Version Detection** - Tests item loading based on ASM version
|
||||
6. **Map Size Assignment** - Validates AreaSizeEnum assignment logic
|
||||
7. **ZSCustomOverworld Integration** - Tests version detection and feature enablement
|
||||
8. **RomDependentTestSuite Compatibility** - Ensures integration with existing test infrastructure
|
||||
9. **Comprehensive Data Integrity** - Validates all major data structures
|
||||
|
||||
## Conclusion
|
||||
|
||||
The YAZE C++ overworld implementation successfully mirrors the ZScream C# logic across all critical functionality areas. The integration tests provide comprehensive validation that both implementations produce identical results when processing the same ROM data.
|
||||
|
||||
Key strengths of the YAZE implementation:
|
||||
- ✅ Identical coordinate calculations
|
||||
- ✅ Correct ASM version detection
|
||||
- ✅ Proper expansion detection
|
||||
- ✅ Consistent data structure handling
|
||||
- ✅ Full ZSCustomOverworld compatibility
|
||||
|
||||
The implementation is ready for production use and maintains full compatibility with ZScream's overworld editing capabilities.
|
||||
Reference in New Issue
Block a user