docs: add overview of overworld entity system and refactor plans
- Introduced a new document detailing the overworld entity system, including its architecture, core components, and critical bug fixes. - Outlined the ongoing refactor plan aimed at modularizing the system for improved maintainability and compatibility with ZScream features. - Documented specific bug fixes related to entity interaction, property management, and coordinate systems to enhance functionality. Benefits: - Provides a comprehensive reference for developers working on the overworld entity system. - Facilitates understanding of recent changes and future development directions.
This commit is contained in:
122
docs/F5-overworld-entity-system.md
Normal file
122
docs/F5-overworld-entity-system.md
Normal file
@@ -0,0 +1,122 @@
|
||||
# Overworld Entity System
|
||||
|
||||
This document provides a technical overview of the overworld entity system, including critical bug fixes that enable its functionality and the ongoing plan to refactor it for modularity and ZScream feature parity.
|
||||
|
||||
## 1. System Overview
|
||||
|
||||
The overworld entity system manages all interactive objects on the overworld map, such as entrances, exits, items, and sprites. The system is undergoing a refactor to move from a monolithic architecture within the `Overworld` class to a modular design where each entity's save/load logic is handled in dedicated files.
|
||||
|
||||
**Key Goals of the Refactor**:
|
||||
- **Modularity**: Isolate entity logic into testable, self-contained units.
|
||||
- **ZScream Parity**: Achieve feature compatibility with ZScream's entity management, including support for expanded ROM formats.
|
||||
- **Maintainability**: Simplify the `Overworld` class by delegating I/O responsibilities.
|
||||
|
||||
## 2. Core Components & Bug Fixes
|
||||
|
||||
Several critical bugs were fixed to make the entity system functional. Understanding these fixes is key to understanding the system's design.
|
||||
|
||||
### 2.1. Entity Interaction and Hover Detection
|
||||
|
||||
**File**: `src/app/editor/overworld/overworld_entity_renderer.cc`
|
||||
|
||||
- **Problem**: Exit entities were not responding to mouse interactions because the hover state was being improperly reset.
|
||||
- **Fix**: The hover state (`hovered_entity_`) is now reset only once at the beginning of the entity rendering cycle, specifically in `DrawExits()`, which is the first rendering function called. Subsequent functions (`DrawEntrances()`, `DrawItems()`, etc.) can set the hover state without it being cleared, preserving the correct hover priority (last-drawn entity wins).
|
||||
|
||||
```cpp
|
||||
// In DrawExits(), which is called first:
|
||||
hovered_entity_ = nullptr; // Reset hover state at the start of the cycle.
|
||||
|
||||
// In DrawEntrances() and other subsequent renderers:
|
||||
// The reset is removed to allow hover state to persist.
|
||||
```
|
||||
|
||||
### 2.2. Entity Property Popup Save/Cancel Logic
|
||||
|
||||
**File**: `src/app/editor/overworld/entity.cc`
|
||||
|
||||
- **Problem**: The "Done" and "Cancel" buttons in entity property popups had inverted logic, causing changes to be saved on "Cancel" and discarded on "Done".
|
||||
- **Fix**: The `set_done` flag, which controls the popup's return value, is now correctly managed. The "Done" and "Delete" buttons set `set_done = true` to signal a save action, while the "Cancel" button does not, correctly discarding changes.
|
||||
|
||||
```cpp
|
||||
// Corrected logic for the "Done" button in popups
|
||||
if (ImGui::Button(ICON_MD_DONE)) {
|
||||
set_done = true; // Save changes
|
||||
ImGui::CloseCurrentPopup();
|
||||
}
|
||||
|
||||
// Corrected logic for the "Cancel" button
|
||||
if (ImGui::Button(ICON_MD_CANCEL)) {
|
||||
// Discard changes (do not set set_done)
|
||||
ImGui::CloseCurrentPopup();
|
||||
}
|
||||
```
|
||||
|
||||
### 2.3. Exit Entity Coordinate System
|
||||
|
||||
**File**: `src/zelda3/overworld/overworld_exit.h`
|
||||
|
||||
- **Problem**: Saving a vanilla ROM would corrupt exit positions, causing them to load at (0,0). This was because the `OverworldExit` class used `uint8_t` for player coordinates, truncating 16-bit values.
|
||||
- **Fix**: The coordinate-related members of `OverworldExit` were changed to `uint16_t` to match the full 0-4088 coordinate range, achieving parity with ZScream's data structures.
|
||||
|
||||
```cpp
|
||||
// In OverworldExit class definition:
|
||||
class OverworldExit : public GameEntity {
|
||||
public:
|
||||
// ...
|
||||
uint16_t y_player_; // Changed from uint8_t
|
||||
uint16_t x_player_; // Changed from uint8_t
|
||||
uint16_t y_camera_; // Changed from uint8_t
|
||||
uint16_t x_camera_; // Changed from uint8_t
|
||||
// ...
|
||||
};
|
||||
```
|
||||
|
||||
### 2.4. Coordinate Synchronization on Drag
|
||||
|
||||
**File**: `src/zelda3/overworld/overworld_exit.h`
|
||||
|
||||
- **Problem**: When dragging an exit, the visual position (`x_`, `y_`) would update, but the underlying data used for saving (`x_player_`, `y_player_`) would not, leading to a data desync and incorrect saves.
|
||||
- **Fix**: The `UpdateMapProperties` method now explicitly syncs the base entity coordinates to the player coordinates before recalculating scroll and camera values. This ensures that drag operations correctly persist.
|
||||
|
||||
```cpp
|
||||
// In OverworldExit::UpdateMapProperties()
|
||||
void UpdateMapProperties(uint16_t map_id) override {
|
||||
// Sync player position from the base entity coordinates updated by the drag system.
|
||||
x_player_ = static_cast<uint16_t>(x_);
|
||||
y_player_ = static_cast<uint16_t>(y_);
|
||||
|
||||
// Proceed with auto-calculation using the now-correct player coordinates.
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
## 3. Entity I/O Refactoring Plan
|
||||
|
||||
The next phase of development is to extract all entity save and load logic from the monolithic `overworld.cc` into dedicated files.
|
||||
|
||||
### 3.1. File Structure
|
||||
|
||||
New files will be created to handle I/O for each entity type:
|
||||
- `src/zelda3/overworld/overworld_entrance.cc`
|
||||
- `src/zelda3/overworld/overworld_exit.cc`
|
||||
- `src/zelda3/overworld/overworld_item.cc`
|
||||
- `src/zelda3/overworld/overworld_transport.cc` (for new transport/whirlpool support)
|
||||
|
||||
### 3.2. Core Functions
|
||||
|
||||
Each new file will implement a standard set of flat functions:
|
||||
- `LoadAll...()`: Reads all entities of a given type from the ROM.
|
||||
- `SaveAll...()`: Writes all entities of a given type to the ROM.
|
||||
- Helper functions for coordinate calculation and data manipulation, mirroring ZScream's logic.
|
||||
|
||||
### 3.3. ZScream Parity Goals
|
||||
|
||||
The refactor aims to implement key ZScream features:
|
||||
- **Expanded ROM Support**: Correctly read/write from vanilla or expanded ROM addresses for entrances and items.
|
||||
- **Pointer Deduplication**: When saving items, reuse pointers for identical item lists on different maps to conserve space.
|
||||
- **Automatic Coordinate Calculation**: For exits and transports, automatically calculate camera and scroll values based on player position, matching the `UpdateMapStuff` logic in ZScream.
|
||||
- **Transport Entity**: Add full support for transport entities (whirlpools, birds).
|
||||
|
||||
### 3.4. `Overworld` Class Role
|
||||
|
||||
After the refactor, the `Overworld` class will act as a coordinator, delegating all entity I/O to the new, modular functions. Its responsibility will be to hold the entity vectors and orchestrate the calls to the `LoadAll...` and `SaveAll...` functions.
|
||||
@@ -1,70 +1,171 @@
|
||||
# Canvas System Overview
|
||||
# Canvas System Guide
|
||||
|
||||
## Canvas Architecture
|
||||
- **Canvas States**: track `canvas`, `content`, and `draw` rectangles independently; expose size/scale through `CanvasState` inspection panel
|
||||
- **Layer Stack**: background ➝ bitmaps ➝ entity overlays ➝ selection/tooltip layers
|
||||
- **Interaction Modes**: Tile Paint, Tile Select, Rectangle Select, Entity Manipulation, Palette Editing, Diagnostics
|
||||
- **Context Menu**: persistent menu with material icon sections (Mode, View, Info, Bitmap, Palette, BPP, Performance, Layout, Custom)
|
||||
This guide provides a comprehensive overview of the `yaze` canvas system, its architecture, and best practices for integration. It reflects the state of the system after the October 2025 refactoring.
|
||||
|
||||
## Core API Patterns
|
||||
- Modern usage: `Begin/End` (auto grid/overlay, persistent context menu)
|
||||
- Legacy helpers still available (`DrawBackground`, `DrawGrid`, `DrawSelectRect`, etc.)
|
||||
- Unified state snapshot: `CanvasState` exposes geometry, zoom, scroll
|
||||
- Interaction handler manages mode-specific tools (tile brush, select rect, entity gizmo)
|
||||
## 1. Architecture
|
||||
|
||||
## Context Menu Sections
|
||||
- **Mode Selector**: switch modes with icons (Brush, Select, Rect, Bitmap, Palette, BPP, Perf)
|
||||
- **View & Grid**: reset/zoom, toggle grid/labels, advanced/scaling dialogs
|
||||
- **Canvas Info**: real-time canvas/content size, scale, scroll, mouse position
|
||||
- **Bitmap/Palette/BPP**: format conversion, palette analysis, BPP workflows with persistent modals
|
||||
- **Performance**: profiler metrics, dashboard, usage report
|
||||
- **Layout**: draggable toggle, auto resize, grid step
|
||||
- **Custom Actions**: consumer-provided menu items
|
||||
The canvas system was refactored from a monolithic class into a modular, component-based architecture. The main `gui::Canvas` class now acts as a façade, coordinating a set of single-responsibility components and free functions.
|
||||
|
||||
## Interaction Modes & Capabilities
|
||||
- **Tile Painting**: tile16 painter, brush size, finish stroke callbacks
|
||||
- Operations: finish_paint, reset_view, zoom, grid, scaling
|
||||
- **Tile Selection**: multi-select rectangle, copy/paste selection
|
||||
- Operations: select_all, clear_selection, reset_view, zoom, grid, scaling
|
||||
- **Rectangle Selection**: drag-select area, clear selection
|
||||
- Operations: clear_selection, reset_view, zoom, grid, scaling
|
||||
- **Bitmap Editing**: format conversion, bitmap manipulation
|
||||
- Operations: bitmap_convert, palette_edit, bpp_analysis, reset_view, zoom, grid, scaling
|
||||
- **Palette Editing**: inline palette editor, ROM palette picker, color analysis
|
||||
- Operations: palette_edit, palette_analysis, reset_palette, reset_view, zoom, grid, scaling
|
||||
- **BPP Conversion**: format analysis, conversion workflows
|
||||
- Operations: bpp_analysis, bpp_conversion, bitmap_convert, reset_view, zoom, grid, scaling
|
||||
- **Performance Mode**: diagnostics, texture queue, performance overlays
|
||||
- Operations: performance, usage_report, copy_metrics, reset_view, zoom, grid, scaling
|
||||
### Core Principles
|
||||
- **Modular Components**: Logic is broken down into smaller, testable units (e.g., state, rendering, interaction, menus).
|
||||
- **Data-Oriented Design**: Plain-old-data (POD) structs like `CanvasState` and `CanvasConfig` hold state, which is operated on by free functions.
|
||||
- **Backward Compatibility**: The refactor was designed to be 100% backward compatible. Legacy APIs still function, but new patterns are encouraged.
|
||||
- **Editor Agnostic**: The core canvas system has no knowledge of `zelda3` specifics, making it reusable for any editor.
|
||||
|
||||
## Debug & Diagnostics
|
||||
- Persistent modals (`View→Advanced`, `View→Scaling`, `Palette`, `BPP`) stay open until closed
|
||||
- Texture inspector shows current bitmap, VRAM sheet, palette group, usage stats
|
||||
- State overlay: canvas size, content size, global scale, scroll, highlight entity
|
||||
- Performance HUD: operation counts, timing graphs, usage recommendations
|
||||
### Code Organization
|
||||
The majority of the canvas code resides in `src/app/gui/canvas/`.
|
||||
```
|
||||
src/app/gui/canvas/
|
||||
├── canvas.h/cc # Main Canvas class (facade)
|
||||
├── canvas_state.h # POD state structs
|
||||
├── canvas_config.h # Unified configuration struct
|
||||
├── canvas_geometry.h/cc # Geometry calculation helpers
|
||||
├── canvas_rendering.h/cc # Rendering free functions
|
||||
├── canvas_events.h # Interaction event structs
|
||||
├── canvas_interaction.h/cc # Interaction event handlers
|
||||
├── canvas_menu.h/cc # Declarative menu structures
|
||||
├── canvas_menu_builder.h/cc # Fluent API for building menus
|
||||
├── canvas_popup.h/cc # PopupRegistry for persistent popups
|
||||
└── canvas_utils.h/cc # General utility functions
|
||||
```
|
||||
|
||||
## Automation API
|
||||
- CanvasAutomationAPI: tile operations (`SetTileAt`, `SelectRect`), view control (`ScrollToTile`, `SetZoom`), entity manipulation hooks
|
||||
- Exposed through CLI (`z3ed`) and gRPC service, matching UI modes
|
||||
## 2. Core Concepts
|
||||
|
||||
## Integration Steps for Editors
|
||||
1. Construct `Canvas`, set renderer (optional) and ID
|
||||
2. Call `InitializePaletteEditor` and `SetUsageMode`
|
||||
3. Configure available modes: `SetAvailableModes({kTilePainting, kTileSelecting})`
|
||||
4. Register mode callbacks (tile paint finish, selection clear, etc.)
|
||||
5. During frame: `canvas.Begin(size)` → draw bitmaps/entities → `canvas.End()`
|
||||
6. Provide custom menu items via `AddMenuItem`/`AddMenuItem(item, usage)`
|
||||
7. Use `GetConfig()`/`GetSelection()` for state; respond to context menu commands via callback lambda in `Render`
|
||||
### Configuration (`CanvasConfig`)
|
||||
- A single, unified `gui::CanvasConfig` struct (defined in `canvas_config.h`) holds all configuration for a canvas instance.
|
||||
- This includes display settings (grid, labels), sizing, scaling, and usage mode.
|
||||
- This replaces duplicated config structs from previous versions.
|
||||
|
||||
## Migration Checklist
|
||||
- Replace direct `DrawContextMenu` logic with new render callback signature
|
||||
- Move palette/BPP helpers into `canvas/` module; update includes
|
||||
- Ensure persistent modals wired (advanced/scaling/palette/bpp/perf)
|
||||
- Update usage tracker integrations to record mode switches
|
||||
- Validate overworld/tile16/dungeon editors in tile paint, select, entity modes
|
||||
### State (`CanvasState`)
|
||||
- A POD struct (`canvas_state.h`) that holds the dynamic state of the canvas, including geometry, zoom, and scroll.
|
||||
- Editors can inspect this state for custom rendering and logic.
|
||||
|
||||
## Testing Notes
|
||||
- Manual regression: overworld paint/select, tile16 painter, dungeon entity drag
|
||||
- Verify context menu persists and modals remain until closed
|
||||
- Ensure palette/BPP modals populate with correct bitmap/palette data
|
||||
- Automation: run CanvasAutomation API tests/end-to-end scripts for overworld edits
|
||||
### Coordinate Systems
|
||||
The canvas operates with three distinct coordinate spaces. Using the correct one is critical to avoid bugs.
|
||||
|
||||
1. **Screen Space**: Absolute pixel coordinates on the monitor (from `ImGui::GetIO().MousePos`). **Never use this for canvas logic.**
|
||||
2. **Canvas/World Space**: Coordinates relative to the canvas's content, accounting for scrolling and panning. Use `Canvas::hover_mouse_pos()` to get this. This is the correct space for entity positioning and high-level calculations.
|
||||
3. **Tile/Grid Space**: Coordinates in tile units. Use `Canvas::CanvasToTile()` to convert from world space.
|
||||
|
||||
A critical fix was made to ensure `Canvas::hover_mouse_pos()` is updated continuously whenever the canvas is hovered, decoupling it from specific actions like painting.
|
||||
|
||||
## 3. Interaction System
|
||||
|
||||
The canvas supports several interaction modes, managed via the `CanvasUsage` enum.
|
||||
|
||||
### Interaction Modes
|
||||
- `kTilePainting`: For painting tiles onto a tilemap.
|
||||
- `kTileSelection`: For selecting one or more tiles.
|
||||
- `kRectangleSelection`: For drag-selecting a rectangular area.
|
||||
- `kEntityManipulation`: For moving and interacting with entities.
|
||||
- `kPaletteEditing`: For palette-related work.
|
||||
- `kDiagnostics`: For performance and debug overlays.
|
||||
|
||||
Set the mode using `canvas.SetUsageMode(gui::CanvasUsage::kTilePainting)`. This ensures the context menu and interaction handlers behave correctly.
|
||||
|
||||
### Event-Driven Model
|
||||
Interaction logic is moving towards an event-driven model. Instead of inspecting canvas state directly, editors should handle events returned by interaction functions.
|
||||
|
||||
**Example**:
|
||||
```cpp
|
||||
RectSelectionEvent event = HandleRectangleSelection(geometry, ...);
|
||||
if (event.is_complete) {
|
||||
// Process the selection event
|
||||
}
|
||||
```
|
||||
|
||||
## 4. Context Menu & Popups
|
||||
|
||||
The context menu system is now unified, data-driven, and supports persistent popups.
|
||||
|
||||
### Key Features
|
||||
- **Unified Item Definition**: All menu items use the `gui::CanvasMenuItem` struct.
|
||||
- **Priority-Based Ordering**: Menu sections are automatically sorted based on the `MenuSectionPriority` enum, ensuring a consistent layout:
|
||||
1. `kEditorSpecific` (highest priority)
|
||||
2. `kBitmapOperations`
|
||||
3. `kCanvasProperties`
|
||||
4. `kDebug` (lowest priority)
|
||||
- **Automatic Popup Persistence**: Popups defined declaratively will remain open until explicitly closed by the user (ESC or close button), rather than closing on any click outside.
|
||||
- **Fluent Builder API**: The `gui::CanvasMenuBuilder` provides a clean, chainable API for constructing complex menus.
|
||||
|
||||
### API Patterns
|
||||
|
||||
**Add a Simple Menu Item**:
|
||||
```cpp
|
||||
canvas.AddContextMenuItem(
|
||||
gui::CanvasMenuItem("Label", ICON_MD_ICON, []() { /* Action */ })
|
||||
);
|
||||
```
|
||||
|
||||
**Add a Declarative Popup Item**:
|
||||
This pattern automatically handles popup registration and persistence.
|
||||
```cpp
|
||||
auto item = gui::CanvasMenuItem::WithPopup(
|
||||
"Properties",
|
||||
"props_popup_id",
|
||||
[]() {
|
||||
// Render popup content here
|
||||
ImGui::Text("My Properties");
|
||||
}
|
||||
);
|
||||
canvas.AddContextMenuItem(item);
|
||||
```
|
||||
|
||||
**Build a Complex Menu with the Builder**:
|
||||
```cpp
|
||||
gui::CanvasMenuBuilder builder;
|
||||
canvas.editor_menu() = builder
|
||||
.BeginSection("Editor Actions", gui::MenuSectionPriority::kEditorSpecific)
|
||||
.AddItem("Cut", ICON_MD_CUT, []() { Cut(); })
|
||||
.AddPopupItem("Settings", "settings_popup", []() { RenderSettings(); })
|
||||
.EndSection()
|
||||
.Build();
|
||||
```
|
||||
|
||||
## 5. Entity System
|
||||
|
||||
A generic, Zelda-agnostic entity system allows editors to manage on-canvas objects.
|
||||
|
||||
- **Flat Functions**: Entity creation logic is handled by pure functions in `src/app/editor/overworld/operations/entity_operations.h`, such as `InsertEntrance`, `InsertSprite`, etc. These functions are designed for ZScream feature parity.
|
||||
- **Delegation Pattern**: The `OverworldEditor` delegates to the `MapPropertiesSystem`, which in turn calls these flat functions to modify the ROM state.
|
||||
- **Mode-Aware Menu**: The "Insert Entity" context submenu is only available when the canvas is in `kEntityManipulation` mode.
|
||||
|
||||
**Usage Flow**:
|
||||
1. Set canvas mode to `kEntityManipulation`.
|
||||
2. Right-click on the canvas to open the context menu.
|
||||
3. Select "Insert Entity" and choose the entity type.
|
||||
4. The appropriate callback is fired, which calls the corresponding `Insert...` function.
|
||||
5. A popup appears to configure the new entity's properties.
|
||||
|
||||
## 6. Integration Guide for Editors
|
||||
|
||||
1. **Construct `Canvas`**: Instantiate `gui::Canvas`, providing a unique ID.
|
||||
2. **Configure**: Set the desired `CanvasUsage` mode via `canvas.SetUsageMode()`. Configure available modes and other options in the `CanvasConfig` struct.
|
||||
3. **Register Callbacks**: If using interaction modes like tile painting, register callbacks for events like `finish_paint`.
|
||||
4. **Render Loop**:
|
||||
- Call `canvas.Begin(size)`.
|
||||
- Draw your editor-specific content (bitmaps, entities, overlays).
|
||||
- Call `canvas.End()`. This handles rendering the grid, overlays, and the context menu.
|
||||
5. **Provide Custom Menus**: Use `canvas.AddContextMenuItem()` or the `CanvasMenuBuilder` to add editor-specific actions to the context menu. Assign the `kEditorSpecific` priority to ensure they appear at the top.
|
||||
6. **Handle State**: Respond to user interactions by inspecting the `CanvasState` or handling events returned from interaction helpers.
|
||||
|
||||
## 7. Debugging
|
||||
|
||||
If you encounter issues with the canvas, check the following:
|
||||
|
||||
- **Context Menu Doesn't Appear**:
|
||||
- Is `config.enable_context_menu` true?
|
||||
- Is the mouse button right-click?
|
||||
- Is the canvas focused and not being dragged?
|
||||
- **Popup Doesn't Persist**:
|
||||
- Are you using the `CanvasMenuItem::WithPopup` pattern?
|
||||
- Is `canvas.End()` being called every frame to allow the `PopupRegistry` to render?
|
||||
- **Incorrect Coordinates**:
|
||||
- Are you using `canvas.hover_mouse_pos()` for world coordinates instead of `ImGui::GetIO().MousePos`?
|
||||
- Verify that you are correctly converting between world space and tile space.
|
||||
- **Menu Items in Wrong Order**:
|
||||
- Have you set the correct `MenuSectionPriority` for your custom menu sections?
|
||||
|
||||
## 8. Automation API
|
||||
|
||||
The `CanvasAutomationAPI` provides hooks for testing and automation. It allows for programmatic control of tile operations (`SetTileAt`, `SelectRect`), view controls (`ScrollToTile`, `SetZoom`), and entity manipulation. This API is exposed via the `z3ed` CLI and a gRPC service.
|
||||
@@ -1,401 +0,0 @@
|
||||
# Canvas Coordinate Synchronization and Scroll Fix
|
||||
|
||||
**Date**: October 10, 2025
|
||||
**Issues**:
|
||||
1. Overworld map highlighting regression after canvas refactoring
|
||||
2. Overworld canvas scrolling unexpectedly when selecting tiles
|
||||
3. Vanilla Dark/Special World large map outlines not displaying
|
||||
**Status**: Fixed
|
||||
|
||||
## Problem Summary
|
||||
|
||||
After the canvas refactoring (commits f538775954, 60ddf76331), two critical bugs appeared:
|
||||
|
||||
1. **Map highlighting broken**: The overworld editor stopped properly highlighting the current map when hovering. The map highlighting only worked during active tile painting, not during normal mouse hover.
|
||||
|
||||
2. **Wrong canvas scrolling**: When right-clicking to select tiles (especially on Dark World), the overworld canvas would scroll unexpectedly instead of the tile16 blockset selector.
|
||||
|
||||
## Root Cause
|
||||
|
||||
The regression had **FIVE** issues:
|
||||
|
||||
### Issue 1: Wrong Coordinate System (Line 1041)
|
||||
**File**: `src/app/editor/overworld/overworld_editor.cc:1041`
|
||||
|
||||
**Before (BROKEN)**:
|
||||
```cpp
|
||||
const auto mouse_position = ImGui::GetIO().MousePos; // ❌ Screen coordinates!
|
||||
const auto canvas_zero_point = ow_map_canvas_.zero_point();
|
||||
int map_x = (mouse_position.x - canvas_zero_point.x) / kOverworldMapSize;
|
||||
```
|
||||
|
||||
**After (FIXED)**:
|
||||
```cpp
|
||||
const auto mouse_position = ow_map_canvas_.hover_mouse_pos(); // World coordinates!
|
||||
int map_x = mouse_position.x / kOverworldMapSize;
|
||||
```
|
||||
|
||||
**Why This Was Wrong**:
|
||||
- `ImGui::GetIO().MousePos` returns **screen space** coordinates (absolute position on screen)
|
||||
- The canvas may be scrolled, scaled, or positioned anywhere on screen
|
||||
- Screen coordinates don't account for canvas scrolling/offset
|
||||
- `hover_mouse_pos()` returns **canvas/world space** coordinates (relative to canvas content)
|
||||
|
||||
### Issue 2: Hover Position Not Updated (Line 416)
|
||||
**File**: `src/app/gui/canvas.cc:416`
|
||||
|
||||
**Before (BROKEN)**:
|
||||
```cpp
|
||||
void Canvas::DrawBackground(ImVec2 canvas_size) {
|
||||
// ... setup code ...
|
||||
ImGui::InvisibleButton(canvas_id_.c_str(), scaled_size, kMouseFlags);
|
||||
|
||||
// ❌ mouse_pos_in_canvas_ only updated in DrawTilePainter() during painting!
|
||||
|
||||
if (config_.is_draggable && IsItemHovered()) {
|
||||
// ... pan handling ...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`mouse_pos_in_canvas_` was only updated inside painting methods:
|
||||
- `DrawTilePainter()` at line 741
|
||||
- `DrawSolidTilePainter()` at line 860
|
||||
- `DrawTileSelector()` at line 929
|
||||
|
||||
**After (FIXED)**:
|
||||
```cpp
|
||||
void Canvas::DrawBackground(ImVec2 canvas_size) {
|
||||
// ... setup code ...
|
||||
ImGui::InvisibleButton(canvas_id_.c_str(), scaled_size, kMouseFlags);
|
||||
|
||||
// CRITICAL FIX: Always update hover position when hovering
|
||||
if (IsItemHovered()) {
|
||||
const ImGuiIO& io = GetIO();
|
||||
const ImVec2 origin(canvas_p0_.x + scrolling_.x, canvas_p0_.y + scrolling_.y);
|
||||
const ImVec2 mouse_pos(io.MousePos.x - origin.x, io.MousePos.y - origin.y);
|
||||
mouse_pos_in_canvas_ = mouse_pos; // Updated every frame during hover
|
||||
}
|
||||
|
||||
if (config_.is_draggable && IsItemHovered()) {
|
||||
// ... pan handling ...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## Technical Details
|
||||
|
||||
### Coordinate Spaces
|
||||
|
||||
yaze has three coordinate spaces:
|
||||
|
||||
1. **Screen Space**: Absolute pixel coordinates on the monitor
|
||||
- `ImGui::GetIO().MousePos` returns this
|
||||
- Never use this for canvas operations!
|
||||
|
||||
2. **Canvas/World Space**: Coordinates relative to canvas content
|
||||
- Accounts for canvas scrolling and offset
|
||||
- `Canvas::hover_mouse_pos()` returns this
|
||||
- Use this for map calculations, entity positioning, etc.
|
||||
|
||||
3. **Tile/Grid Space**: Coordinates in tile units (not pixels)
|
||||
- `Canvas::CanvasToTile()` converts from canvas to grid space
|
||||
- Used by automation API
|
||||
|
||||
### Usage Patterns
|
||||
|
||||
**For Hover/Highlighting** (CheckForCurrentMap):
|
||||
```cpp
|
||||
auto hover_pos = canvas.hover_mouse_pos(); // Updates continuously
|
||||
int map_x = hover_pos.x / kOverworldMapSize;
|
||||
```
|
||||
|
||||
**For Active Painting** (DrawOverworldEdits):
|
||||
```cpp
|
||||
auto paint_pos = canvas.drawn_tile_position(); // Updates only during drag
|
||||
int map_x = paint_pos.x / kOverworldMapSize;
|
||||
```
|
||||
|
||||
## Testing
|
||||
|
||||
### Visual Testing
|
||||
|
||||
**Map Highlighting Test**:
|
||||
1. Open overworld editor
|
||||
2. Hover mouse over different maps (without clicking)
|
||||
3. Verify current map highlights correctly
|
||||
4. Test with different scale levels (0.25x - 4.0x)
|
||||
5. Test with scrolled canvas
|
||||
|
||||
**Scroll Regression Test**:
|
||||
1. Open overworld editor
|
||||
2. Switch to Dark World (or any world)
|
||||
3. Right-click on overworld canvas to select a tile
|
||||
4. **Expected**: Tile16 blockset selector scrolls to show the selected tile
|
||||
5. **Expected**: Overworld canvas does NOT scroll
|
||||
6. ❌ **Before fix**: Overworld canvas would scroll unexpectedly
|
||||
|
||||
### Unit Tests
|
||||
Created `test/unit/gui/canvas_coordinate_sync_test.cc` with regression tests:
|
||||
- `HoverMousePos_IndependentFromDrawnPos`: Verifies hover vs paint separation
|
||||
- `CoordinateSpace_WorldNotScreen`: Ensures world coordinates used
|
||||
- `MapCalculation_SmallMaps`: Tests 512x512 map boundaries
|
||||
- `MapCalculation_LargeMaps`: Tests 1024x1024 v3 ASM maps
|
||||
- `OverworldMapHighlight_UsesHoverNotDrawn`: Critical regression test
|
||||
- `OverworldMapIndex_From8x8Grid`: Tests all three worlds (Light/Dark/Special)
|
||||
|
||||
Run tests:
|
||||
```bash
|
||||
./build/bin/yaze_test --unit
|
||||
```
|
||||
|
||||
## Impact Analysis
|
||||
|
||||
### Files Changed
|
||||
1. `src/app/editor/overworld/overworld_editor.cc` (line 1041-1049)
|
||||
- Changed from screen coordinates to canvas hover coordinates
|
||||
- Removed incorrect `canvas_zero_point` subtraction
|
||||
|
||||
2. `src/app/gui/canvas.cc` (line 414-421)
|
||||
- Added continuous hover position tracking in `DrawBackground()`
|
||||
- Now updates `mouse_pos_in_canvas_` every frame when hovering
|
||||
|
||||
3. `src/app/editor/overworld/overworld_editor.cc` (line 2344-2360)
|
||||
- Removed fallback scroll code that scrolled the wrong canvas
|
||||
- Now only uses `blockset_selector_->ScrollToTile()` which targets the correct canvas
|
||||
|
||||
4. `src/app/editor/overworld/overworld_editor.cc` (line 1403-1408)
|
||||
- Changed from `ImGui::IsItemHovered()` (checks last drawn item)
|
||||
- To `ow_map_canvas_.IsMouseHovering()` (checks canvas hover state directly)
|
||||
|
||||
5. `src/app/editor/overworld/overworld_editor.cc` (line 1133-1151)
|
||||
- Added world offset subtraction for vanilla large map parent coordinates
|
||||
- Now properly accounts for Dark World (0x40-0x7F) and Special World (0x80-0x9F)
|
||||
|
||||
### Affected Functionality
|
||||
- **Fixed**: Overworld map highlighting during hover (all worlds, all ROM types)
|
||||
- **Fixed**: Vanilla Dark World large map highlighting (was drawing off-screen)
|
||||
- **Fixed**: Vanilla Special World large map highlighting (was drawing off-screen)
|
||||
- **Fixed**: Overworld canvas no longer scrolls when selecting tiles
|
||||
- **Fixed**: Tile16 selector properly scrolls to show selected tile (via blockset_selector_)
|
||||
- **Fixed**: Entity renderer using `hover_mouse_pos()` (already worked correctly)
|
||||
- **Preserved**: Tile painting using `drawn_tile_position()` (unchanged)
|
||||
- **Preserved**: Multi-area map support (512x512, 1024x1024)
|
||||
- **Preserved**: All three worlds (Light 0x00-0x3F, Dark 0x40-0x7F, Special 0x80+)
|
||||
- **Preserved**: ZSCustomOverworld v3 large maps (already worked correctly)
|
||||
|
||||
### Related Code That Works Correctly
|
||||
These files already use the correct pattern:
|
||||
- `src/app/editor/overworld/overworld_entity_renderer.cc:68-69` - Uses `hover_mouse_pos()` for entity placement
|
||||
- `src/app/editor/overworld/overworld_editor.cc:664` - Uses `drawn_tile_position()` for painting
|
||||
|
||||
## Multi-Area Map Support
|
||||
|
||||
The fix properly handles all area sizes:
|
||||
|
||||
### Standard Maps (512x512)
|
||||
```cpp
|
||||
int map_x = hover_pos.x / 512; // 0-7 range
|
||||
int map_y = hover_pos.y / 512; // 0-7 range
|
||||
int map_index = map_x + map_y * 8; // 0-63 (8x8 grid)
|
||||
```
|
||||
|
||||
### ZSCustomOverworld v3 Large Maps (1024x1024)
|
||||
```cpp
|
||||
int map_x = hover_pos.x / 1024; // Large map X
|
||||
int map_y = hover_pos.y / 1024; // Large map Y
|
||||
// Parent map calculation handled in lines 1073-1190
|
||||
```
|
||||
|
||||
The existing multi-area logic (lines 1068-1190) remains unchanged and works correctly with the fix.
|
||||
|
||||
## Issue 3: Wrong Canvas Being Scrolled (Line 2344-2366)
|
||||
|
||||
**File**: `src/app/editor/overworld/overworld_editor.cc:2344`
|
||||
|
||||
**Problem**: When selecting tiles with right-click on the overworld canvas, `ScrollBlocksetCanvasToCurrentTile()` was calling `ImGui::SetScrollX/Y()` which scrolls **the current ImGui window**, not a specific canvas.
|
||||
|
||||
**Call Stack**:
|
||||
```
|
||||
DrawOverworldCanvas() // Overworld canvas is current window
|
||||
└─ CheckForOverworldEdits() (line 1401)
|
||||
└─ CheckForSelectRectangle() (line 793)
|
||||
└─ ScrollBlocksetCanvasToCurrentTile() (line 916)
|
||||
└─ ImGui::SetScrollX/Y() (lines 2364-2365) // ❌ Scrolls CURRENT window!
|
||||
```
|
||||
|
||||
**Before (BROKEN)**:
|
||||
```cpp
|
||||
void OverworldEditor::ScrollBlocksetCanvasToCurrentTile() {
|
||||
if (blockset_selector_) {
|
||||
blockset_selector_->ScrollToTile(current_tile16_);
|
||||
return;
|
||||
}
|
||||
|
||||
// Fallback: maintain legacy behavior when the selector is unavailable.
|
||||
constexpr int kTilesPerRow = 8;
|
||||
constexpr int kTileDisplaySize = 32;
|
||||
|
||||
int tile_col = current_tile16_ % kTilesPerRow;
|
||||
int tile_row = current_tile16_ / kTilesPerRow;
|
||||
float tile_x = static_cast<float>(tile_col * kTileDisplaySize);
|
||||
float tile_y = static_cast<float>(tile_row * kTileDisplaySize);
|
||||
|
||||
const ImVec2 window_size = ImGui::GetWindowSize();
|
||||
float scroll_x = tile_x - (window_size.x / 2.0F) + (kTileDisplaySize / 2.0F);
|
||||
float scroll_y = tile_y - (window_size.y / 2.0F) + (kTileDisplaySize / 2.0F);
|
||||
|
||||
// ❌ BUG: This scrolls whatever ImGui window is currently active!
|
||||
// When called from overworld canvas, it scrolls the overworld instead of tile16 selector!
|
||||
ImGui::SetScrollX(std::max(0.0f, scroll_x));
|
||||
ImGui::SetScrollY(std::max(0.0f, scroll_y));
|
||||
}
|
||||
```
|
||||
|
||||
**After (FIXED)**:
|
||||
```cpp
|
||||
void OverworldEditor::ScrollBlocksetCanvasToCurrentTile() {
|
||||
if (blockset_selector_) {
|
||||
blockset_selector_->ScrollToTile(current_tile16_); // Correct: Targets specific canvas
|
||||
return;
|
||||
}
|
||||
|
||||
// CRITICAL FIX: Do NOT use fallback scrolling from overworld canvas context!
|
||||
// The fallback code uses ImGui::SetScrollX/Y which scrolls the CURRENT window,
|
||||
// and when called from CheckForSelectRectangle() during overworld canvas rendering,
|
||||
// it incorrectly scrolls the overworld canvas instead of the tile16 selector.
|
||||
//
|
||||
// The blockset_selector_ should always be available in modern code paths.
|
||||
// If it's not available, we skip scrolling rather than scroll the wrong window.
|
||||
}
|
||||
```
|
||||
|
||||
**Why This Fixes It**:
|
||||
- The modern `blockset_selector_->ScrollToTile()` targets the specific tile16 selector canvas
|
||||
- The fallback `ImGui::SetScrollX/Y()` has no context - it just scrolls the active window
|
||||
- By removing the fallback, we prevent scrolling the wrong canvas
|
||||
- If `blockset_selector_` is null (shouldn't happen in modern builds), we safely do nothing instead of breaking user interaction
|
||||
|
||||
## Issue 4: Wrong Hover Check (Line 1403)
|
||||
|
||||
**File**: `src/app/editor/overworld/overworld_editor.cc:1403`
|
||||
|
||||
**Problem**: The code was using `ImGui::IsItemHovered()` to check if the mouse was over the canvas, but this checks the **last drawn ImGui item**, which could be entities, overlays, or anything drawn after the canvas's InvisibleButton. This meant hover detection was completely broken.
|
||||
|
||||
**Call Stack**:
|
||||
```
|
||||
DrawOverworldCanvas()
|
||||
└─ DrawBackground() at line 1350 // Creates InvisibleButton (item A)
|
||||
└─ DrawExits/Entrances/Items/Sprites() // Draws entities (items B, C, D...)
|
||||
└─ DrawOverlayPreviewOnMap() // Draws overlay (item E)
|
||||
└─ IsItemHovered() at line 1403 // ❌ Checks item E, not item A!
|
||||
```
|
||||
|
||||
**Before (BROKEN)**:
|
||||
```cpp
|
||||
if (current_mode == EditingMode::DRAW_TILE) {
|
||||
CheckForOverworldEdits();
|
||||
}
|
||||
if (IsItemHovered()) // ❌ Checks LAST item (overlay/entity), not canvas!
|
||||
status_ = CheckForCurrentMap();
|
||||
```
|
||||
|
||||
**After (FIXED)**:
|
||||
```cpp
|
||||
if (current_mode == EditingMode::DRAW_TILE) {
|
||||
CheckForOverworldEdits();
|
||||
}
|
||||
// CRITICAL FIX: Use canvas hover state, not ImGui::IsItemHovered()
|
||||
// IsItemHovered() checks the LAST drawn item, which could be entities/overlay,
|
||||
// not the canvas InvisibleButton. ow_map_canvas_.IsMouseHovering() correctly
|
||||
// tracks whether mouse is over the canvas area.
|
||||
if (ow_map_canvas_.IsMouseHovering()) // Checks canvas hover state directly
|
||||
status_ = CheckForCurrentMap();
|
||||
```
|
||||
|
||||
**Why This Fixes It**:
|
||||
- `IsItemHovered()` is context-sensitive - it checks whatever the last `ImGui::*()` call was
|
||||
- After drawing entities and overlays, the "last item" is NOT the canvas
|
||||
- `Canvas::IsMouseHovering()` tracks the hover state from the InvisibleButton in `DrawBackground()`
|
||||
- This state is set correctly when the InvisibleButton is hovered (line 416 in canvas.cc)
|
||||
|
||||
## Issue 5: Vanilla Large Map World Offset (Line 1132-1136)
|
||||
|
||||
**File**: `src/app/editor/overworld/overworld_editor.cc:1132-1136`
|
||||
|
||||
**Problem**: For vanilla ROMs, the large map highlighting logic wasn't accounting for world offsets when calculating parent map coordinates. Dark World maps (0x40-0x7F) and Special World maps (0x80-0x9F) use map IDs with offsets, but the display grid coordinates are 0-7.
|
||||
|
||||
**Before (BROKEN)**:
|
||||
```cpp
|
||||
if (overworld_.overworld_map(current_map_)->is_large_map() ||
|
||||
overworld_.overworld_map(current_map_)->large_index() != 0) {
|
||||
const int highlight_parent =
|
||||
overworld_.overworld_map(current_highlighted_map)->parent();
|
||||
const int parent_map_x = highlight_parent % 8; // ❌ Wrong for Dark/Special!
|
||||
const int parent_map_y = highlight_parent / 8;
|
||||
ow_map_canvas_.DrawOutline(parent_map_x * kOverworldMapSize,
|
||||
parent_map_y * kOverworldMapSize,
|
||||
large_map_size, large_map_size);
|
||||
}
|
||||
```
|
||||
|
||||
**Example Bug**:
|
||||
- Dark World map 0x42 (parent) → `0x42 % 8 = 2`, `0x42 / 8 = 8`
|
||||
- This draws the outline at grid position (2, 8) which is **off the screen**!
|
||||
- Correct position should be (2, 0) in the Dark World display grid
|
||||
|
||||
**After (FIXED)**:
|
||||
```cpp
|
||||
if (overworld_.overworld_map(current_map_)->is_large_map() ||
|
||||
overworld_.overworld_map(current_map_)->large_index() != 0) {
|
||||
const int highlight_parent =
|
||||
overworld_.overworld_map(current_highlighted_map)->parent();
|
||||
|
||||
// CRITICAL FIX: Account for world offset when calculating parent coordinates
|
||||
int parent_map_x;
|
||||
int parent_map_y;
|
||||
if (current_world_ == 0) {
|
||||
// Light World (0x00-0x3F)
|
||||
parent_map_x = highlight_parent % 8;
|
||||
parent_map_y = highlight_parent / 8;
|
||||
} else if (current_world_ == 1) {
|
||||
// Dark World (0x40-0x7F) - subtract 0x40 to get display coordinates
|
||||
parent_map_x = (highlight_parent - 0x40) % 8;
|
||||
parent_map_y = (highlight_parent - 0x40) / 8;
|
||||
} else {
|
||||
// Special World (0x80-0x9F) - subtract 0x80 to get display coordinates
|
||||
parent_map_x = (highlight_parent - 0x80) % 8;
|
||||
parent_map_y = (highlight_parent - 0x80) / 8;
|
||||
}
|
||||
|
||||
ow_map_canvas_.DrawOutline(parent_map_x * kOverworldMapSize,
|
||||
parent_map_y * kOverworldMapSize,
|
||||
large_map_size, large_map_size);
|
||||
}
|
||||
```
|
||||
|
||||
**Why This Fixes It**:
|
||||
- Map IDs are **absolute**: Light World 0x00-0x3F, Dark World 0x40-0x7F, Special 0x80-0x9F
|
||||
- Display coordinates are **relative**: Each world displays in an 8x8 grid (0-7, 0-7)
|
||||
- Without subtracting the world offset, coordinates overflow the display grid
|
||||
- This matches the same logic used for v3 large maps (lines 1084-1096) and small maps (lines 1141-1172)
|
||||
|
||||
## Commit Reference
|
||||
|
||||
**Canvas Refactoring Commits**:
|
||||
- `f538775954` - Organize Canvas Utilities and BPP Format Management
|
||||
- `60ddf76331` - Integrate Canvas Automation API and Simplify Overworld Editor Controls
|
||||
|
||||
These commits moved canvas utilities to modular components but introduced the regression by not maintaining hover position tracking.
|
||||
|
||||
## Future Improvements
|
||||
|
||||
1. **Canvas Mode System**: Complete the interaction handler modes (tile paint, select, etc.)
|
||||
2. **Persistent Context Menus**: Implement mode switching through context menu popups
|
||||
3. **Debugging Visualization**: Add canvas coordinate overlay for debugging
|
||||
4. **E2E Tests**: Create end-to-end tests for overworld map highlighting workflow
|
||||
|
||||
## Related Documentation
|
||||
- `docs/G1-canvas-guide.md` - Canvas system architecture
|
||||
- `docs/E5-debugging-guide.md` - Debugging techniques
|
||||
- `docs/debugging-startup-flags.md` - CLI flags for editor testing
|
||||
Reference in New Issue
Block a user