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
…l grid
Moved ground-blocking logic from chunk-specific tracking to a viewport-level grid to support consistent light occlusion across multiple floors. Refactored LightGatherer and LightManager to use the new `ViewportGroundBlocking` struct instead of `GroundBrightness`.
- `ImguiMapEditor/Rendering/Light/LightGatherer.cpp` — removed `GroundBrightness` tracking, updated `gatherForChunkMultiFloor` and `gatherLightsFromNeighborChunk` to use `ViewportGroundBlocking`
- `ImguiMapEditor/Rendering/Light/LightGatherer.h` — replaced `GroundBrightness` with `ViewportGroundBlocking` struct
- `ImguiMapEditor/Rendering/Light/LightManager.cpp` — initialized `viewport_blocking_`, updated `render` and `computeChunkLight` signature and logic to use the new blocking grid
- `ImguiMapEditor/Rendering/Light/LightManager.h` — added `viewport_blocking_` member, updated `computeChunkLight` signature
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 a significant overhaul to the lighting system within the map editor to align with OTClient's rendering behavior. By implementing a more robust light gathering and rendering pipeline, the editor now correctly handles floor-based light occlusion and translucency. These changes include updates to domain models for creatures and items, improved UI controls for fine-tuning lighting parameters, and a refactored rendering engine that provides a more accurate representation of the game world's lighting.
Highlights
New Light Rendering Pipeline: Implemented OTClient-style light rendering with support for floor occlusion and translucency, significantly improving visual accuracy.
Domain Model Updates: Added light properties to CreatureType and ItemType, including a new 'lens_help' property to allow light propagation through specific items.
UI Enhancements: Added new lighting controls for intensity, color, and ambient light in both the main editor and preview windows.
Refactored Light Gathering: Replaced the old chunk-based light gathering with a viewport-based ViewportLightBuffer for better performance and more accurate light calculation.
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.
Refactors the lighting system to emulate OTClient/RME-style light rendering by replacing chunk-based caching with a viewport light buffer, integrating server global light and client minimum ambient sliders, adding floor visibility–aware light gathering (including translucent propagation and creature lights), and exposing new lighting controls in the UI and configuration paths.
File-Level Changes
Change
Details
Files
Replace chunk-based light cache with a single RME-style viewport light buffer and per-tile light-occlusion tracking.
Introduce ViewportLightBuffer to hold all visible lights and per-tile starting indices for occluded lights.
Refactor LightGatherer to collect lights across floors into the viewport buffer using projected floor offsets and a collection margin.
Implement ground-occlusion registration that blocks lights from floors below solid ground tiles at their projected positions.
Rework LightManager to compute lighting directly over the viewport with OTClient-style ambient rules and optional injected lights.
Remove LightCache usage and chunk grid copying, instead initializing a viewport buffer and computing lighting globally over it.
Add renderClientVisible to derive start/end floors from FloorVisibilityCalculator and optionally inject extra lights like player light.
Implement computeViewportLight that applies ambient/global light based on camera floor, client slider, and server global light, then blends all lights constrained by tile_start indices.
Allow external callers (e.g., preview) to pass injected lights that are projected to camera floor and merged into the viewport light list.
Align floor visibility and light blocking with OTClient semantics, including new item flags and translucent light propagation.
Update FloorVisibilityCalculator to use first item (ground or top item), respect IgnoreLook and BlockMissiles flags, and change how covered tiles and upper tiles are traversed when determining first visible floor.
Extend ItemType and ClientDataService merging to carry translucency-related lens_help, and introduce a TranslucentLight tile flag to mark tiles lit by translucent ground above.
Add helpers in LightGatherer to detect ground that blocks light from below and tiles/items that carry translucent light, using lens_help and is_translucent to propagate minimal light from above-ground translucent tiles.
Introduce server-style global light configuration and UI controls for both editor and ingame preview lighting.
Extend LightConfig with GlobalLight, client_slider, camera_floor, and adjust ambient_level/color usage; ensure ambient is derived differently above vs below ground.
Wire LightingPass and IngamePreviewRenderer to build LightConfig from ViewSettings, track a config hash for invalidation, and call renderClientVisible/render with injected player light for preview.
Add new lighting controls in MainWindow and IngameBoxWindow for server light intensity/color and client minimum ambient sliders, persisting them via ViewSettings and EditorSession state and CallbackMediator wiring.
Propagate light visibility origin and support player/selection-based light visibility similar to RME.
Extend RenderContext, MapRenderer, MapPanel, and MapPanelInput to track and propagate an optional light_visibility_origin based on last click/selection/paste, resetting when the camera floor changes.
Use FloorVisibilityCalculator with visibility_origin in LightManager::renderClientVisible to compute start/end floors that match RME client view rather than show-all-floors toggles.
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!
This PR replaces chunk-cache lighting with a viewport-buffer pipeline. It adds a server/global light model (GlobalLight), extends LightConfig and domain types for creature/item light/lens data, introduces ViewportLightBuffer and a refactored LightGatherer to populate it, updates LightManager to compute viewport lighting (optionally merging injected lights) and invalidate on a LightConfig hash, persists server-light settings through ViewSettings/EditorSession and tab callbacks, adds UI pickers/sliders, and threads an optional light-visibility origin from input into rendering.
Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'feat(lighting): support for OTClient style light rendering' accurately and concisely summarizes the main change - adding OTClient-style light rendering support with viewport-based light buffers, lighting config updates, and visual palette picker.
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.
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces a significant rework of the lighting system to support OTClient-style light rendering, including server-side global light settings and client-side ambient sliders. It also improves floor visibility logic to better match client behavior.
🔍 General Feedback
Correctness: Identified a potential regression in FloorVisibilityCalculator::isLookPossible where null tiles are treated as non-transparent.
Maintainability: There is some code duplication (projected floor offset calculation) and use of magic numbers for default light settings.
Performance: The removal of the light cache simplifies the system but introduces a full viewport re-render for lighting on every change. This should be monitored for performance impacts.
Completeness: The PR is comprehensive in its UI and rendering updates, but would benefit from unit tests for the complex visibility and light gathering logic.
Overall, the rework brings the editor much closer to in-game rendering accuracy. Fixing the null-tile visibility check is recommended before merging.
The reason will be displayed to describe this comment to others. Learn more.
🟠 This helper function is duplicated here and in LightManager.cpp. It should be moved to a common header (e.g., Config.h or a utility file) to avoid redundancy.
The reason will be displayed to describe this comment to others. Learn more.
🟡 With the removal of LightCache, fine-grained invalidation is no longer possible. While this simplifies the implementation, it means any map change (no matter how small) will cause a full re-gather and re-compute of the viewport lighting. This might be a performance bottleneck for very large viewports or slow CPUs. Consider if a simple bounding-box check could be used to only re-render light if the change is within the viewport.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Magic numbers 215 (default color) and 200 (default intensity) are used here and in other files. It would be better to define them as named constants in Domain::LightConfig or Core::Config.
The reason will be displayed to describe this comment to others. Learn more.
🔴 Returning false for !tile (null tiles) seems incorrect. Empty space should be transparent to light and view. The previous implementation correctly returned true. This change might cause visibility issues in areas with empty tiles.
The reason will be displayed to describe this comment to others. Learn more.
🟢 The comment says ambient_level is "computed from global_light + slider", but the code only assigns global_light.intensity. The actual combined computation happens in LightManager::computeViewportLight. The comment should be updated to reflect that these are now purely for backward compatibility and might not reflect the final rendered intensity.
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements RME-style viewport lighting and floor visibility culling by introducing a global viewport light buffer, tracking server-side global light settings alongside a client minimum ambient slider, and propagating light through translucent tiles. However, two critical issues were identified in the visibility culling logic: tileLimitsFloorsView incorrectly ignores secondary items on a tile and early-exits on IgnoreLook flags, and isLookPossible incorrectly treats empty tiles as opaque blockers instead of transparent space.
The reason will be displayed to describe this comment to others. Learn more.
The simplified implementation of tileLimitsFloorsView only checks the first_thing (either the ground item or the first item in tile->getItems()). This introduces two correctness issues:
Ignoring other items: If a tile has a non-blocking ground (or translucent ground) and a blocking wall/roof in tile->getItems(), the blocking effect of the wall/roof will be completely ignored because only the ground is checked.
Early exit on IgnoreLook: If the first item has the IgnoreLook flag, the function returns false immediately, ignoring any other blocking items on the tile.
To fix this, we should check the ground item first, and then iterate through all items in tile->getItems(), skipping only those with IgnoreLook and returning true if any item blocks the view.
boolFloorVisibilityCalculator::tileLimitsFloorsView(constDomain::Tile*tile, boolis_free_view) const {
if (!tile) return false;
auto check_item_blocking= [this, is_free_view](constDomain::Item*item) ->bool {
if (!item) returnfalse;
constDomain::ItemType*item_type=getItemType(item);
if (!item_type) return false;
if (item_type->hasFlag(Domain::ItemFlag::IgnoreLook)) {
return false;
}
constboolis_ground_tile=item_type->isGround() ||item_type->is_ground;
constboolis_bottom=item_type->always_on_bottom||item_type->is_on_bottom;
constboolblocks_projectile=item_type->blocks_projectile||item_type->hasFlag(Domain::ItemFlag::BlockMissiles);
if (is_free_view) {
returnis_ground_tile||is_bottom;
}
returnis_ground_tile|| (is_bottom&&blocks_projectile);
};
if (check_item_blocking(tile->getGround())) {
return true;
}
for (const auto&item : tile->getItems()) {
if (check_item_blocking(item.get())) {
return true;
}
}
return false;
}
The reason will be displayed to describe this comment to others. Learn more.
An empty/null tile represents empty space, which is transparent and does not block the line of sight. Returning false here incorrectly treats empty tiles as opaque blockers, which will break the floor visibility culling algorithm when empty tiles are present around the camera.
We should restore the original behavior and return true when tile is nullptr.
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
In FloorVisibilityCalculator::isLookPossible, changing the behavior for a null tile from true to false makes empty tiles count as opaque; if this isn’t strictly required for OTClient compatibility, consider reverting or documenting this to avoid surprising visibility behavior.
In LightGatherer::gatherLightsForViewportFloor, the special-case branch for floor == Config::Map::GROUND_LAYER + 1 computes above_offset but still uses floor_offset when projecting translucent-light tiles; this looks like it will misproject those lights and may need to use above_offset instead.
The projectedFloorOffsetTiles helper is duplicated in both LightGatherer.cpp and LightManager.cpp; consider centralizing this in a shared header or utility to avoid divergence between the two implementations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In FloorVisibilityCalculator::isLookPossible, changing the behavior for a null tile from `true` to `false` makes empty tiles count as opaque; if this isn’t strictly required for OTClient compatibility, consider reverting or documenting this to avoid surprising visibility behavior.
- In LightGatherer::gatherLightsForViewportFloor, the special-case branch for `floor == Config::Map::GROUND_LAYER + 1` computes `above_offset` but still uses `floor_offset` when projecting translucent-light tiles; this looks like it will misproject those lights and may need to use `above_offset` instead.
- The `projectedFloorOffsetTiles` helper is duplicated in both LightGatherer.cpp and LightManager.cpp; consider centralizing this in a shared header or utility to avoid divergence between the two implementations.
## Individual Comments### Comment 1
<locationpath="ImguiMapEditor/Rendering/Visibility/FloorVisibilityCalculator.cpp"line_range="24-33" />
<code_context>
+ const Domain::Item* first_thing = tile->getGround();
</code_context>
<issue_to_address>
**issue (bug_risk):** Considering only the first item when limiting floor visibility can miss blockers
The old `tileLimitsFloorsView` walked all items on the tile; now it only inspects `ground` / the first item from `getItems()`. That can mark a tile as non-blocking when a blocking wall/object is further down the stack.
If you’re optimizing, you could still fast-path on `first_thing` when it clearly blocks, but then fall back to scanning the rest of the items when it doesn’t, to preserve the previous behavior while using the new flag-based logic.
</issue_to_address>
### Comment 2
<locationpath="ImguiMapEditor/Rendering/Visibility/FloorVisibilityCalculator.cpp"line_range="67-22" />
<code_context>
}
bool FloorVisibilityCalculator::isLookPossible(const Domain::Tile* tile) const {
- if (!tile) return true; // Empty tile is transparent
+ if (!tile) return false;
// Check all items for projectile blocking
</code_context>
<issue_to_address>
**issue (bug_risk):** Treating missing tiles as opaque in isLookPossible changes previous semantics
Previously, `isLookPossible` returned `true` for `nullptr`, treating empty tiles as transparent. Now `nullptr` returns `false`, so diagonal checks in `calcFirstVisibleFloor` will treat missing tiles as opaque. This can cause floors to be marked as covered when the map simply has no tile (e.g., sparse/partially loaded maps). If non-existent tiles should be considered empty space, this condition should remain `true` or missing tiles should be distinguished from solid ones explicitly.
</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.
issue (bug_risk): Considering only the first item when limiting floor visibility can miss blockers
The old tileLimitsFloorsView walked all items on the tile; now it only inspects ground / the first item from getItems(). That can mark a tile as non-blocking when a blocking wall/object is further down the stack.
If you’re optimizing, you could still fast-path on first_thing when it clearly blocks, but then fall back to scanning the rest of the items when it doesn’t, to preserve the previous behavior while using the new flag-based logic.
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Treating missing tiles as opaque in isLookPossible changes previous semantics
Previously, isLookPossible returned true for nullptr, treating empty tiles as transparent. Now nullptr returns false, so diagonal checks in calcFirstVisibleFloor will treat missing tiles as opaque. This can cause floors to be marked as covered when the map simply has no tile (e.g., sparse/partially loaded maps). If non-existent tiles should be considered empty space, this condition should remain true or missing tiles should be distinguished from solid ones explicitly.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR successfully implements the requested OTClient-style lighting system, including translucent propagation and ground-blocking mechanisms. However, the overall quality is currently not up to standards. The logic in LightGatherer.cpp is identified as high-risk due to high cyclomatic complexity paired with a lack of automated test coverage. Additionally, performance bottlenecks were identified in LightManager.cpp regarding redundant vector allocations during chunk processing. These architectural and quality issues, along with duplicated hashing logic across render passes, should be addressed to ensure long-term maintainability.
About this PR
The LightConfig struct in LightTypes.h retains deprecated ambient_color and ambient_level fields. These should be removed to clean up the domain model and ensure the code reflects the new global light and client slider logic.
Test suggestions
Verify light sources are correctly extracted from both creature types and item types during chunk gathering.
Verify that multiple light sources at the same coordinate/floor are deduplicated and keep the highest intensity.
Verify translucent propagation: tiles on z=8 should receive light if the tile directly above on z=7 is marked as translucent or has lens_help.
Verify ground blocking: a light source on floor 8 should not illuminate floor 7 if floor 7 has a solid (non-translucent) ground item.
Verify ambient light calculation: above-ground (z <= 7) should use the server's global light as a base, while underground (z > 7) should be black by default.
Unit test coverage for high-complexity logic in LightGatherer.cpp (specifically ground blocking and floor propagation).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Unit test coverage for high-complexity logic in LightGatherer.cpp (specifically ground blocking and floor propagation).
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The configuration hashing logic used for cache invalidation is duplicated here and in LightingPass.cpp. This should be centralized to ensure that adding new fields to LightConfig correctly triggers invalidation across all render passes.
Try running the following prompt in your IDE agent:
Add a uint32_t computeHash() const method to the LightConfig struct in ImguiMapEditor/Domain/LightTypes.h that returns the combined hash of intensity, color, and slider. Then, update IngamePreviewRenderer.cpp and LightingPass.cpp to use this method for cache invalidation.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
This method handles multiple distinct concerns: item-based lighting, creature-based lighting, and z-axis propagation. Given the high complexity and lack of coverage for this file, moving the 'Translucent ground propagation' logic (lines 215-239) into its own method would reduce the function length and facilitate isolated testing.
Try running the following prompt in your IDE agent:
In ImguiMapEditor/Rendering/Light/LightGatherer.cpp, extract the translucent ground propagation logic (lines 215-239) into a separate private method named handleFloorPropagation to reduce the length of gatherLightsFromNeighborChunk.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
This method's complexity (12) stems from nested tile processing within chunk iteration. As this file is identified as complex and lacks automated coverage, refactoring is highly recommended to improve testability. Extracting the logic that identifies and registers blocking tiles into a private helper method would clarify the implementation.
Try running the following prompt in your IDE agent:
Refactor MapEditor::Rendering::LightGatherer::registerGroundBlockingForChunk in ImguiMapEditor/Rendering/Light/LightGatherer.cpp by extracting the tile processing logic inside the forEachTile lambda into a private helper function named processTileBlocking.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Avoid copying the gathered lights vector for every chunk. Try running the following prompt in your coding agent:
Update computeChunkLight to accept an optional second vector of injected lights and iterate over both in the light processing loop to avoid the vector copy on line 190 of LightManager.cpp.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🤖 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/Domain/LightTypes.h`:
- Around line 38-39: Update the DEPRECATED comments for the fields ambient_color
and ambient_level in LightTypes.h to make clear they are only stored for
backward compatibility and not computed here; e.g., state that ambient_level is
initialized to 255 and any computation combining global_light and a slider
occurs elsewhere in the rendering pipeline (not in this struct) so developers
won’t expect logic inside the struct definition.
In `@ImguiMapEditor/Domain/Tile.h`:
- Line 30: The comment on the TranslucentLight flag in Tile.h is ambiguous about
"z=8" — update the comment for TranslucentLight to state explicitly whether z=8
is merely an example or a required constraint; if it is an example, change the
text to something like "e.g. z=8" and say the flag marks an underground tile
that is lit by translucent ground above; if z=8 is special, state that
explicitly and briefly explain why (what z coordinate system or rendering rule
makes z==8 special) and where that invariant is enforced (e.g., reference the
rendering/lighting code or function that checks z). Ensure the updated comment
references the TranslucentLight flag so readers know the intended scope.
In `@ImguiMapEditor/IO/CreatureXmlReader.cpp`:
- Around line 138-144: The light attributes are being cast to uint8_t without
range checks causing silent truncation; update the handling for
node.attribute("lightlevel") and node.attribute("lightcolor") in
CreatureXmlReader.cpp to read the value into a wider type (e.g., unsigned int),
validate it is <= 255, and if it exceeds 255 log or warn (include attribute name
and raw value) and clamp to 255 before assigning to creature->light_level and
creature->light_color (use static_cast<uint8_t> on the validated/clamped value).
In `@ImguiMapEditor/Rendering/Light/LightGatherer.cpp`:
- Around line 16-25: The function projectedFloorOffsetTiles is duplicated across
LightGatherer.cpp and LightManager.cpp; extract it into a single shared header
(e.g. a MapLightingUtils or LightHelpers header) as an inline/free function in
the same anonymous or project namespace, export or include that header in both
LightGatherer.cpp and LightManager.cpp, and remove the duplicate definitions
from the cpp files so both units call the single shared
projectedFloorOffsetTiles implementation; ensure any dependent constants like
LIGHT_COLLECTION_MARGIN_TILES or Config::Map::GROUND_LAYER remain visible to the
shared header or are referenced from it.
In `@ImguiMapEditor/Rendering/Light/LightManager.cpp`:
- Around line 16-25: The function projectedFloorOffsetTiles is duplicated;
remove the duplicate in LightManager.cpp and consolidate to a single shared
implementation (e.g., move the function to a common header/source or an existing
utility module) so both LightManager and LightGatherer call that single symbol.
Update includes in the files that used the duplicate so they include the new
header, keep the function in an appropriate namespace, and ensure only one
definition of projectedFloorOffsetTiles remains in the codebase.
In `@ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.cpp`:
- Around line 226-259: The config hash computed in IngamePreviewRenderer
(config_hash) omits config.camera_floor but LightManager::render checks
camera_floor when deciding config_changed; include camera_floor in the hash
computation so changes to floor update last_config_hash_ and trigger
invalidateAll() consistently—update the config_hash expression to incorporate
config.camera_floor (or floor) alongside config.global_light.intensity,
config.global_light.color, and config.client_slider so IngamePreviewRenderer and
LightManager use matching invalidation criteria.
In `@ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.h`:
- Around line 70-71: Remove the unused field last_ambient_light_ from the
IngamePreviewRenderer class: locate the declaration "uint8_t last_ambient_light_
= 255;" (next to last_config_hash_) in IngamePreviewRenderer.h and delete it;
ensure no other code refers to last_ambient_light_ (if there are references,
either implement their intended logic or update those sites to use
last_config_hash_ or another appropriate member), then rebuild to confirm no
remaining references.
🪄 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: 316b00cc-0982-40ef-a4a0-2b5246ce5230
📥 Commits
Reviewing files that changed from the base of the PR and between d6edc18 and a230e91.
…nagement
Centralized lighting configuration constants, extracted projection logic into a shared header, and added hashing support to `LightConfig` for efficient cache invalidation.
- `ImguiMapEditor/Core/Config.h` — introduced `Lighting` namespace with default configuration constants
- `ImguiMapEditor/Domain/LightTypes.h` — updated to use central lighting constants and added `computeHash()` to `LightConfig`
- `ImguiMapEditor/Domain/Tile.h` — updated `TranslucentLight` documentation
- `ImguiMapEditor/IO/CreatureXmlReader.cpp` — added light attribute clamping for creature parsing
- `ImguiMapEditor/Rendering/Frame/RenderState.h` — updated `last_config_hash` documentation
- `ImguiMapEditor/Rendering/Light/LightGatherer.cpp` — utilized shared `projectedFloorOffsetTiles` and centralized lighting constants
- `ImguiMapEditor/Rendering/Light/LightManager.cpp` — removed local `projectedFloorOffsetTiles`, updated ambient calculation to use central constants
- `ImguiMapEditor/Rendering/Light/LightProjection.h` — created new header for shared projection logic
- `ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.cpp` — utilized `config.computeHash()` and centralized constants for player lights
- `ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.h` — removed unused `last_ambient_light_` member
- `ImguiMapEditor/Rendering/Passes/LightingPass.cpp` — refactored invalidation logic to use `config.computeHash()`
- `ImguiMapEditor/Services/ViewSettings.cpp` — updated settings loading to use central lighting constants
- `ImguiMapEditor/Services/ViewSettings.h` — updated lighting settings to use central constants
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces a significant rework of the lighting system to support OTClient-style light rendering, including global server light intensity/color, floor-based visibility for light propagation, and creature/item light properties.
The architectural changes are solid, especially the move towards a more realistic visibility algorithm and the addition of per-floor light propagation.
🔍 General Feedback
Correctness: The visibility calculator has been refined to match client-side logic (covered position shift), which is a great improvement. However, some optimizations in tileLimitsFloorsView might miss blockers if they are not the first item on the tile.
Maintainability: The use of computeHash for cache invalidation is a clean way to handle lighting updates.
UI/UX: The addition of server light sliders in the preview window is very helpful for testing in-game aesthetics directly within the editor.
Performance: The removal of LightCache in favor of a direct ViewportLightBuffer should be monitored for any regressions on high-resolution displays, although it likely simplifies the pipeline.
Please address the minor concerns regarding tile blocking logic and the potential regression in isLookPossible for null tiles.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changing the return value for null tiles from true to false might cause issues in areas where tiles are not yet loaded or are intentionally left empty. Typically, empty space (null tile) in Tibia is considered transparent. Is this change intentional to prevent rendering artifacts in unloaded chunks?
The reason will be displayed to describe this comment to others. Learn more.
🟠 The new logic only checks the ground or the very first item in the tile's item list. If a tile has multiple items (e.g., a non-blocking decoration followed by a blocking wall), this will fail to identify the tile as a blocker. The previous implementation correctly iterated through all items on the tile.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Why is the color slider limited to 215? While 215 is the standard white light in the Tibia palette, other indices (0-255) might be used for colored world lighting effects in some servers. Consider allowing the full 0-255 range.
render() now accepts injected_lights, but the reuse path ignores them. If a preview/player light moves while bounds and LightConfig stay the same, this returns the previous texture and the injected light appears frozen. At minimum, bypass the fast path when injected lights are present; ideally hash them into the cached state.
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/Light/LightManager.cpp` around lines 117 - 128, The
fast-return path in render() (the branch gated by bounds_changed,
config_changed, and force_update_) ignores the new injected_lights parameter
causing moving preview/player lights to appear frozen; update the logic to
either bypass this fast path when injected_lights is non-empty or incorporate
injected_lights into the cache key used for texture reuse (e.g., include a
hash/unique identifier derived from injected_lights when deciding reuse). Modify
the condition that checks bounds_changed/config_changed/force_update_ to also
check injected_lights.empty() (for the minimal fix) or add injected_lights
hashing into the cached state comparison used before calling overlay_->apply
with texture_->getTextureId() so the cached texture is invalidated when injected
lights change.
Player light color matches the comment (no mismatch) Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR is defined as 215, so the preview player light’s color aligns with the comment’s OTClient local player color=215 (white). Optional: adjust the comment wording to avoid implying this is a separately defined “player” color constant.
🤖 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/Passes/IngamePreviewRenderer.cpp` around lines 242 -
251, The comment implies a distinct "player" color constant but the code uses
Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR (value 215), so update the comment
near the player light initialization in IngamePreviewRenderer.cpp to reflect
that it reuses the server default color rather than a separate player color;
mention Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR and the player light
construction (player_lights vector and Domain::LightSource fields) so readers
know which symbols are involved.
🤖 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/Light/LightGatherer.cpp`:
- Around line 232-235: The translucent propagation path in LightGatherer.cpp is
hard-coding Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR and
Config::Lighting::TRANSLUCENT_PROPAGATION_INTENSITY in the addLight(...) call;
instead obtain the current active/global server light settings (tint and
propagation intensity) from the runtime LightManager or the session preview
state and pass those values into addLight(projected_x, projected_y, ..., floor)
so propagation matches day/night configuration — either thread the active light
struct through this call site or replace the literal constants with a
LightManager::getActivePropagationTint() / getActivePropagationIntensity()
accessor and use those returned values when calling addLight.
---
Outside diff comments:
In `@ImguiMapEditor/Rendering/Light/LightManager.cpp`:
- Around line 117-128: The fast-return path in render() (the branch gated by
bounds_changed, config_changed, and force_update_) ignores the new
injected_lights parameter causing moving preview/player lights to appear frozen;
update the logic to either bypass this fast path when injected_lights is
non-empty or incorporate injected_lights into the cache key used for texture
reuse (e.g., include a hash/unique identifier derived from injected_lights when
deciding reuse). Modify the condition that checks
bounds_changed/config_changed/force_update_ to also check
injected_lights.empty() (for the minimal fix) or add injected_lights hashing
into the cached state comparison used before calling overlay_->apply with
texture_->getTextureId() so the cached texture is invalidated when injected
lights change.
In `@ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.cpp`:
- Around line 242-251: The comment implies a distinct "player" color constant
but the code uses Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR (value 215), so
update the comment near the player light initialization in
IngamePreviewRenderer.cpp to reflect that it reuses the server default color
rather than a separate player color; mention
Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR and the player light construction
(player_lights vector and Domain::LightSource fields) so readers know which
symbols are involved.
🪄 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: dea9ecd7-c8e4-435c-a947-3182f4cd7dd8
📥 Commits
Reviewing files that changed from the base of the PR and between a230e91 and 9a4d5df.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Use the active server/global light for translucent propagation.
This path hard-codes the default server light color and propagation intensity, so changing the new session/preview server light settings will not change the light leaking through translucent ground. Please thread the current global light into this step, or defer the propagation tint/intensity decision to LightManager, so underground propagation stays consistent with the configured day/night state.
🤖 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/Light/LightGatherer.cpp` around lines 232 - 235, The
translucent propagation path in LightGatherer.cpp is hard-coding
Config::Lighting::DEFAULT_SERVER_LIGHT_COLOR and
Config::Lighting::TRANSLUCENT_PROPAGATION_INTENSITY in the addLight(...) call;
instead obtain the current active/global server light settings (tint and
propagation intensity) from the runtime LightManager or the session preview
state and pass those values into addLight(projected_x, projected_y, ..., floor)
so propagation matches day/night configuration — either thread the active light
struct through this call site or replace the literal constants with a
LightManager::getActivePropagationTint() / getActivePropagationIntensity()
accessor and use those returned values when calling addLight.
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request introduces a significant rework of the lighting system, bringing it much closer to OTClient and RME behavior. Key highlights include support for creature and item light properties, global world light (intensity and color), and a more accurate floor visibility algorithm.
The implementation is generally solid, with good use of caching and a hashing mechanism to minimize unnecessary recalculations.
🔍 General Feedback
Lighting Accuracy: The addition of server-style light intensity and color, along with player light in the preview, greatly enhances the editor's utility for map designers.
Performance: Using a hash of the lighting configuration for cache invalidation is a great approach.
UI/UX: The new sliders for server light properties are useful, though the behavior of the light visibility origin following every click might need a quick check to ensure it doesn't become distracting during normal editing.
Optimization: The floor visibility calculator is a bit loop-heavy with tile lookups; minor optimizations here could benefit larger maps.
Overall, a very high-quality contribution that improves the visual fidelity of the editor.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The computeHash function currently excludes the enabled field. While LightingPass checks this field before calling render, it might be safer to include it in the hash to ensure the cache is invalidated if the master toggle is changed, or add a comment explaining why it's omitted.
The reason will be displayed to describe this comment to others. Learn more.
🟠 Using attr.as_uint() and then checking > 255 is safe, but ensure that the warning message (which was truncated in the diff) clearly identifies which creature has the invalid light property to help users fix their creatures.xml.
The reason will be displayed to describe this comment to others. Learn more.
🟢 Updating the light visibility origin on every left-click (paste, selection, etc.) will cause the lighting focus to shift constantly. Is this the intended behavior for the editor? In many cases, users might prefer the lighting to stay focused on a specific area or follow the camera.
The reason will be displayed to describe this comment to others. Learn more.
🟡 The loop for calculating floor visibility performs multiple map.getTile calls per iteration. Given that this is called frequently during rendering, consider if there's a more efficient way to access these tiles, perhaps by checking if the chunk has changed before doing a full tile lookup.
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the lighting system to use a single viewport-wide light buffer and introduces global server light settings alongside client ambient sliders. It also updates floor visibility calculations and adds creature light property parsing. Feedback highlights two critical regressions in FloorVisibilityCalculator: returning false for null tiles in isLookPossible incorrectly blocks sight, and checking only the first item in tileLimitsFloorsView fails to account for other blocking items on the tile.
The reason will be displayed to describe this comment to others. Learn more.
Returning false when tile is nullptr in isLookPossible causes the floor visibility algorithm to treat empty/null tiles as blocking sight. As a result, in calcFirstVisibleFloor, the algorithm will skip checking upper floors for any neighbor positions that have null tiles on the camera floor. This means any roofs or blockers on upper floors at those positions will be completely ignored, leading to incorrect rendering of higher floors that should be occluded.
It should return true for null tiles to indicate that empty space is transparent and allows looking through to upper floors.
The reason will be displayed to describe this comment to others. Learn more.
In tileLimitsFloorsView, the new implementation only checks the first_thing (either the ground or the first item in getItems()) to determine if the tile limits the floor view. This is a regression from the original implementation which iterated through all items on the tile.
If a tile has no ground but contains multiple items (e.g., a decorative item and a wall), and the decorative item happens to be the first in the list, the wall will be completely ignored, and the tile will incorrectly be treated as not limiting the view.
To ensure correctness, the function should check all items on the tile, or at least ensure that any blocking items (like walls/ground) are not missed.
boolFloorVisibilityCalculator::tileLimitsFloorsView(constDomain::Tile*tile, boolis_free_view) const {
if (!tile) return false;
const auto check_blocking= [&](constDomain::Item*item) {
if (!item) return false;
constDomain::ItemType*item_type=getItemType(item);
if (!item_type||item_type->hasFlag(Domain::ItemFlag::IgnoreLook)) {
return false;
}
constboolis_ground_tile=item_type->isGround() ||item_type->is_ground;
constboolis_bottom=item_type->always_on_bottom||item_type->is_on_bottom;
constboolblocks_projectile=item_type->blocks_projectile||item_type->hasFlag(Domain::ItemFlag::BlockMissiles);
if (is_free_view) {
returnis_ground_tile||is_bottom;
}
returnis_ground_tile|| (is_bottom&&blocks_projectile);
};
if (check_blocking(tile->getGround())) {
return true;
}
for (const auto&item : tile->getItems()) {
if (check_blocking(item.get())) {
return true;
}
}
return false;
}
Ensure dynamic preview/player lights are correctly recomputed every frame by disabling texture reuse when injected lights are active. Updated preview renderer comments to clarify the source of default player light colors.
- `ImguiMapEditor/Rendering/Light/LightManager.cpp` — added `has_injected_lights` check to force recomputation when dynamic lights are present
- `ImguiMapEditor/Rendering/Passes/IngamePreviewRenderer.cpp` — [minor] updated comment to reference `DEFAULT_SERVER_LIGHT_COLOR`
Replaced numerical sliders for light color selection with a visual palette picker to improve usability and offer a preview of Tibia's 8-bit color palette.
- `ImguiMapEditor/CMakeLists.txt` — added `LightColorPalettePicker.cpp` to build sources
- `ImguiMapEditor/Core/Config.h` — defined `LIGHT_COLOR_COUNT` and `MAX_SERVER_LIGHT_COLOR` constants
- `ImguiMapEditor/Presentation/MainWindow.cpp` — integrated `LightColorPalettePicker` into main editor light settings
- `ImguiMapEditor/UI/Widgets/LightColorPalettePicker.cpp` — implemented popup widget with palette grid, swatches, and hover tooltips
- `ImguiMapEditor/UI/Widgets/LightColorPalettePicker.h` — defined `LightColorPalettePicker` interface
- `ImguiMapEditor/UI/Windows/IngameBoxWindow.cpp` — replaced slider with `LightColorPalettePicker` in ingame box settings
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This PR introduces a significant rework of the lighting system to support OTClient-style light rendering, including server light intensity/color persistence, creature/item light properties, and dynamic light visibility based on floor blocking rules.
🔍 General Feedback
The new lighting model is much more authentic to the Tibia/OTClient experience. The addition of the LightColorPalettePicker is a great UI improvement for configuring world lighting.
I identified a few issues that should be addressed:
🔴 Light Projection Bug: High-floor lights and ground occlusion seem to be shifted NW instead of SE due to a sign error in the projection logic.
🟠 Cache Invalidation: The light texture reuse optimization might lead to stale dynamic lights staying visible when they should have been removed.
🟡 Performance: Some iteration loops over chunks could be optimized, though they are likely acceptable for the editor's typical viewport.
Overall, a solid enhancement to the rendering pipeline!
The reason will be displayed to describe this comment to others. Learn more.
🔴 The projection logic for multi-floor lighting appears to be reversed.
In projectedFloorOffsetTiles, floor 6 (above floor 7) returns an offset of 1.
In LightGatherer::gatherLightsForViewportFloor, this is used as projected_x = tile->getX() - floor_offset, resulting in x - 1.
However, in TerrainPass.cpp, higher floors are shifted SE (positive offset), so a tile at world x on floor 6 is drawn at screen x + 1.
This causes light sources and ground occlusion to be misaligned with the terrain they are supposed to affect on floors other than the current one.
The reason will be displayed to describe this comment to others. Learn more.
🔴 The min_x/y and max_x/y calculation for visible chunks is also affected by the reversed floor_offset.
If floor_offset is 1 for floor 6, we want to gather world tiles x such that x + 1 is on screen. So x should be screen_x - 1.
The current code uses light_buffer.origin_x + floor_offset, which is screen_x + 1.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Performance concern: forEachTile iterates over all 1024 tiles in each visible chunk for every floor from start_floor to current_floor. While necessary to find ground occlusion, this could be expensive for large viewports or many floors.
Since this logic is primarily to support "OTClient style" visibility where ground blocks light from below, perhaps the occlusion can be gathered more efficiently or cached alongside the terrain mesh.
The reason will be displayed to describe this comment to others. Learn more.
🟠 Potential stale cache issue.
The optimization to reuse the light texture doesn't account for the case where injected_lights were present in the previous frame but are absent in the current one. If has_injected_lights becomes false, but bounds_changed and config_changed are also false, the renderer will reuse the texture that still contains the "baked" lights from the previous frame.
Consider tracking last_has_injected_lights_ or forcing an update if the injected_lights list changed its state.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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/Light/LightManager.cpp`:
- Around line 114-120: The cache reuse logic incorrectly ignores transitions
where injected_lights were present last frame but are absent now, leaving stale
injected lighting; modify LightManager to track prior injected-light presence
(e.g., add a member like had_injected_lights_ or similar) and update it each
frame, then change the reuse condition (the check using injected_lights /
has_injected_lights and force_update_) so that a change in injected-light state
(had_injected_lights_ != has_injected_lights) forces a recompute (set
force_update_ or bypass reuse) and then update had_injected_lights_ to the
current has_injected_lights for the next frame.
🪄 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: 8ed38426-15c6-488f-88f1-ea3aa34f5fd2
📥 Commits
Reviewing files that changed from the base of the PR and between 9a4d5df and 181dfbf.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Handle injected-light state transitions in cache reuse.
If injected lights were present in the previous frame but are absent now, this condition can still reuse the old texture and keep stale injected lighting visible. Track prior injected-light presence and force recompute on state changes.
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
[warning] 114-114: variable 'has_injected_lights' is not initialized
(cppcoreguidelines-init-variables)
🪛 Cppcheck (2.20.0)
[style] 115-115: The function 'getDataDirectory' is never used.
(unusedFunction)
[style] 116-116: The function 'setDataDirectory' is never used.
(unusedFunction)
[style] 118-118: The function 'setDescription' is never used.
(unusedFunction)
[style] 115-115: The function 'getDuration' is never used.
(unusedFunction)
[style] 116-116: The function 'setDuration' is never used.
(unusedFunction)
[style] 119-119: The function 'getSubtype' is never used.
(unusedFunction)
[style] 120-120: The function 'setSubtype' 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/Rendering/Light/LightManager.cpp` around lines 114 - 120, The
cache reuse logic incorrectly ignores transitions where injected_lights were
present last frame but are absent now, leaving stale injected lighting; modify
LightManager to track prior injected-light presence (e.g., add a member like
had_injected_lights_ or similar) and update it each frame, then change the reuse
condition (the check using injected_lights / has_injected_lights and
force_update_) so that a change in injected-light state (had_injected_lights_ !=
has_injected_lights) forces a recompute (set force_update_ or bypass reuse) and
then update had_injected_lights_ to the current has_injected_lights for the next
frame.
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: EnhancementNew feature or request.
1 participant
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
![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}
![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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Improvements