Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ImguiMapEditor/Domain/ItemType.h
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ class ItemType {
bool is_wall = false;
bool is_locked = false; // Door key lock status (for highlight_locked_doors)

// Lying object (multi-tile corpses) — drawn before creatures
bool is_lying_object = false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Unused property: The is_lying_object property is merged into ItemType from DAT, but it is not utilized in the ItemRenderer or any other part of the rendering pipeline.

The PR summary mentions: "Multi-tile “lying” items now render beneath creatures for correct depth.", but without logic checking this flag in ItemRenderer::queueAll (e.g., to move these items to Pass 1), they will continue to render in Pass 2 (Common items), which is already beneath creatures.

If the intention was to specifically handle multi-tile corpses that might overlap creatures on adjacent tiles due to isometric rendering order, further logic in ItemRenderer would likely be needed.


// Sprite IDs for rendering
std::vector<uint32_t> sprite_ids;

Expand Down
85 changes: 78 additions & 7 deletions ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,42 +25,113 @@ int ItemAnimation::getPhaseFromFrames(int frames, int64_t global_ms,
return tick % frames;
}

int ItemAnimation::getPingPongPhase(int raw_phase, int frames) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Improved Animation Support

The addition of ping-pong animation support and start_frame (random and fixed) brings the editor closer to OTClient's rendering parity. The mathematical implementation of getPingPongPhase is correct.

if (frames <= 1) return 0;
int cycle = frames * 2 - 2;
int mod = raw_phase % cycle;
return mod >= frames ? cycle - mod : mod;
}

int ItemAnimation::getPhase(const Domain::ItemType &item, int64_t global_ms,
int tile_x, int tile_y, int tile_z) {
int frames = std::max<int>(item.frames, 1);
if (frames <= 1)
return 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Restricted async animation check.

bool is_async = !item.frame_durations.empty() && item.animation_mode == 0;

This check requires duration data for an item to be considered asynchronous. However, some items without explicit duration data (defaulting to 500ms) might still be intended to run asynchronously if animation_mode == 0. Consider checking only the animation_mode.

bool per_instance = !item.frame_durations.empty() && item.animation_mode == 0;
int tile_offset = per_instance ? (tile_x * 17 + tile_y * 31 + tile_z * 7) : 0;
bool is_async = !item.frame_durations.empty() && item.animation_mode == 0;
int tile_offset = is_async ? (tile_x * 17 + tile_y * 31 + tile_z * 7) : 0;

int start_phase = 0;
if (item.start_frame == 255 && is_async) {
start_phase = (tile_x * 17 + tile_y * 31 + tile_z * 7) % frames;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Animation Synchronization: The is_async flag is only set if frame_durations is not empty. This means that items using a static 500ms frame duration (common for many Tibia items) will always have tile_offset = 0, causing their animations to be perfectly synchronized across all tiles even if animation_mode == 0 (Async) was requested.

Consider allowing tile_offset for items without explicit durations if animation_mode == 0.

} else if (item.start_frame > 0) {
start_phase = item.start_frame % frames;
}

if (item.loop_count == -1) {
// Ping-pong: apply start_phase to raw phase BEFORE reflection
if (item.frame_durations.empty()) {
int tick = static_cast<int>(global_ms / 500);
return getPingPongPhase(tick + tile_offset + start_phase, frames);
}

// Build explicit ping-pong timeline from per-phase durations
const int num_phases = static_cast<int>(item.frame_durations.size());
if (num_phases == 0) return start_phase;

// Per-phase durations (stack allocation — clamp to prevent overflow)
constexpr int MAX_PHASES = 32;
const int effective_phases = std::min<int>(num_phases, MAX_PHASES);
int phase_durs[MAX_PHASES];
int cycle_length = 0;
for (int i = 0; i < effective_phases; ++i) {
phase_durs[i] = getPhaseDuration(item, i);
cycle_length += phase_durs[i];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Potential Buffer Overflow: If num_phases (from item.frame_durations.size()) exceeds MAX_PHASES, the second loop will use i values starting from num_phases - 2, which could be much larger than MAX_PHASES. These values are used as indices in phase_durs and added to timeline, potentially overflowing both or causing out-of-bounds access.

Additionally, the second loop does not check if timeline_len exceeds MAX_TIMELINE.

Suggested change
cycle_length += phase_durs[i];
const int effective_phases = std::min<int>(num_phases, MAX_PHASES);
for (int i = 0; i < effective_phases; ++i)
timeline[timeline_len++] = i;
for (int i = effective_phases - 2; i >= 1; --i)
timeline[timeline_len++] = i;

}

// Ping-pong timeline: [0, 1, ..., N-1, N-2, ..., 1] (omit duplicate endpoints)
constexpr int MAX_TIMELINE = MAX_PHASES * 2 - 2;
int timeline[MAX_TIMELINE];
int timeline_len = 0;
for (int i = 0; i < effective_phases; ++i)
timeline[timeline_len++] = i;
for (int i = effective_phases - 2; i >= 1; --i)
timeline[timeline_len++] = i;

// Backward portion cycle length
int backward_length = 0;
for (int i = effective_phases - 2; i >= 1; --i)
backward_length += phase_durs[i];
cycle_length += backward_length;

if (cycle_length <= 0) return start_phase;

// Start offset: sum durations of first start_phase phases
int start_offset = 0;
for (int i = 0; i < start_phase && i < effective_phases; ++i)
start_offset += phase_durs[i];

int elapsed = static_cast<int>(
(global_ms + static_cast<int64_t>(tile_offset) * 500 + start_offset) % cycle_length);

// Walk the timeline to find the active phase
for (int i = 0; i < timeline_len; ++i) {
int dur = phase_durs[timeline[i]];
if (elapsed < dur)
return timeline[i] % frames;
elapsed -= dur;
}
return 0;
}

// Forward loop
if (item.frame_durations.empty()) {
int tick = static_cast<int>(global_ms / 500);
return (tick + tile_offset) % frames;
return (tick + tile_offset + start_phase) % frames;
}

int total = static_cast<int>(item.total_duration);
if (total <= 0)
return 0;
return start_phase;

int64_t elapsed_ms = global_ms + static_cast<int64_t>(tile_offset) * 500;
int elapsed = static_cast<int>(elapsed_ms % total);

for (int phase = 0; phase < static_cast<int>(item.frame_durations.size()); ++phase) {
int dur = getPhaseDuration(item, phase);
if (elapsed < dur)
return phase % frames;
return (phase + start_phase) % frames;
elapsed -= dur;
}

return 0;
return start_phase;
}

int ItemAnimation::getPhaseDuration(const Domain::ItemType &item, int phase) {
if (phase < 0 || phase >= static_cast<int>(item.frame_durations.size()))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Type Safety: The return value of getPhaseDuration is cast to int, but the calculation (d.first + d.second) / 2 could potentially overflow if d.first and d.second are large. While unlikely for frame durations, using int64_t for the sum or static casting the operands to int64_t before addition would be safer.

return 500;
const auto &d = item.frame_durations[phase];
return (d.first + d.second) / 2;
return static_cast<int>((d.first + d.second) / 2);
}

} // namespace Rendering
Expand Down
1 change: 1 addition & 0 deletions ImguiMapEditor/Rendering/Animation/ItemAnimation.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ struct ItemAnimation {

private:
static int getPhaseDuration(const Domain::ItemType &item, int phase);
static int getPingPongPhase(int raw_phase, int frames);
};

} // namespace Rendering
Expand Down
5 changes: 2 additions & 3 deletions ImguiMapEditor/Rendering/Map/TileRenderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -189,15 +189,14 @@ void TileRenderer::queueTile(const Domain::Tile &tile, int tile_x, int tile_y,
}
}

// Queue all items - delegate to ItemRenderer
// Pass cached items with pre-resolved types
// Queue all items
item_renderer_.queueAll(render_cache_, screen_x, screen_y, size, tile_x,
tile_y, tile_z, anim_ticks, ground_color, item_alpha,
isItemSelected, view_settings_, missing_sprites,
&accumulated_elevation, tile_has_hook_south,
tile_has_hook_east, check_tooltips, &tile_needs_tooltip);

// CREATURE RENDERING - Per-tile immediate (OTClient parity)
// Creature rendering (per-tile for correct isometric depth)
// With diagonal tile iteration, creatures must be rendered PER-TILE to
// maintain correct isometric depth. NW tiles (drawn first) should have
// creatures that can be overlapped by SE tile items (drawn later). Deferring
Expand Down
Loading
Loading