Skip to content

Cherry-Pick: Fix "Match Corners" not correctly updating cells when erasing (godotengine/godot#112145) - #1460

Open
morgothaufheroin88 wants to merge 2 commits into
Redot-Engine:masterfrom
morgothaufheroin88:fix/tilemap-match-corners-erase
Open

morgothaufheroin88 wants to merge 2 commits into
Redot-Engine:masterfrom
morgothaufheroin88:fix/tilemap-match-corners-erase

Conversation

@morgothaufheroin88

@morgothaufheroin88 morgothaufheroin88 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1292

Cherry-pick of godotengine/godot#112145 (upstream commit b2a4bda3b0d1fdd9707ca4053c6bb1300a29f7bd by @IphStich, merged 2025-10-29), plus a regression test.

TileMapLayer::terrain_fill_pattern() — the path the terrains eraser uses — collected the cells it may rewrite by walking only the neighbors reachable through a valid peering bit of the terrain set:

if (tile_set->is_valid_terrain_peering_bit(p_terrain_set, bit)) {

With Match Corners the only valid bits are the four corners, so for a square grid that neighborhood is just the four diagonal cells. But a square cell's corner points are each shared by four cells, so the four orthogonal neighbors share corners with the erased cell as well — they get new constraints and are then never updated, which is the stale tiles reported in #1292. Match Corners and Sides has side bits too, so all eight neighbors end up in the list and the bug does not show there, exactly as the issue describes.

Upstream's fix walks every existing neighbor instead, like terrain_fill_connect() (the painting path) already did:

if (tile_set->is_existing_neighbor(bit)) {

Cells whose constraints did not change keep their pattern, so Match Sides is unaffected.

Testing

Adds tests/scene/test_tile_map_layer.h (there were no TileMapLayer tests). It builds a tile set per terrain mode with one tile for every combination of that mode's peering bits, fills a 5x5 block, erases the center cell the way the eraser does, and checks which cells terrain_fill_pattern() returns.

  • On master the corners subcase fails on exactly the four orthogonal neighbors; corners and sides and sides pass.
  • With the fix: 23/23 assertions pass.
  • Full suite: 1350/1350 test cases, 2 459 004 assertions pass.
  • pre-commit run -a clean.

Also tested by hand in the editor with the MRP attached to #1292 (Match Corners, square 60x60 tiles), erasing cells with patched and unpatched builds of the same commit side by side.

Summary by CodeRabbit

  • Bug Fixes
    • Terrain pattern fills now account for all existing surrounding cells, including neighbors without a matching terrain peering bit.
    • Erasing a cell during a terrain pattern fill now includes the relevant neighboring cells in the fill result. Orthogonal neighbors are included, with diagonal neighbors also included for corner-matching modes.

IphStich and others added 2 commits October 1, 2026 20:23
…ult in all neighbors updating correctly

(cherry picked from commit b2a4bda3b0d1fdd9707ca4053c6bb1300a29f7bd)
Covers the "Match Corners" erase bug fixed in the previous commit: a
square cell's corners are shared with all eight surrounding cells, so
erasing a cell has to reconsider the four orthogonal neighbors too, not
only the four diagonal ones reachable through a corner peering bit.

The test builds a tile set per terrain mode with one tile for every
combination of that mode's peering bits, fills a 5x5 block, then erases
the center cell the way the editor's eraser does and checks which cells
terrain_fill_pattern() hands back. Diagonals are only required in the
modes that have corner bits.

Before the fix the "corners" subcase fails on all four orthogonal
neighbors; "corners and sides" and "sides" pass either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@morgothaufheroin88
morgothaufheroin88 requested review from a team October 1, 2026 17:42
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: Redot-Engine/redot-engine/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 20224a79-f652-453b-b52a-95dc5cb9835c

📥 Commits

Reviewing files that changed from the base of the PR and between 80d31f7 and 355c4c3.

📒 Files selected for processing (3)
  • scene/2d/tile_map_layer.cpp
  • tests/scene/test_tile_map_layer.h
  • tests/test_main.cpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


Walkthrough

Terrain fill now includes existing neighboring cells when it builds the modifiable-cell set. New tests erase the center of a filled area across three terrain modes and check the affected cells.

Changes

Terrain Fill

Layer / File(s) Summary
Terrain fill neighbor expansion
scene/2d/tile_map_layer.cpp
terrain_fill_pattern expands the modifiable-cell set across existing neighbor directions.
Terrain erase regression coverage
tests/scene/test_tile_map_layer.h, tests/test_main.cpp
Tests check center erasure and affected neighbors in corners-and-sides, corners-only, and sides-only modes. The test header is included in the test runner.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: joltedjon, mcdubhghlas

Merge Risk: ⚪ Minimal · up to 355c4

The change addresses incomplete neighbor updates when erasing corner-matched terrain. No actionable merge-blocking risk is established; merge after normal checks pass.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. 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 clearly identifies the main change: fixing incorrect cell updates when erasing in "Match Corners" mode. It also identifies the cherry-pick source issue.
Linked Issues check ✅ Passed The change addresses #1292. TileMapLayer::terrain_fill_pattern() now collects every existing neighboring cell through is_existing_neighbor(bit), instead of limiting collection to valid terrain pee…
Out of Scope Changes check ✅ Passed The pull request changes only terrain fill neighbor collection, adds focused TileMapLayer regression coverage, and registers the test header. These changes directly support #1292. No unrelated chang…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tilesets: Match Corners does not set tiles correctly when erasing tiles.

3 participants