Fix monsters attacking through walls with ranged attacks

[eduardosmaniotto]

Problem

Monsters with ranged attacks (e.g. Hell Spider, Cursed Wizard)
could target and hit players standing behind walls. Target search
and the attack decision used pure Chebyshev distance with no terrain
check, and damage application only blocks on safezone.

Fix

• GameMapTerrain.HasLineOfSight(from, to): integer Bresenham over
  the raw .att attribute flags; only tiles with the Blocked (wall)
  bit stop projectiles — holes (NoGround) and water can be shot
  across, as in the original game (Dungeon pits, Chaos Castle ground).
  Endpoints are excluded so occupants never block themselves, and a
  diagonal step squeezing between two corner-touching wall tiles is
  treated as blocked. O(range) reads, zero allocations.
• GameMapTerrain.AttributeMap preserves the raw TerrainAttributeType
  bitmask per tile (the old code collapsed it to walkable/unwalkable and
  lost the wall-vs-hole distinction), plus BlocksSight(x, y) and
  ApplyTerrainAttribute(x, y, attribute, set) for runtime changes.
• LocateableExtensions.HasLineOfSightTo: same-map guard + delegation,
  mirroring the existing IsInRange/IsAtSafezone extension pattern.
  Call sites keep cheap IsInRange() as pre-filter, LOS second.
• AI gates (decision point, so monsters repath instead of camping):
  BasicMonsterIntelligence (search prefers nearest visible with
  nearest fallback, validity + attack gate require sight — also covers
  guards and summoned monsters' base behavior),
  SummonedMonsterIntelligence (same visible-preferred pattern),
  RandomAttackInRangeTrapIntelligence,
  AttackAreaTargetInDirectionTrapIntelligence,
  AttackAreaWhenPressedTrapIntelligence.
• Runtime terrain writers routed through the terrain so sight stays
  truthful: MiniGameContext (ground collapse), Kanturu barrier, and
  castle siege gates (close/open now saves and restores the wall bit
  instead of a stale walkability snapshot). Non-safezone changes never
  alter safezone status, exactly as before.
• Player skills intentionally untouched (client already blocks; server
  stays lenient).

Known limitations

• Unknown high bits in newer .att files (e.g. 32) default to
  non-sight-blocking — the permissive direction, closest to pre-fix
  behavior. If a map ever shows shooting through something solid, that
  tile's flag is the first lead.

Tests

• dotnet build src/GameLogic — 0 errors, no new warnings.
• Full MUnique.OpenMU.Tests: 892/892 passed (incl. 27 siege,
  133 mini-game, 18 terrain).
• GameMapTerrainTests now covers: same-tile, clear lines,
  wall blocks symmetrically, endpoints never block, closed diagonal
  corner blocked, single-wall corner visible, diagonally-adjacent
  tiles visible, hole/water don't block (while asserting they're still
  unwalkable), Blocked|NoGround blocks, diagonal-between-holes
  passes, and runtime wall/hole/safezone transitions incl. safezone
  preservation.
• One CastleSiegeNpcTests setup updated to express its pre-blocked
  tile through the terrain API (natural wall in gate area) instead of
  a raw WalkMap poke unreachable in production.

Before patch

Screencast_20260911_193219.webm

After patch

Screencast_20260911_194949.webm-10mb.mp4

[sven-n]

I think we should also prevent selecting targets which are not in line of sight. But there are some things to consider - in some terrains are not only walls, but also deep holes, e.g. in dungeon or chaos castle. Monsters with range attacks can attack over these holes.

[sven-n]

Check my comment

[eduardosmaniotto]

Now it checks for holes/water. Tested in Chaos Castle:
https://github.com/user-attachments/assets/e24df5d6-d27a-4ce8-ad74-1304ce1118ed

[sven-n]

Thanks, this is a well-structured change — keeping the raw .att bitmask instead of collapsing it to walkable/unwalkable is the right foundation, the Bresenham implementation is allocation-free, and routing all runtime terrain writers through ApplyTerrainAttribute is a real improvement over the scattered WalkMap pokes (the byte loop counter fix at coordinate 255 is a nice catch along the way).

Four things from my side, one of them blocking:

1. Blood Castle regression (blocking) — recomputing walkability from the raw bits means a mini-game terrain change now only opens an area if the configured TerrainAttribute matches the bit actually present in the terrain file. The Blood Castle bridge is configured as NoGround, but the tiles are Blocked (4) on maps 11 and 13-17, and Blocked|NoGround (12) on map 52. Only Blood Castle 2 has plain NoGround there. So on 7 of 8 levels the bridge would never open. Details and suggested fixes in the inline comment on MiniGameContext.cs. Kanturu's barrier really is all 8, so that part of the change is correct.

2. HasLineOfSight is not symmetric for the diagonal corner case — the endpoint short-circuit runs before the closed-corner check, so a corner adjacent to the target doesn't block while the same corner adjacent to the viewer does. Inline on GameMapTerrain.cs.

3. Traps latch onto targets that lose sight — RandomAttackInRangeTrapIntelligence gates the attack on sight but never invalidates _currentTarget, and a trap can't reposition. Inline on that file.

4. Duplication / reference point in the target search — inline on SummonedMonsterIntelligence.cs, non-blocking.

One question on the sight mask

Blocked|NoGround (12) currently blocks sight, and the test HasLineOfSightBlockedHoleCombinationBlocks pins that down. That value is not rare in the shipped terrain data — it's the dominant out-of-bounds marker on the arena maps (Terrain41-47, Terrain51: ~58-63k tiles each), but it also appears inside playable maps: Terrain2.att (Dungeon) has 21592 tiles of pure 8 and 4660 of 12, and Terrain52.att has it on the Blood Castle bridge. Since your original concern was exactly the dungeon/Chaos Castle pits, it'd be worth confirming in-game that the pits you meant are plain 8 and not 12 — otherwise the fix would still block those shots on some maps.

On the "unknown high bits" limitation in the description: the data supports the permissive choice. Terrain19/Terrain54 use bit 32 (659 + 2707 + 32 tiles), and Terrain11/25/31/32/35-37 carry bits 64/128 on scattered tiles. These look like the client's TW_ACTION/TW_HEIGHT/TW_CAMERA_UP flags rather than walls, so not blocking on them is right — and where they're combined with bit 4 the mask still blocks, which is what you want. Might be worth naming those bits in the comment on SightBlockingAttributes.

Smaller notes

• AttributeMap is exposed as a public mutable byte[,], so a direct write silently desyncs WalkMap/AIgrid/SafezoneMap. Same shape as the existing WalkMap, so not a new sin — but now that all in-tree writers go through ApplyTerrainAttribute, consider whether UpdateAiGridValue still needs to be public.
• IsWalkableValue could be written as (value & ~(byte)TerrainAttributeType.Safezone) == 0, which reads closer to the new bitmask model than value == 0 || value == 1.
• The two branches of ApplyTerrainAttribute duplicate the set/clear expression; hoisting it above the safezone check would shorten the method.
• IsCurrentTargetValid now drops the target the moment it leaves sight. That's fine for repathing, but it also means a player attacking a monster from behind a wall (player skills stay lenient, as you note) causes the monster to re-search every tick. Worth a quick look in-game that it doesn't produce visible target-flapping.

---

Generated by Claude Code

[sven-n]

Re-reviewed at 5a9019a. All four points from the last round are addressed, and CI is green.

First, a correction I owe you: the Blood Castle regression I reported in my last review was wrong. I looked up Terrain{mapNumber}.att, but TerrainUpdateHelper resolves map N to Terrain{N+1}.att — map numbers are 0-based, terrain files 1-based. Blood Castle 1 loads Terrain12.att (not Terrain11.att) and BC8 loads Terrain53.att; in both, the bridge tiles really are NoGround, exactly as configured. Your original per-bit removal would have worked. Details and what I'd suggest doing about the current version are in the inline comment on ApplyTerrainAttribute — short version: what you have now is also fine (it matches pre-PR master semantics), so nothing blocks the PR either way. Sorry for sending you down that path.

One thing still open: HasLineOfSight remains asymmetric, now because of Bresenham's tie-breaking rather than the corner rule. Minimal repro is a single wall at (10,11) with (10,10) ↔ (11,12) — true one way, false the other. Brute force over random layouts puts it at ~3.9% of pairs; a four-line endpoint normalization takes it to exactly 0. No production call site evaluates a pair both ways today, so nothing is visibly broken, but the tests do assert symmetry in their comments. Inline on the Bresenham setup.

What's fixed well:

• Diagonal corner pocket — checking the corner before the endpoint return is the right ordering, and the explicit melee-adjacency short-circuit at the top is a cleaner way to express "adjacent is always visible" than relying on return order. Good that the corner test now asserts both directions.
• Trap latching — adding the sight check to the invalidation condition is exactly right, and the comment explaining why a trap needs this when a monster doesn't (it can't walk around) is the kind of thing that stops someone "simplifying" it back out later.
• NpcTargetSelection.GetNearestPreferVisible — nice extraction. Passing getDistance as a delegate rather than assuming distance-to-viewer is what lets the summon keep its owner-centered radius while checking sight from itself, and the comment there now makes that split explicit instead of looking like a bug.
• Small notes — UpdateAiGridValue going private, the bitmask form of IsWalkableValue, and documenting the high .att bits all landed.

Nothing else from me. With the symmetry point resolved (or consciously deferred), this looks good to go from my side.

---

Generated by Claude Code

[sven-n]

Re-reviewed at d5fbefc. Both open points are resolved, CI is green, and I have nothing further. I've resolved all six threads.

Endpoint normalization — verified. I re-ran the brute force against the exact code as it now stands (including the melee short-circuit and the corner-before-endpoint ordering): 0 asymmetric pairs out of 7.6M ordered pairs over random wall layouts, against 3.9% before. All the existing sight cases still behave as their tests describe — clear lines, wall-between in both directions, endpoint exemptions, closed corner in both directions, single-wall corner visible, diagonal-adjacent visible. Placing the swap after the same-tile and melee checks is right: those are direction-independent anyway, so they don't need it, and the swap can't affect them. HasLineOfSightTieBreakingIsSymmetric pins the exact case down.

Revert of the remove-path — agreed, and I re-verified the whole caller surface on this branch rather than trusting the earlier analysis. There are exactly five ApplyTerrainAttribute call sites, and only three can pass false:

• MiniGameContext — the only configured non-safezone removals in the codebase are Blood Castle's five areas, and every one matches the bits in the file actually loaded (Terrain12.att / Terrain53.att): bridge NoGround on 8, the other four Blocked on 4. Chaos Castle only sets NoGround and toggles Safezone, so it takes the other branches.
• KanturuContext — removes NoGround from the barrier column, which is uniformly 8 in Terrain39.att (map 38).
• CastleSiegeNpcController.ReleaseGateTerrain — this is the one the revert actually improves. With the per-bit clear the close/open cycle round-trips exactly: a gate tile at 20 (Blocked|Water) has WasBlocked == true and comes back as 20, and a hypothetical 16 (water, no wall) closes to 20 and reopens to 16 instead of becoming walkable. Matches master's "restore prior state" intent.

ApplyTerrainAttributeRemoveKeepsOtherBits is a good addition — it's the test that would have caught the round-trip problem, and it documents the contract the castle gates depend on.

The rest stands as reviewed: the corner-pocket ordering, the trap invalidation, and NpcTargetSelection.GetNearestPreferVisible all look right, and the comments explaining why each one is shaped that way should survive future refactors.

Good to merge from my side.

---

Generated by Claude Code