You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Implemented multi-tile corpse rendering before creatures and aligned mount drawing logic with OTClient parity. Added support for ping-pong animations and per-instance random phase offsets.
- `ImguiMapEditor/Domain/ItemType.h` — added `is_lying_object` flag
- `ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp` — added ping-pong phase logic, handled `start_frame` and `is_async` offsets
- `ImguiMapEditor/Rendering/Animation/ItemAnimation.h` — declared `getPingPongPhase`
- `ImguiMapEditor/Rendering/Map/TileRenderer.cpp` — added pre-creature drawing for multi-tile lying objects
- `ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp` — extracted `drawMount` and `emitPlaceholder` methods, updated drawing order
- `ImguiMapEditor/Rendering/Tile/CreatureRenderer.h` — added helper methods for mount and placeholder rendering
- `ImguiMapEditor/Rendering/Tile/ItemRenderer.h` — updated common item rendering to reverse order and exclude multi-tile lying corpses
- `ImguiMapEditor/Services/ClientDataService.cpp` — mapped `is_lying_object` from DAT data
Standardized item and brush preview rendering into a unified `RenderPreviewCard` utility to enforce consistent styling, selection feedback, and rounded aesthetics. Updated compositor logic to correctly support multi-layer items by iterating through layers before grid tiles.
- `ImguiMapEditor/Services/ItemCompositor.cpp` — updated composite logic to iterate layers first, matching map renderer order; removed hardcoded background color
- `ImguiMapEditor/UI/Utils/PreviewUtils.cpp` — implemented `RenderPreviewCard` for uniform rounded UI element rendering with state support
- `ImguiMapEditor/UI/Utils/PreviewUtils.hpp` — added `RenderPreviewCard` declaration
- `ImguiMapEditor/UI/Widgets/Properties/PropertyPanelRenderer.cpp` — migrated slot rendering to `RenderPreviewCard`
- `ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` — migrated brush grid to `RenderPreviewCard`, cleaned up redundant drawing logic
- `ImguiMapEditor/UI/Widgets/TilesetGridWidget.h` — updated `getBrushTexture` return type to `Rendering::Texture*`
- `ImguiMapEditor/UI/Widgets/TilesetWidget.cpp` — migrated item grid to `RenderPreviewCard`
- `ImguiMapEditor/UI/Windows/BrowseTile/ItemsListRenderer.cpp` — migrated item list row rendering to `RenderPreviewCard`
Refactors and extends rendering and previewing of creatures and items to match OTClient/RME behavior, adds proper mount/addon handling and lying corpse depth ordering, unifies UI preview cards, and enhances item animation phase logic including ping-pong and start-frame support.
File-Level Changes
Change
Details
Files
Improve creature rendering to support mounts, addons, shared placeholder emission, and correct sprite selection/layout.
Early-return placeholder emission is centralized into emitPlaceholder helper
Direction mapping and frame selection for outfits are simplified while preserving frame-group logic
Outfit drawing now iterates addons via pattern_y and supports multi-layer colorization with template sprites
Mount drawing is extracted into drawMount, handling its own frame groups, animation, offsets, and colorization
Screen position computation for outfit and mount accounts for offsets scaled by tile size, and missing sprites are collected consistently
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
In UI::Utils::RenderPreviewCard you mix ImGui::IsMouseHoveringRect(min, max) (manual rect) with callers using ImGui::IsItemHovered() on the trailing dummy; consider basing both hover/tooltip logic on the same ImGui item to avoid subtle mismatches when hovering padded vs non-padded regions.
In TilesetGridWidget::renderBrushGrid you now have two different card rendering paths (one using RenderPreviewCard, one reimplementing a rounded card and selection border); it would be cleaner and less error-prone to consolidate these to a single shared helper so card visuals and interaction remain consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `UI::Utils::RenderPreviewCard` you mix `ImGui::IsMouseHoveringRect(min, max)` (manual rect) with callers using `ImGui::IsItemHovered()` on the trailing dummy; consider basing both hover/tooltip logic on the same ImGui item to avoid subtle mismatches when hovering padded vs non-padded regions.
- In `TilesetGridWidget::renderBrushGrid` you now have two different card rendering paths (one using `RenderPreviewCard`, one reimplementing a rounded card and selection border); it would be cleaner and less error-prone to consolidate these to a single shared helper so card visuals and interaction remain consistent.
## Individual Comments### Comment 1
<locationpath="ImguiMapEditor/UI/Utils/PreviewUtils.cpp"line_range="48-57" />
<code_context>
+bool RenderPreviewCard(Rendering::Texture* texture, float size,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Hover detection is decoupled from the ImGui item, which can be fragile with clipping/scrolling.
`RenderPreviewCard` computes hover using `ImGui::IsMouseHoveringRect(min, max)` while using a separate `ImGui::Dummy` as the clickable item. This can diverge from ImGui’s own hover logic in more complex layouts (clipping, overlapping items). To keep hover and click semantics consistent, derive the rect from the `Dummy` item (`ImGui::GetItemRectMin/Max()` + `ImGui::IsItemHovered()`), or call `Dummy` first and then base both hover and click on that item’s state.
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR is currently not up to standards according to Codacy, and the review identifies several high-severity issues that prevent merging. While intended as a 'bugix', the changes introduce a significant rework of the item rendering compositor and new features like creature mounts and addons without a supporting PR description.
Key issues include a depth-sorting regression in ItemRenderer.h that will cause items to render in the wrong vertical order, and a logic error in ItemAnimation.cpp where frame offsets are applied at the wrong stage of the ping-pong calculation, leading to visual jumps. There is also a risk of double-rendering multi-tile objects in TileRenderer.cpp. Until the core rendering passes and animation math are corrected and validated with tests, this PR should not be merged.
About this PR
The PR description is empty despite containing significant architectural changes to the rendering pipeline and new features like mounts and addons. This makes validation and future maintenance difficult.
The title 'bugix' is misleading. The PR introduces substantial new features (mounts, addons, ping-pong animations) alongside bug fixes. Please update the title and provide a description of the new capabilities.
Missing: Verify creature mount rendering correctly applies colorization from lookMountHead/Body/Legs/Feet
Missing: Verify creature addon visibility correctly respects the lookAddons bitmask
Missing: Verify multi-tile items marked as 'is_lying_object' are skipped in the standard item pass and rendered in the pre-creature pass
Missing: Verify async animation logic correctly calculates tile-based offsets to prevent all instances from animating in unison
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing: Verify ItemAnimation::getPingPongPhase returns correct sequence (e.g., 0, 1, 2, 1 for 3 frames)
2. Missing: Verify creature mount rendering correctly applies colorization from lookMountHead/Body/Legs/Feet
3. Missing: Verify creature addon visibility correctly respects the lookAddons bitmask
4. Missing: Verify multi-tile items marked as 'is_lying_object' are skipped in the standard item pass and rendered in the pre-creature pass
5. Missing: Verify async animation logic correctly calculates tile-based offsets to prevent all instances from animating in unison
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
The condition 'cycle <= 0' is redundant. Since 'frames' is guaranteed to be at least 2 by the previous check, 'cycle' (2 * frames - 2) will always be greater than zero.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Applying start_phase to the final index in a ping-pong animation breaks visual continuity. The offset should be applied to the timing or raw phase before the reflection logic, similar to the fixed-duration implementation at line 58. Additionally, the method has exceeded complexity thresholds and should be refactored to extract ping-pong and forward loop logic into helper methods.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Changing Pass 2 to reverse iteration creates a depth-sorting inconsistency. If the item stack is stored Bottom-to-Top (as in RME/OTClient), rbegin will draw the topmost items first, causing them to be covered by lower items. Furthermore, ensure that items with a null 'type' (no metadata) still render correctly in the proper pass now that the explicit fallback handling has been removed.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Manually re-queuing lying objects after queueAll breaks the item stack's depth order on the same tile and can lead to double-rendering if the item is also marked is_on_bottom. These items should be handled as a distinct pass within the ItemRenderer to preserve their relative elevation.
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces significant improvements to item and creature rendering, standardizes the preview UI across several widgets, and adds support for advanced animation modes (ping-pong, start frames).
The most critical finding is a regression in item rendering order within ItemRenderer::queueAll. Changing the common items pass to reverse iteration while the input items vector is sorted bottom-to-top will cause rendering artifacts (specifically, the ground being drawn on top of other items).
🔍 General Feedback
Rendering Parity: Great progress on matching OTClient/RME rendering logic, especially regarding multi-tile corpses (lying objects) and mount rendering.
UI Consistency: Standardizing item/creature previews into a reusable RenderPreviewCard is a very good architectural move.
Correctness: The animation logic for ping-pong and start frames looks solid.
🔴 Critical Issues
Rendering Order: Pass 2 in ItemRenderer::queueAll must be forward-iterating to maintain the painter's algorithm for bottom-to-top sorted item stacks.
🟡 Minor Suggestions
Consistency: Use SpriteUtils::getSpriteIndex in ItemCompositor for more robust indexing.
Cleanup: Some unused legacy code in ItemCompositor was correctly removed, but ensure that the removal of the gray background shade in previews is an intended design choice.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Critical Regression in Rendering Order
Changing Pass 2 to reverse iteration (rbegin to rend) will cause items to be drawn in top-to-bottom order. Since render_cache_ (passed from TileRenderer) contains items in bottom-to-top order (Ground at index 0, followed by stack items), this reverse loop will draw the Ground last, causing it to appear on top of all other items.
In Tibia, items should be drawn from bottom to top (painter's algorithm). If "OTClient parity" is desired, please ensure the input items span is already sorted top-to-bottom, otherwise, this should remain a forward loop.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Use SpriteUtils::getSpriteIndex for Consistency
While this simplified indexing works for frame 0 with no patterns, it's safer and more maintainable to use Utils::SpriteUtils::getSpriteIndex(type, cx, cy, layer, 0, 0, 0, 0). This ensures that if the internal sprite storage order ever changes (or if we want to composite specific frames/patterns), the compositor remains correct.
Also, type->sprite_ids contains flattened IDs, so ensuring the loop order matches getSpriteIndex is critical. Your current loop order (layer -> height -> width) correctly matches getSpriteIndex.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Standardized Preview UI
Excellent addition of RenderPreviewCard. Standardizing the preview rendering across the Properties panel, Tileset grid, and Browse Tile window significantly improves UI consistency. Using ImGuiCol_Header for selection and adding the gold border for selected items is a nice touch.
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.
Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces significant enhancements to the map editor's rendering and UI systems. It improves the animation engine with new ping-pong capabilities, refines the rendering order for multi-tile corpses to ensure correct isometric depth, and adds support for rendering creature mounts. Furthermore, it standardizes the visual representation of items and creatures across the UI by introducing a unified preview card component, ensuring a consistent look and feel throughout the application.
Highlights
Animation System: Added support for ping-pong animation loops and improved start frame logic for item animations.
Rendering Pipeline: Implemented specific handling for multi-tile lying objects (corpses) to ensure they are rendered before creatures, preventing depth sorting issues.
Creature Rendering: Added support for rendering creature mounts and refactored the rendering logic for better performance and correctness.
UI Standardization: Introduced a unified 'RenderPreviewCard' component to standardize item and creature previews across the application's UI widgets.
Item Compositor: Refactored the item compositor to support multi-layer items and improved the handling of multi-tile sprite composition.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
Feature
Command
Description
Code Review
/gemini review
Performs a code review for the current pull request in its current state.
Pull Request Summary
/gemini summary
Provides a summary of the current pull request in its current state.
Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help
/gemini help
Displays a list of available commands.
Customization
To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several rendering and UI improvements, including support for multi-tile lying corpses, refactored creature and mount rendering, multi-layer item compositing, and a unified RenderPreviewCard utility for UI previews. However, the code review identifies several critical issues: reverse iteration of common items in ItemRenderer.h breaks visual stacking and elevation; rendering multi-tile corpses after queueAll in TileRenderer.cpp causes elevation and overlap bugs; a boundary condition in ItemAnimation.cpp causes visual flickering in ping-pong animations; and using ImGui::IsMouseHoveringRect in PreviewUtils.cpp leads to robust input handling issues when overlapping windows are present.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The reason will be displayed to describe this comment to others. Learn more.
Iterating through the common items in reverse order (rbegin to rend) completely breaks the visual stacking order and the accumulated_elevation calculation. In this codebase, items are stored in bottom-to-top order (where the end of the vector is the top of the stack). Rendering them in reverse order means the topmost item is drawn first at 0 elevation, and the bottom-most item is drawn last at the highest elevation, causing items to render upside down and float in the air.
Please revert this to forward iteration as in the original implementation.
for (const auto &entry : items) {
if (entry.type&&entry.type->is_on_bottom) continue;
if (entry.type&&entry.type->is_on_top) continue;
if (entry.type&&entry.type->is_lying_object&&
(entry.type->width>1||entry.type->height>1))
continue;
renderItem(entry);
}
The reason will be displayed to describe this comment to others. Learn more.
In the ping-pong animation path with custom durations, when elapsed is exactly equal to total, the mirroring logic sets elapsed to total. Since total is the sum of all frame durations, the subsequent loop fails to find any phase where elapsed < dur (as elapsed will reduce to exactly dur in the last iteration, and dur < dur is false). This causes the function to fall through and return start_phase (the first frame) for a single tick/frame, causing a noticeable visual flicker at the peak of the animation.
Using elapsed = cycle_total - 1 - elapsed maps the reverse range [total, cycle_total - 1] perfectly to [0, total - 1], preventing this boundary bug.
The reason will be displayed to describe this comment to others. Learn more.
Excluding multi-tile lying corpses from queueAll and rendering them afterwards in TileRenderer::queueTile introduces a severe rendering and elevation bug:
Any items stacked on top of the corpse (which appear after the corpse in the items vector) will be rendered during queueAll first, without the corpse's elevation contribution, causing them to render too low.
The corpse is rendered after queueAll, meaning it will visually overlap and render on top of items that are supposed to be on top of it.
The corpse is rendered with the fully accumulated elevation of the entire stack (including items above it), causing it to float.
Furthermore, this does not solve the isometric multi-tile overlap issue across adjacent tiles, as those tiles are processed in diagonal order. It is highly recommended to keep corpses in the normal queueAll pass.
The reason will be displayed to describe this comment to others. Learn more.
Using ImGui::IsMouseHoveringRect to detect hover states can cause visual bugs because it returns true even if the mouse is hovering over an overlapping window, popup, or modal menu that is currently on top of the preview card.
It is highly recommended to call ImGui::Dummy first, and then use ImGui::IsItemHovered() and ImGui::IsItemClicked() to ensure correct and robust input/hover state handling.
This PR adds an ItemType flag is_lying_object (propagated from DAT), refactors ItemAnimation to support ping-pong loops, start-phase and async offsets, updates SpriteUtils and ItemCompositor for layer-aware sprite indexing and compositing, changes ItemRenderer/TileRenderer and CreatureRenderer to render mounts, use placeholders for missing assets, and introduces RenderPreviewCard plus widget updates to use typed Rendering::Texture* previews.
Possibly related PRs
Open-Tibia-Tools/tibia-imgui-map-editor#27: Modifies animation phase computation in ItemAnimation and updates renderers to use the new phase API, directly related to the animation refactoring in this PR.
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 17.50% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check
❓ Inconclusive
The title is partially related to the changeset but lacks clarity. It mentions 'item compositor' (one aspect of the changes) but doesn't capture the main themes: item rendering enhancements, creature mount support, animation improvements, and UI refactoring across multiple systems.
Consider a more descriptive title that reflects the scope, such as: 'feat: enhance item/creature rendering with mounts, animation fixes, and compositor improvements' or 'feat: add mount rendering and fix item animation/composition logic'.
✅ Passed checks (3 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
Comment @coderabbitai help to get the list of available commands and usage tips.
start_phase applied incorrectly in duration-based ping-pong path.
In the duration-based ping-pong branch, start_phase is applied as a rotation of the final phase index ((phase + start_phase) % frames). This breaks the symmetric nature of ping-pong animations.
Compare with line 58 where start_phase is correctly added to the raw phase before ping-pong reflection. The same approach should be used here: offset should be incorporated into the elapsed time or raw phase calculation before selecting which phase it falls into.
Example issue: with frames=3 and start_phase=1, ping-pong goes 0→1→2→1→0. After adding 1 and modulo, it becomes 1→2→0→2→1, which is no longer symmetric.
🐛 Suggested fix: apply start_phase as timing offset
- int elapsed = static_cast<int>((global_ms + static_cast<int64_t>(tile_offset) * 500) % cycle_total);+ // Apply start_phase as a timing offset (fraction of total duration)+ int phase_offset_ms = (static_cast<int64_t>(start_phase) * total) / frames;+ int elapsed = static_cast<int>((global_ms + static_cast<int64_t>(tile_offset) * 500 + phase_offset_ms) % cycle_total);
if (elapsed >= total)
elapsed = cycle_total - elapsed;
for (int phase = 0; phase < static_cast<int>(item.frame_durations.size()); ++phase) {
int dur = getPhaseDuration(item, phase);
if (elapsed < dur)
- return (phase + start_phase) % frames;+ return phase;
elapsed -= dur;
}
- return start_phase;+ return 0;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp` around lines 66 - 72,
The ping-pong duration branch incorrectly applies start_phase as a modular shift
to the final phase index ((phase + start_phase) % frames); instead incorporate
start_phase as a timing/phase offset before deciding which phase the elapsed
time falls into (mirroring the approach used at line 58). Adjust the computation
so you convert start_phase into an initial time/phase offset (or add it to the
raw phase index prior to ping-pong reflection) and subtract that from elapsed
(or add to the raw phase) before iterating getPhaseDuration(item, phase) and
returning phase; use the same treatment of start_phase used elsewhere to
preserve symmetric ping-pong order for item.frame_durations, getPhaseDuration,
frames and start_phase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImguiMapEditor/Rendering/Map/TileRenderer.cpp`:
- Around line 199-208: The corpse-correction loop may double-render items that
are both multi-tile lying corpses and marked is_on_bottom; update the loop that
iterates render_cache_ (the block that checks type->is_lying_object and
type->width/height) to skip any item whose type->is_on_bottom is true so they
aren't re-queued here; locate the corpse correction pass around the call to
item_renderer_.queueWithColor and add a guard (check type->is_on_bottom) before
calling queueWithColor to avoid double-rendering items also handled in
queueAll's on-bottom pass.
In `@ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp`:
- Around line 47-48: The fallback for computing dir uses a hardcoded "2 %
pattern_x" which incorrectly wraps directions; replace that logic to wrap using
the actual direction value like drawMount does: compute pattern_x as now (const
int pattern_x = std::max<int>(1, outfit_data->pattern_x)) and set dir by taking
direction % pattern_x (or direction < pattern_x ? direction : direction %
pattern_x) so the wrap uses the direction variable and not the literal 2; update
the code around variables pattern_x, dir and reference outfit_data to match
drawMount's wrapping behavior that uses mount_data->pattern_x.
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp`:
- Around line 289-290: The code hardcodes isSelected=false before calling
Utils::RenderPreviewCard, so clicked tiles never render as selected; replace the
hardcoded flag by computing whether the current tile/texture (tex) equals the
widget's current selection (e.g., compare to the widget's selected texture/index
member) and pass that boolean to RenderPreviewCard, and ensure the click
handling (the clicked variable) updates that selected member so subsequent calls
to RenderPreviewCard will render selected styling; look for references to
RenderPreviewCard, getIconSize(), the local isSelected/clicked variables and the
TilesetGridWidget's selected texture/index state to implement this change.
---
Duplicate comments:
In `@ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp`:
- Around line 66-72: The ping-pong duration branch incorrectly applies
start_phase as a modular shift to the final phase index ((phase + start_phase) %
frames); instead incorporate start_phase as a timing/phase offset before
deciding which phase the elapsed time falls into (mirroring the approach used at
line 58). Adjust the computation so you convert start_phase into an initial
time/phase offset (or add it to the raw phase index prior to ping-pong
reflection) and subtract that from elapsed (or add to the raw phase) before
iterating getPhaseDuration(item, phase) and returning phase; use the same
treatment of start_phase used elsewhere to preserve symmetric ping-pong order
for item.frame_durations, getPhaseDuration, frames and start_phase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31cf9e9b-7470-4bba-964a-f9aae4a32b08
📥 Commits
Reviewing files that changed from the base of the PR and between ae0ebb9 and 365f56d.
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces significant improvements to item rendering and UI consistency. Key highlights include:
Mount Rendering Support: Well-implemented support for mounts with proper coloring and animation selection.
Improved Item Compositing: Multi-layer and multi-tile items are now composited correctly for UI previews.
Rendering Order Fix: Switching from REVERSE to FORWARD iteration in queueAll fixes long-standing item stacking issues.
UI Consistency: The new RenderPreviewCard utility unifies the look and feel of item/creature previews across the application.
🔍 General Feedback
While the core changes are solid, I've noted a few areas for refinement:
Animation Logic: There's a slight discrepancy between index-based and duration-based ping-pong animations. The current duration-based approach doubles the time spent on "peak" frames.
Lying Objects: The is_lying_object flag was added but isn't explicitly used to override is_on_top behavior in the rendering passes.
Safety: Added a suggestion for a null/empty check in ItemCompositor to prevent potential crashes with malformed item data.
Overall, a very positive set of changes that enhances the editor's visual fidelity.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Consistency Issue: There is a slight inconsistency between the index-based ping-pong logic (getPingPongPhase uses 2*frames - 2) and the duration-based logic here (uses 2*total).
The duration-based version effectively doubles the duration of the "peak" frames (first and last), resulting in a sequence like (0, 1, 2, 2, 1, 0), whereas the index-based version results in (0, 1, 2, 1).
While often acceptable in an editor, for strict OTClient parity, you might want to use a cycle of 2*total - duration(first) - duration(last).
The reason will be displayed to describe this comment to others. Learn more.
🟡 Redundant Calculation: raw_elapsed already includes tile_offset * 500 ms, which desynchronizes animations per tile.
int raw_elapsed = static_cast<int>((global_ms + static_cast<int64_t>(tile_offset) * 500) % cycle_total);
Adding start_phase * (total / frames) to it might result in "double-shifting" the animation start point if start_frame == 255. If this is intended for parity with a specific client behavior, please disregard.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Implementation Gap: The is_lying_object flag was added to ItemType, but it doesn't appear to be used in the rendering passes yet.
To fulfill the PR's claim of "Corrected rendering order for multi-tile lying objects (now render before creatures)", you might need to handle cases where a corpse is marked as is_on_top in the DAT (which would otherwise defer it to the post-creature pass).
Consider explicitly including lying objects in queueAll even if they have the is_on_top flag:
if (entry.type && entry.type->is_on_top && !entry.type->is_lying_object) continue;
The reason will be displayed to describe this comment to others. Learn more.
🔴 Safety Issue: If type->sprite_ids is empty, this will crash. While isValidForRendering() usually catches this, it's safer to check explicitly here before accessing [0].
The reason will be displayed to describe this comment to others. Learn more.
🟢 Refinement: Using std::max<int>(1, ...) for loop bounds and denominators is good practice here to prevent division by zero or infinite loops if data is malformed.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Excellent Addition: This unified card renderer significantly improves UI consistency across the editor. The use of standard ImGui theme colors instead of hardcoded values is also a great improvement.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp`:
- Around line 57-72: The current ping-pong uses a uniform 2*item.total_duration
cycle and shifts start_phase by total/frames which breaks uneven
frame_durations; instead build an explicit timeline using per-phase durations:
create a vector of phase durations from item.frame_durations via
getPhaseDuration(item, phase), append the reverse sequence (omit duplicate
end-phase) to form the ping-pong cycle, compute the cycle length as the sum of
those durations, compute the start offset by summing durations of the first
start_phase phases, add (global_ms + tile_offset*500) mod cycle_length, then
walk the per-phase durations to find and return the active phase (mod frames
where appropriate). Use symbols item.frame_durations, getPhaseDuration,
item.total_duration, start_phase, global_ms, tile_offset and frames to locate
and replace the current logic.
In `@ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp`:
- Around line 153-159: selectMountFrame() may return an index into
mount_data->idle_frames but drawMount() only swaps mount_sprites to
mount_data->walk_sprite_ids; update drawMount() so it picks the correct sprite
array based on the frame group (idle vs walk) before indexing: inspect
selectMountFrame's return or mount_data flags and set mount_sprites to
mount_data->idle_sprite_ids when idle_frames are used (and to
mount_data->walk_sprite_ids when walking), falling back to
mount_data->sprite_ids only if specific idle/walk sprite vectors are empty;
apply the same fix for the other occurrence around the 208-217 block so idle
mounted creatures use idle_sprite_ids instead of sprite_ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae1e7c86-8717-403e-aa94-b90ed3bf907b
📥 Commits
Reviewing files that changed from the base of the PR and between 365f56d and 88e1929.
Improved ping-pong animation calculation, refined mount sprite selection for idle/walk states, added safety checks for item rendering, and highlighted the selected brush in the tileset grid.
- `ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp` — refactored ping-pong animation to use explicit timeline based on per-phase durations
- `ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp` — updated mount sprite selection to use idle groups when available
- `ImguiMapEditor/Services/ItemCompositor.cpp` — added safety check for empty `sprite_ids` in single-tile item rendering
- `ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` — added highlighting for selected brush in tileset grid
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces significant improvements to item and creature rendering, including mount support, ping-pong animations, and corrected depth sorting for common items.
🔍 General Feedback
Correctness: Found a logic error in creature direction mapping for outfits with fewer than 4 directions.
Performance: The refactored ItemAnimation::getPhase method performs multiple std::vector allocations in a hot path, which could degrade frame rates on large maps.
Completeness: The is_lying_object property is introduced but not explicitly used in the rendering pipeline, although the general stacking order fix for Pass 2 partially addresses the described issue.
UI: The new RenderPreviewCard utility is a great addition for UI consistency.
Please address the performance and direction mapping issues before merging.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Incorrect direction mapping for creatures with < 4 directions.
The current logic:
int dir = direction < pattern_x ? direction : 2 % pattern_x;
will map South (2) and West (3) to index 0 (North) if pattern_x is 2. This is likely incorrect for creatures that only have South and East directions. Using a modulo or a more specific mapping table would be safer.
The reason will be displayed to describe this comment to others. Learn more.
🟠 Performance: Memory allocations in hot path.
std::vector<int> timeline and phase_durs are allocated on every call to getPhase. Since this method is called for every animated item on screen every frame, this will lead to significant allocator churn.
Consider using a fixed-size stack array or caching these timelines, as animation data for a specific ItemType is typically static.
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.
The reason will be displayed to describe this comment to others. Learn more.
🟠 Unused property: is_lying_object.
The property is_lying_object is correctly parsed from the data files but is never used in the rendering passes. If these items should be rendered with higher priority (e.g., in Pass 1 with walls/ground decorations) to ensure they are always beneath creatures even if they are high in the tile stack, they should be checked here.
The fallback 2 % pattern_x uses a hardcoded literal 2 instead of wrapping the actual direction. Compare with drawMount at line 151 which correctly uses dir % std::max<int>(1, mount_data->pattern_x).
For example, if pattern_x = 2 and direction = 3, the result is 2 % 2 = 0, not 3 % 2 = 1.
Proposed fix
const int pattern_x = std::max<int>(1, outfit_data->pattern_x);
- int dir = direction < pattern_x ? direction : 2 % pattern_x;+ int dir = direction % pattern_x;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp` around lines 47 - 48, The
fallback for computing dir is wrong: replace the hardcoded `2` with the actual
`direction` so the wrap uses `direction % pattern_x`; specifically update the
computation that uses `pattern_x` (from `outfit_data->pattern_x`) and
`direction` to mirror the correct logic used in
`drawMount`/`mount_data->pattern_x` (i.e., compute `pattern_x = std::max<int>(1,
outfit_data->pattern_x)` then set `dir` to `direction` if less than `pattern_x`,
otherwise use `direction % pattern_x`).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp`:
- Around line 332-334: The cross-selected call to renderBrushCard currently
omits the isPulsing and pulseElapsed arguments (relying on defaults) so the
preview never pulses; update the call that computes isCrossSelected (where
brush->getName() == selectedBrushName_) to pass the same pulse state used
elsewhere: compute or reuse the local isPulsing and pulseElapsed variables
(matching other renderBrushCard call sites) and call renderBrushCard(cursorPos,
tileSize, tex, isCrossSelected, ImGui::IsItemHovered(), isPulsing, pulseElapsed)
so the cross-tileset preview animates consistently.
---
Duplicate comments:
In `@ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp`:
- Around line 47-48: The fallback for computing dir is wrong: replace the
hardcoded `2` with the actual `direction` so the wrap uses `direction %
pattern_x`; specifically update the computation that uses `pattern_x` (from
`outfit_data->pattern_x`) and `direction` to mirror the correct logic used in
`drawMount`/`mount_data->pattern_x` (i.e., compute `pattern_x = std::max<int>(1,
outfit_data->pattern_x)` then set `dir` to `direction` if less than `pattern_x`,
otherwise use `direction % pattern_x`).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6fea9d0-af3d-45a0-ae55-81190b1d49cd
📥 Commits
Reviewing files that changed from the base of the PR and between 88e1929 and 8d17fb5.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Check the function signature in the header to verify if default parameters exist
rg -A5 'renderBrushCard' ImguiMapEditor/UI/Widgets/TilesetGridWidget.h
Cross-tileset path disables pulse animation by relying on renderBrushCard defaults
renderBrushCard declares bool isPulsing = false and float pulseElapsed = 0.0f, so the 5-argument call compiles—but it also means the cross-tileset preview never enables pulse animation, unlike other call sites that pass isPulsing/pulseElapsed. Pass the computed pulse values in the cross-selected path to keep behavior consistent.
🧰 Tools🪛 Cppcheck (2.20.0)
[style] 332-332: The function 'isReadable' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImguiMapEditor/UI/Widgets/TilesetGridWidget.cpp` around lines 332 - 334, The
cross-selected call to renderBrushCard currently omits the isPulsing and
pulseElapsed arguments (relying on defaults) so the preview never pulses; update
the call that computes isCrossSelected (where brush->getName() ==
selectedBrushName_) to pass the same pulse state used elsewhere: compute or
reuse the local isPulsing and pulseElapsed variables (matching other
renderBrushCard call sites) and call renderBrushCard(cursorPos, tileSize, tex,
isCrossSelected, ImGui::IsItemHovered(), isPulsing, pulseElapsed) so the
cross-tileset preview animates consistently.
Replaced dynamic heap-allocated std::vectors with fixed-size stack arrays to eliminate allocation overhead in animation phase calculation, and simplified creature direction mapping.
- `ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp` — migrated timeline and phase duration buffers to stack arrays with constant size limits, removed unnecessary vector headers
- `ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp` — simplified direction mapping logic to a direct modulo operation
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
Overall, this PR introduces significant improvements to the rendering pipeline, especially regarding animation timing (ping-pong support) and creature mounts. The introduction of interactive preview cards also enhances the UI consistency.
However, there are a few critical and high-priority issues that should be addressed:
🔴 Buffer Overflow in ItemAnimation.cpp: The ping-pong timeline construction for animations with custom durations can overflow the stack-allocated timeline buffer and access phase_durs out-of-bounds if num_phases exceeds MAX_PHASES (32).
🟠 Incorrect Mount Rendering: CreatureRenderer sets z_pattern = 1 for all mounted creatures without verifying if the outfit's DAT data actually supports multiple Z patterns. This will lead to incorrect sprite lookups for older client versions or simple outfits.
🟡 Unused is_lying_object: The is_lying_object property was added to ItemType and merged from DAT, but it is not yet utilized in ItemRenderer to control rendering order, which was one of the stated goals in the PR summary.
🟡 Code Duplication: Identical logic for getSpriteIndex is implemented for both ClientItem and ItemType in SpriteUtils.cpp.
🔍 General Feedback
Performance: The move to forward-order rendering in ItemRenderer (painter's algorithm) is a great fix for depth issues.
Maintainability: The consolidation of preview logic into RenderPreviewCard is a good architectural move.
Testing: Ensure that ping-pong animations and multi-tile mount offsets are verified with different client versions (especially 7.6 vs 10.x+).
The reason will be displayed to describe this comment to others. Learn more.
🟠 Missing Pattern Z Check: Setting z_pattern = 1 assumes the outfit data actually has at least 2 Z patterns (one for rider, one for rider+mount). If outfit_data->pattern_z is 1, passing z_pattern = 1 to getSpriteIndex will result in an incorrect (likely out-of-bounds) index.
Suggested change
// Draw mount before rider
int z_pattern = 0;
if (outfit.lookMount > 0 && outfit_data->pattern_z > 1) {
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.
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.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Code Duplication: The implementation of getSpriteIndex for ItemType is identical to the one for ClientItem. Consider using a template or a private helper function to avoid logic duplication.
Clamped animation phase counts to prevent potential overflows and added a pattern depth check when rendering mounts.
- `ImguiMapEditor/Rendering/Animation/ItemAnimation.cpp` — clamped loop iterations to `MAX_PHASES` to prevent potential out-of-bounds access
- `ImguiMapEditor/Rendering/Tile/CreatureRenderer.cpp` — added `outfit_data->pattern_z > 1` check when deciding to render a mount
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR significantly improves the rendering quality and correctness for items and creatures. Key highlights include:
Mount Rendering: Correct implementation of mount visuals with proper state handling (idle/walk).
Item Composition: Fixed UI icon rendering for multi-layer and multi-tile items using correct sprite indexing.
Animation Logic: Added support for Ping-pong animations and start-frame offsets.
UI Improvements: Introduced interactive preview cards for a more consistent look across the editor.
🔍 General Feedback
The architectural changes in the rendering pipeline are solid and improve maintainability. I've noted a few minor points regarding an unused property and animation synchronization edge cases, but overall this is a high-quality contribution.
One specific concern is the is_lying_object property which appears to be currently unused despite being mentioned in the PR summary as a fix for rendering depth. If this was intended to force corpses into a specific rendering pass, that logic seems to be missing.
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.
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.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Excellent Fix: Moving to Utils::SpriteUtils::getSpriteIndex and adding the layer loop correctly handles multi-layer items (like those with templates/colorable parts) in the UI icons. The previous implementation was only correct for simple single-layer items.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Robust Implementation: The mount rendering logic correctly handles the interaction between rider animation state (idle vs walk) and mount sprite selection. The addition of z_pattern logic for outfits with mounts is also correct for OTClient/Tibia parity.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Improved Readability: The refactored multi-pass rendering logic is much cleaner and easier to reason about while maintaining the correct OTClient painter's algorithm order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Area: DataChanges to data files (data/ — materials, clients.xml, menubar.xml).Area: RenderingChanges to the OpenGL rendering pipeline.Area: UIChanges to the wxWidgets GUI (palettes, dialogs, menus, toolbar).Type: BugInconsistencies or issues which will cause a problem for users.Type: EnhancementNew feature or request.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
github-actions
Bot
added
Type: Enhancement
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Improvements