Skip to content

fix: mmap tile coordinates, navmesh ground resolution, chase floor clamping, and character-select display IDs#2

Open
HKevinH wants to merge 38 commits into
FirelandsProject:masterfrom
HKevinH:master
Open

fix: mmap tile coordinates, navmesh ground resolution, chase floor clamping, and character-select display IDs#2
HKevinH wants to merge 38 commits into
FirelandsProject:masterfrom
HKevinH:master

Conversation

@HKevinH

@HKevinH HKevinH commented May 30, 2026

Copy link
Copy Markdown

Overview

This PR consolidates a series of fixes addressing critical issues across the navigation, movement, and character visualization systems.

The changes resolve long-standing problems in mmap generation, navmesh height queries, creature chase behavior, and character equipment rendering on the character selection screen. Together, these fixes restore reliable pathfinding, prevent creatures from drifting or flying during pursuit, improve protection against corrupted navigation data, and ensure equipment visuals are displayed correctly before entering the world.


Key Improvements

Corrected mmap Tile Placement

The mmap generator was using an incorrect coordinate convention when converting ADT tiles into navigation meshes. Although the correct map files were generated, the resulting navmesh tiles were being placed into the wrong world-space positions.

This caused widespread failures such as:

  • .mmap path returning "start not found"
  • Players being unable to locate nearby polygons
  • Navigation working only near specific map regions

The generator now follows the actual World of Warcraft ADT coordinate convention, ensuring generated tiles are loaded into the proper navmesh slots.

Impact

  • Navigation meshes load in the correct location.
  • Pathfinding works consistently across the entire continent.
  • Existing invalid mmaps must be regenerated.

Increased NavMesh Polygon Capacity

Dense terrain tiles frequently exceeded the available polygon budget configured for Detour.

Previous configuration:

maxTiles = 4096
maxPolys = 1024

New configuration:

maxTiles = 1024
maxPolys = 4096

This rebalances Detour's bit allocation while maintaining support for continent-scale navigation meshes.

Impact

  • Eliminates failures such as:
MMAP tile add failed: status=0x80000008
  • Dense terrain tiles now build and load successfully.
  • Improved stability during navmesh generation.

Reworked NavMesh Ground Height Resolution

The previous implementation relied on findNearestPoly() to determine ground height.

This approach is problematic because Detour selects the nearest polygon in three-dimensional space rather than the floor directly beneath the query position.

For airborne callers this often resulted in:

  • Cliff tops being selected instead of ground.
  • Incorrect Z values.
  • Height drift during chase movement.

The implementation has been replaced with a true vertical ground query using:

  • queryPolygons()
  • getPolyHeight()

The algorithm now:

  1. Searches all polygons beneath the query column.
  2. Selects the highest floor below the caller.
  3. Falls back to the lowest floor above if necessary.
  4. Uses tight horizontal bounds and large vertical bounds.

Impact

  • Ground height now reflects the actual floor under the position.
  • Height calculations remain stable for flying players and elevated callers.
  • Chase movement receives reliable terrain information.

Fixed Airborne Creature Chase Behavior

One of the most visible issues was creatures slowly rising into the air while pursuing a target.

The root cause was a feedback loop:

  1. A bad navmesh height was selected.
  2. Chase movement snapped the creature to that height.
  3. The next update used the new position as input.
  4. Height errors accumulated over time.

Creatures could eventually end up hovering above terrain or sitting on distant cliff peaks.

The chase system now projects movement targets onto the navmesh floor before path calculations occur.

Additional ground clamping ensures the final broadcast position always remains on the terrain surface.

Impact

  • Creatures remain grounded while chasing.
  • Flying players no longer cause NPCs to climb into the air.
  • Chase movement behaves consistently across uneven terrain.

Added Protection Against Corrupted NavMesh Data

Even with improved height queries, corrupted .mmtile files can still contain invalid polygons at incorrect elevations.

To prevent these errors from affecting gameplay, two safety mechanisms were introduced.

Target Projection Guard

Before using a navmesh-derived ground position:

  • If the projected floor differs from the creature's current height by more than 15 yards,
  • The projection is considered invalid and rejected.

Post-Step Ground Guard

After movement is calculated:

  • If the projected ground differs from the creature's current height by more than 5 yards,
  • The adjustment is ignored.

Both cases generate warning logs to aid diagnostics.

Impact

  • Prevents vertical teleports.
  • Prevents snapping onto ghost polygons.
  • Makes corrupted tiles immediately visible through server logs.

Fixed Incorrect Chase Waypoints

Detour's findStraightPath() may occasionally return an initial waypoint projected onto an invalid or distant polygon.

In those cases creatures would:

  1. Begin moving toward the incorrect waypoint.
  2. Temporarily run away from the target.
  3. Correct themselves only after subsequent updates.

A new validation step compares:

  • Direction to waypoint
  • Direction to target

Waypoints are discarded when they:

  • Move away from the target, and
  • Increase distance to the target.

Impact

  • Creatures immediately move toward their target.
  • Eliminates incorrect first-step movement caused by ghost polygons.
  • Preserves valid detours around obstacles.

Added Chase Debug Instrumentation

To simplify future debugging, additional chase diagnostics have been added.

New log markers include:

CHASE ground-project target
CHASE ground-clamp step
CHASE broadcast
CHASE finalize stand

These logs make it possible to reconstruct chase behavior from logs alone.

Impact

  • Easier troubleshooting.
  • Faster identification of navigation regressions.
  • Improved visibility into height-resolution decisions.

Fixed Character Selection Equipment Rendering

Characters appearing naked on the character selection screen were traced to incomplete item_template records.

Previously:

  • A database row with displayid = 0 was treated as authoritative.
  • DBC and DB2 fallbacks were skipped.
  • SMSG_CHAR_ENUM transmitted a display ID of zero.

Although equipment appeared correctly in-game, the selection screen relied on resolved display IDs and therefore rendered no equipment visuals.

The resolution logic now:

  1. Reads the SQL row.
  2. Detects displayid == 0.
  3. Falls back to:
    • Item.db2
    • CharStartOutfit.dbc
  4. Preserves inventory metadata from SQL.
  5. Uses DBC display information when available.

Impact

  • Equipment is displayed correctly on the character selection screen.
  • No database migration is required.
  • Existing characters are automatically fixed.

Files Modified

Navigation Mesh Generation

  • tools/vmap/mmap_generator/MmapGenerator.cpp

Changes:

  • Correct ADT tile coordinate conversion.
  • Correct V9 terrain orientation.
  • Rewound triangle winding order.

Navigation Runtime

  • DetourNavMeshManager.h
  • DetourNavMeshManager.cpp

Changes:

  • Added GetNavMeshHeight().
  • Increased polygon capacity.
  • Implemented vertical floor resolution.

Collision Queries

  • MapCollisionQueriesReal.cpp

Changes:

  • Ground height now prioritizes navmesh data before VMAP fallback.

Combat and Movement

  • WorldSessionCombat.cpp
  • CreatureChaseMovement.cpp

Changes:

  • Ground projection.
  • Chase floor clamping.
  • Safety guards.
  • Debug logging.
  • Wrong-direction waypoint filtering.

Persistence Layer

  • MySqlCharacterRepository.cpp

Changes:

  • DBC/DB2 fallback for display ID resolution.

Unit Tests

  • CreatureChaseMovementTest.cpp

Changes:

  • Added missing mock implementations required by IMapCollisionQueries.

Validation

After regenerating mmaps:

Navigation

  • .mmap reports valid navmesh status.
  • Terrain height matches expected ground elevation.
  • .mmap path generates valid routes.

Movement

  • Creatures remain on the terrain surface.
  • Flying players no longer cause NPCs to gain altitude.
  • No unexpected vertical snaps occur.

Character Selection

  • Existing characters display equipped gear correctly.
  • No more naked character models on the selection screen.

Migration Requirements

1. Delete Existing mmaps

Old mmaps were generated using incorrect world coordinates and must be removed.

Example:

rm mmaps/*.mmtile

2. Regenerate mmaps

firelands-mmap-generator -m all -i maps -o mmaps

or for a specific map:

firelands-mmap-generator -m 1 -i maps -o mmaps

3. Restart World Server

No database schema changes are required.


Result

image image

This PR restores reliable navigation and movement behavior by correcting mmap generation, improving ground-height resolution, hardening chase movement against invalid navigation data, and fixing equipment rendering on the character selection screen.

The result is a significantly more stable and production-ready navigation stack, while preserving backward compatibility and requiring no database migrations.

HKevinH and others added 30 commits May 29, 2026 21:16
…queries

- Added DetourNavMeshManager for managing navigation meshes.
- Introduced MapCollisionQueriesReal to handle real map collision queries using DetourNavMeshManager.
- Updated WorldSession to include navMeshState for creature chase movement.
- Enhanced WorldSessionCombat to utilize navMesh for creature movement.
- Created MmapGenerator for generating navigation mesh tiles from .map data.
- Updated configuration to include collision data root for pathfinding.
- Added tests for new collision query implementations and creature chase movement.
- Introduced mmap_generator tool for generating navmesh tiles.
…dling

- Introduced VMapManager2 class to manage virtual map loading and collision detection.
- Implemented WorldModelRuntime to handle the reading and processing of world model data.
- Added support for loading VMAP files and managing their associated models and spawns.
- Created necessary data structures such as LoadedTile and LoadedMap for efficient map management.
- Implemented ray intersection and height retrieval functionalities for collision detection.
- Added unit tests for VMapManager2 and WorldModelRuntime to ensure correctness of the new features.
- Updated CMakeLists.txt to include new test files for VMapManager2.
…re chase

- MmapGenerator: TileOriginX/Y now follow the WoW ADT convention so the
  generated tiles land at the right navmesh slot ((31 - row)*TS for X,
  (31 - col)*TS for Y), with V9 sampled from its NW origin and triangle
  winding rebuilt to keep normals pointing up.
- DetourNavMeshManager: drop maxTiles to 1024 (tileBits=10) so maxPolys
  can rise to 4096 (polyBits=12), eliminating DT_INVALID_PARAM addTile
  failures on dense terrain.
- DetourNavMeshManager: new GetNavMeshHeight that queries findNearestPoly
  with a wide vertical extent so a caller above the floor still resolves
  the ground polygon below. MapCollisionQueriesReal::GetHeight prefers
  it over the vmap fallback.
- WorldSessionCombat: project the chase targetZ to the navmesh ground
  before the relocation check and after the step, so creatures stay on
  the floor when the player flies up.
- CreatureChaseMovementTest: implement the remaining IMapCollisionQueries
  overrides on the mock so the suite builds again.
Adds four MMAP_DEBUG breadcrumbs in TryBroadcastCreatureSplineStep and
TryFinalizeCreatureChaseStand:
- ground-project target: raw vs. grounded targetZ, plus from.
- ground-clamp step: pre/post clamp on projected.position.z.
- broadcast: from/to that the client actually receives.
- finalize stand: from, raw target, computed stand position.

Lets us see exactly which tick (and which value) is pulling the creature
up when the player is airborne.
GetNavMeshHeight used findNearestPoly with a 100-yard horizontal and
1000-yard vertical extent. With a high zHint (creature drifting after
chasing a flying player), the 3D-nearest poly was a nearby cliff peak,
not the floor below. The chase clamp then snapped the creature to the
peak, the next tick re-projected with the new (higher) zHint, and the
creature climbed the cliff every tick until it sat on top.

Switch to queryPolygons with a tight horizontal half-extent (1.5y) so
only polys whose XY footprint covers the query column are considered,
then read each candidate's true surface height with getPolyHeight. We
pick the highest floor at or below the query Y (with 2y of slop). If
nothing sits below, fall back to the lowest poly above so the caller
still gets a sensible value.
…tically

Logs from a Razor Hill test showed targetZ_grounded jumping to ~85y on a
flat area where the real terrain is ~15y. The .mmtile data is genuinely
corrupted (no ground poly at the creature's actual XY, only a ghost poly
~70y above). The clamp then reinforced the bad value tick after tick.

Add two guards inside TryBroadcastCreatureSplineStep:

- target projection: if navmesh ground is more than 15y from creature's
  current Z, ignore it and keep targetZ at from.z.
- post-step clamp: if navmesh ground is more than 5y from from.z, keep
  the creature at from.z instead of snapping onto the bad poly.

Each rejection logs a WARN so the bad .mmtile coordinates are visible
during regen/debug. This is a safety net while the underlying terrain
data is being fixed -- once the .map extraction produces correct V9s
and mmaps are regenerated, these guards should never trip.
…ero row

SMSG_CHAR_ENUM showed characters naked even when they were clothed once
in-world. Root cause: FetchItemProto returned immediately on any
item_template hit, including rows where displayid was 0. The char enum
ships the resolved displayId straight to the client, so a zero means
the slot renders empty; the world stays clothed because PLAYER_VISIBLE_
ITEM_*_ENTRYID carries the raw entry and the client resolves visuals
from ItemSparse.db2 locally.

Treat a SQL row with displayId == 0 as incomplete and fall through to
the ItemDb2 / CharStartOutfit DBC lookup, then keep the SQL row's
inventoryType / buyCount but override displayId from the fallback. If
both fallbacks also fail, return the (incomplete) SQL row so existing
behavior is preserved for genuinely zero-display items.
After fixing the char-enum and ground-clamp, the user reported the NPC
walking correctly on the ground but heading *away* from the player on
the first step. Cause: Detour's findStraightPath returns a start
projection on the nearest poly, which on corrupted/ghost mmtiles can
land yards away from the creature's actual XY. The chase then takes
its first step toward that ghost point and never reaches the player.

Add a guard inside StepCreatureAlongNavMeshPath: a waypoint is skipped
when the vector from current to wp points away from the target (dot < 0)
*and* the wp is farther from the target than current. That catches the
ghost-projection case without breaking legitimate detours (those have
a positive component toward the target). Falls through to the next
waypoint, ultimately reaching the appended end-target waypoint.
Reports came in that .mmap path showed no visual markers even after
the path resolved with status=Complete. Three things were stacking up:

- The marker spawn log was MMAP_TRACE and got filtered, so there was
  no way to confirm where the markers actually landed.
- The visual lift was wp.z + 1.0y, which puts the marker inside any
  player or creature model (humans are ~2y tall). When the path
  collapses to a single waypoint (start ~= end), the lone marker
  spawned inside the player and looked like 'nothing rendered'.
- The waypoint Z came straight from Detour's corridor. On tiles with
  the ghost-poly corruption we already guard against in the chase
  loop, that Z is tens of yards above the real ground, so the marker
  spawned in mid-air far above the camera frame.

Re-ground each waypoint through GetHeight() before placing, lift the
marker 3y above that, promote the spawn line to MMAP_DEBUG, and emit a
client notification when the result collapsed to zero markers so a
trivial path is obviously distinct from a missing marker.
…ayer

Reported behaviour: the creature walks toward the player, turns
around, replans, then walks back. The cause was the relocation
threshold of 0.5y: any normal player movement triggered a full
replan, state.waypoints was cleared, and currentWaypoint reset to 0
every tick. On tiles with ghost polys, findNearestPoly alternated
between two bad start projections each tick, so the corridor's wp[0]
flipped direction and the NPC oscillated.

Two changes:

- Raise the replan threshold to 3y. The path now commits across
  several ticks of walking, so a transient ghost projection does not
  flip the corridor mid-tick.
- Short-range bypass: when the target is within 8y 2D, skip the
  navmesh corridor entirely and chase in a straight line. This is the
  case least likely to need a real detour and most likely to be hurt
  by a ghost start projection. Clears any stale waypoints so the
  state stays consistent.
The startup log shipped ~3000 "MMAP tile missing" lines because the
manager probes all 64x64 tile slots on every map load. The vast
majority of those slots are ocean/void in WoW: a continent has on the
order of 900 valid ADTs, not 4096. The real generator failures got
buried under the noise.

Now the manager checks first whether maps/<mapId><ty><tx>.map exists:

- If the .map is absent, the tile is silently skipped (genuinely
  off-continent, never expected to have an mmtile).
- If the .map is present but the .mmtile is not, log MMAP_WARN
  pointing at the missing file. That is a real generator regression.
- After the load loop, emit one MMAP_INFO summary: loaded vs
  expected vs missing, so the operator can see at a glance whether a
  regeneration succeeded fully or partially.
@seobryn

seobryn commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Looks great but with some comments:

  1. Please update the RBAC stuff, we already update that part to remove the access level.
  2. Please check the flying creatures and test if this code works with them, because I'm checking the code and I see some parts that force Clamp to ground without checking if the creature is flying or walking and I think we want to be sure if the code also Works with this creatures.

@HKevinH

HKevinH commented Jun 2, 2026

Copy link
Copy Markdown
Author

Looks great but with some comments:

  1. Please update the RBAC stuff, we already update that part to remove the access level.
  2. Please check the flying creatures and test if this code works with them, because I'm checking the code and I see some parts that force Clamp to ground without checking if the creature is flying or walking and I think we want to be sure if the code also Works with this creatures.

That's it—it's all settled.

('faction', 'Change faction reaction', '.faction <factionId> <reaction>', 128),

-- GM Tickets (mask=256 = ManageGmTickets)
('ticket', 'Manage GM tickets', '.ticket list|close|respond', 256),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The command .ticket ui is missing

('dev', 'Toggle Developer tag', '.dev [on|off]', 64),
('visible', 'Toggle GM visibility to players', '.visible [on|off]', 64),
('fly', 'Toggle fly mode', '.fly [on|off]', 64),
('speed', 'Set movement speed multiplier', '.speed <value> (1=normal)', 64),

@seobryn seobryn Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Incorrect description, this command works as follow:

.speed [number] to modify the speed
.speed reset to reset the default speed

Default speed = 7

This command modifies the walk speed and also the Fly Speed.

('additem', 'Add item to inventory', '.additem <itemId> [count]', 128),
('delitem', 'Delete item from inventory', '.delitem <itemId> [count]', 128),
('level', 'Set character level', '.level <value>', 128),
('cd', 'Reset all spell cooldowns', '.cd', 128),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Include also the racials CD's

('cd', 'Reset all spell cooldowns', '.cd', 128),
('damage', 'Deal damage to targeted creature', '.damage <amount>', 128),
('revive', 'Revive yourself or targeted player', '.revive', 128),
('faction', 'Change faction reaction', '.faction <factionId> <reaction>', 128),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DO you check that's working?


static constexpr uint64_t kMaxRestartDelaySeconds = 7ULL * 24 * 3600;

static char const *FindPathStatusName(FindPathStatus status) {

@seobryn seobryn Jun 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this code Is stored in the Command service?

COmmand service should store and handle commands only not Logic

}
}

void CommandService::RegisterCommand(const std::string &name, CommandEntry entry) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you dont want to use this function anymore in your structure, please remove it

return true;
}

bool CommandService::HandleMmap(std::shared_ptr<ICommandSession> session,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This logic should be placed in another place

@seobryn seobryn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some comments before merging

…, ticket

Update syntax/description rows to match actual handlers: .speed takes a number or reset (default 7, affects run+fly), .cd resets racials too, .faction uses forced/template subcommands, and .ticket lists queue|mine|ui|take|reply|close.
@HKevinH HKevinH requested a review from seobryn June 5, 2026 01:17
HKevinH added 2 commits June 4, 2026 22:20
Move the .mmap GM command (navmesh queries + visual path markers) and its marker state out of CommandService into a dedicated MmapDebugCommands class. Share JoinArgs/AsciiEqualsLower/IsAllDigitAscii via CommandTextUtils.h, relocate FindPathStatusName next to its enum, and drop the unused RegisterCommand.
The shared Logger unconditionally created an <name>-mmaps.log (deriving a default path when none was set), so the auth server produced a stray mmaps log. Create the mmap sink only when WithMmapFile() sets a path; MmapXxx() falls back to the main logger when absent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants