New terrain engine + extractor - #78
Conversation
One terrain engine, in src/shared/terrain. FusedTerrain answers with a column of surfaces instead of a single height, so terrain, WMO floors, liquid and dynamic game objects all come back from one query. DynamicCollision puts doors and lifts in that same column. One extractor replaces the three separate tools. mangos-extractor bakes DBCs, game-object models, terrain tiles, vessel decks and the navmesh, in that order, each from the output of the last. The navmesh is built from the baked tiles, so the pathfinder walks the surface collision actually answers with. Client support is 5.4.8. MCNK offset 0x14 is read as holes_high_res, not as an offset. Hole maps are 64-bit per chunk. MCVT is located by scanning the sub-chunks. MH2O resolves through the LiquidObject chain, and object 42 is depth-only ocean. The archive set is the Mists one, with all eighteen update builds. The update archives now patch each other, not only the bases. Without that, 1374 files across 29 maps resolved to pre-release builds. Three Recast accuracy values from #250 are restored. On slope, navmesh points more than a yard off the floor fall from 40.5% to 10.9%. mangos-height-check scores a bake against a position corpus, separating liquid, model floors and unsettled probes rather than averaging them in. 107 tests cover the parsers, the tile format and the collision model. Removed: src/game/vmap, src/tools/Extractor_projects, src/tools/Extractor_Binaries and the GridMap class. Measured on a 5.4.8 client: 9724 tiles over 288 maps. Of 2164 recorded client positions, 98.70% land within 2 yards of the baked floor, 99.14% on dry ground. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 35 medium |
| Complexity | 36 medium 29 minor |
🟢 Metrics 1996 complexity · 70 duplication
Metric Results Complexity 1996 Duplication 70
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7d869d411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return m_TerrainData->IsInLineOfSight(srcX, srcY, srcZ, destX, destY, destZ, ignore) | ||
| && m_dyn_tree.IsInLineOfSight(srcX, srcY, srcZ, destX, destY, destZ, phasemask); |
There was a problem hiding this comment.
When a production database has a DISABLE_TYPE_VMAP row with COLLISION_DISABLE_LOS for this map, this unconditional static query ignores it and starts blocking sight and spell casts again. The commit still loads these rows and preserves DisableMgr::IsVMAPDisabledFor, while the previous VMap path returned clear LOS when that flag was set, so the new terrain entry points need to apply the corresponding per-map disable before querying collision.
Useful? React with 👍 / 👎.
| { | ||
| continue; | ||
| } | ||
| BakeMap(source, entry.first, entry.second, tileDir); |
There was a problem hiding this comment.
If an ADT cannot be parsed or WriteTile fails, BakeMap only records/logs the failure and this loop continues without retaining an error result; navigation then enumerates only the tiles that were successfully written, so it can also succeed and main returns 0. This is especially destructive through linux/getmangos.sh, which deletes the installed data first and treats that zero exit status as a complete extraction, leaving users with missing terrain tiles.
Useful? React with 👍 / 👎.
| if (std::ifstream(g_tileDir + "/" + TileFileName(mapId, tx, ty), | ||
| std::ios::binary).good()) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
When a starting-area tile exists but is truncated or was produced with another tile version, this check returns true and lets MapManager::ExistMapAndVMap pass startup; the later ReadTile rejects the magic/version and returns null, so height, liquid, and collision queries silently run with no data. The previous existence check validated its map header, so this should likewise verify that the tile is readable and compatible rather than checking only that the path opens.
Useful? React with 👍 / 👎.
| // Pins the cell's tile against the cache sweep for as long as a grid stands on it. | ||
| // The tile data itself still loads lazily, on the first query that reaches it. | ||
| m_terrain.PinCell(int(x), int(y)); |
There was a problem hiding this comment.
When multiple map instances load the same grid, this increments the terrain pin count once per reference, but Unload calls UnpinCell only when the shared grid reference count reaches zero. After N references are released the tile therefore remains pinned N-1 times, preventing the five-minute cache sweep from ever evicting it and causing resident terrain memory to accumulate as instances are created and destroyed; either pin only on firstReference or unpin on every matching unload.
Useful? React with 👍 / 👎.
| { | ||
| merged->areas[i] = NAV_GROUND; | ||
| } | ||
| merged->flags[i] = merged->areas[i] ? 1 : 0; |
There was a problem hiding this comment.
When Detour evaluates a unit's includeFlags, it tests polyFlags, not the separate area id, but this assigns flag NAV_GROUND (1) to every nonempty polygon regardless of whether its area is water, magma, or slime. Consequently a swim-only creature whose filter excludes ground cannot use water polygons at all, while players and walkers that include ground are allowed across every liquid type; retain the polygon's NAV_* bit in merged->flags[i] as the previous generator did.
Useful? React with 👍 / 👎.
| const long id = std::strtol(token.c_str(), nullptr, 10); | ||
| if (id > 0) | ||
| { | ||
| ids.push_back(uint32(id)); |
There was a problem hiding this comment.
When LoadAllGridsOnMaps contains 0, this predicate silently drops it even though map 0 is the valid Eastern Kingdoms map and the previous parser inserted that value. Administrators therefore can no longer force-load map 0 with the documented comma-separated setting; reject malformed or negative tokens without treating zero as invalid.
Useful? React with 👍 / 👎.
| out.liquidShow[idx] = 1; | ||
| out.liquidEntry[idx] = entry; | ||
| out.liquidDark[idx] = 0; | ||
| out.liquidDeepAttr[idx] = (deepBits != 0) ? 1 : 0; |
There was a problem hiding this comment.
When an MH2O chunk has a partially populated deep attribute bitmap, this marks every visible liquid cell as deep merely because any bit is set. The baked tile then adds MAP_LIQUID_TYPE_DARK_WATER across the whole chunk, starting the fatigue timer in shallow cells near the actual deep-water boundary; select the bit corresponding to the current 8x8 chunk cell instead of testing the bitmap as a boolean.
Useful? React with 👍 / 👎.
| const Aabb& wb = inst.worldBounds; | ||
| if (!wb.coversColumn(x, y) || wb.hi.z < ceiling - MAX_DROP || | ||
| wb.lo.z > ceiling + 0.1f) |
There was a problem hiding this comment.
When a player is flying above a WMO whose XY footprint is below them, this accepts any instance up to 300 yards beneath the query and raycasts down into its roof or floor even though the point is outside the model's vertical bounds. If the underlying terrain is below the WMO floor, the later terrain guard does not cancel the hit, so IsOutdoors can classify the player as indoors and incorrectly reject outdoors-only spells; area lookup should exclude a WMO whose upper bound is below the query point.
Useful? React with 👍 / 👎.
| LocalLiquid out; | ||
| out.z = z; | ||
| out.entry = lq.entry; | ||
| out.kind = lq.kind; | ||
| return out; |
There was a problem hiding this comment.
When two liquid-bearing WMO groups overlap in local X/Y at different elevations, this returns the first group in serialized order and discards the other surface without considering the query Z. FusedTerrain::ColumnAt therefore cannot add both surfaces or select the one belonging to the player's level, so a lower room beneath an upper pool can be reported as underwater in the wrong liquid, while a pool in a later group can disappear entirely; return all matching group surfaces or select the enclosing group using the vertical position.
Useful? React with 👍 / 👎.
Every finding from the PR review, plus what auditing around them turned up. The nine: - The `disables` table stopped reaching collision entirely. IsVMAPDisabledFor had no callers left, so a production row disabling LOS, height, areaflag or liquid status on a map did nothing at all. Answered in TerrainInfo, which is where the one gather feeding every height, floor and liquid query lives. - The extractor exited 0 after a partial bake. getmangos.sh deletes the installed data BEFORE running it, so that reported a world with holes in it as a complete extraction, with nothing left to restore. The tile failures now reach the exit code, and nav is not built over a set known to be incomplete. - HasTile opened the file instead of reading it. A truncated or stale-format tile passed the startup probe and then answered every query with nothing. - The terrain pin was taken per reference and released on the last one, so a cell touched by N map instances stayed pinned N-1 times and could never be swept. Both it and the navmesh tile are now taken by the first referent. - Nav polygon flags collapsed to 1. dtQueryFilter tests flags, never the area, so that stamped NAV_GROUND onto water, magma and slime: swim-only creatures could not enter their own water and land creatures walked across lava. The comment claiming the filter reads the area went with it. - ParseIdList rejected 0, which is Eastern Kingdoms, making LoadAllGridsOnMaps=0 a no-op. It now rejects what is actually wrong -- a token that is not a number. - The MH2O deep attribute was broadcast over the whole chunk whenever any bit was set, starting the fatigue timer in the shallows beside real deep water. Indexed per cell, over the chunk's own 8x8, offset by the instance rectangle. - The WMO area lookup accepted any model up to 300 yards below the query, so flying over a building reported the player as indoors and outdoors-only spells were refused. The point must be inside the box vertically, as it was in vmap. - WMO liquid answered with the first group in serialized order. A pool above a flooded room reported the wrong water in the wrong room and made the other surface disappear. The enclosing surface wins now. Found while auditing them: - WmoModel::LiquidLocal indexes the corner grid at a stride of tilesX+1, and nothing ever checked the height count against the dimensions -- an MLIQ whose vertex grid is not the tile grid's corners, or a corrupt tile, reads off the end of the vector on the first liquid query. Guarded at both ends. - ReadMcnk read its 128-byte header without checking the record was that long; the caller only proves the declared size fits the file. - vmap.enableLOS and vmap.enableHeight were still shipped in the dist config though the code says they are "gone rather than ignored". - The 2.0f no-hit sentinel is now NO_HIT_FRACTION rather than a literal in four places across three files. Three tests, each red before its fix. The existing deep-attribute tests all used an all-ones or all-zeros mask, which is exactly why a chunk-wide broadcast passed them. The extractor path normalisation and the menu's stale-dest check are picked from the transports branch, where they were found; neither has anything to do with vessels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Work that was done on transports but has nothing to do with vessels, so it belongs here rather than behind that system. Picked from de6f02e, 5f985db, 5012343, 0fc21f1 and d219600. Crashes and wrong behaviour: - SERVER_SIDE_SPELL is a static array, and the storage field it was pointed at is delete[]'d by SQLStorageBase::Free(). Freeing static memory, on logout. - Player::SetGameMaster tested `m_ExtraFlags |= PLAYER_EXTRA_GM_ON`. The assignment made the condition always true, which happened to be the right answer -- the flag is set unconditionally a dozen lines up -- so it never showed a symptom. - BattlemasterList.dbc and LFGDungeons.dbc are the 3.3.5a spellings; the 5.4.8 client ships BattleMasterList and LfgDungeons, which the extractor writes verbatim. They resolved on NTFS and failed to load on every case-sensitive filesystem, which includes the one CI builds on. Database: - The SOAP thread queries LoginDatabase on itself for per-request auth and never registered with the client library. DbThreadGuard, which also replaces the mysql_thread_init/end pair in SqlDelayThread so the end hook cannot be skipped by an unexpected exit path. - MYSQL_OPT_RECONNECT is deprecated from client 8.0.34 and prints a warning of its own to stderr, past our logging, at every connect. - SqlOperation::Execute (takes the lock) split from ExecuteLocked (the work), so the connection mutex can be plain. It was recursive only because a transaction locked the connection and then every statement inside it locked it again -- and a recursive mutex admits every other accidental re-entry too, silently. - Missing <cassert>, <cstring>, <map>, <algorithm>, <sstream> and <stdexcept>. Build and install: - project() was declared twice, the second time without LANGUAGES, re-entering compiler detection and resetting what the first call established. - WITH_NET_TESTS was read by a guard and passed by CI but never declared as an option, and CMake takes an undeclared -D almost silently. - CONF_INSTALL_DIR was "../etc", and install(DESTINATION) resolves a relative path against CMAKE_INSTALL_PREFIX -- so the configs landed one level above the prefix, where several servers shared one etc. - Nothing recorded a runtime search path for Eluna's Lua, which installs to <prefix>/lib while the servers go to <prefix>/bin, so an installed mangosd only ever worked from the build tree. The version bump to 0.23.0 rides along from 5012343. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review the follow up commits only do they fix the issues raised? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d71c18e940
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ok) { g_console.Detail(msg); } else { g_console.Error(msg); } | ||
| return ok ? 0 : 1; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
When a WDT identifies a global-WMO map but source.Load() cannot load its WMO—for example because the referenced model is missing or malformed—this branch still returns zero. The extractor consequently reports success and proceeds to nav generation without the map's only terrain tile, so the follow-up does not fully fix the reported partial-bake success issue; this path must contribute a failure when wdt->hasGlobalWmo is set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in c207019.
BakeMap now reads wdt->hasGlobalWmo rather than inferring the answer from the tile it just failed to load — that inference was the bug, because "the load returned nothing" and "this map declares no global WMO" produced the same value. A map that declares one and cannot bake it returns 1; a map that declares none still returns 0, since Map.dbc lists identities that never had terrain.
Verified against a real 5.4.8 client, because my first attempt to verify it did not: I baked map 33 believing it was a global-WMO map, and it is an ordinary ADT grid, so the branch never executed and rc=0 told me nothing. Maps 34, 43, 48, 90 and 349 are the real thing — each writes w_<id>.tile and exits 0 — while 36, 47, 129, 189 and 209 carry no global WMO and correctly still exit 0, which is the false-failure case this change could have introduced.
| const bool candAbove = cand >= p.z; | ||
| if (candAbove != (cur >= p.z)) | ||
| { | ||
| return candAbove; | ||
| } | ||
| return std::fabs(cand - p.z) < std::fabs(cur - p.z); |
There was a problem hiding this comment.
For stacked liquids with the query between them, this preference always selects the upper surface: with pools at z=5 and z=40 and a player at z=20, candAbove chooses z=40 and getLiquidStatus() can report the player underwater even though that pool belongs to the floor above. The added test covers below both and above both but omits this case; determining the enclosing WMO group/floor is necessary rather than inferring it solely from liquid-height proximity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, and this is the sharper version of the original finding -- thank you for not letting the first fix stand.
Proximity was never containment, and preferring the surface at-or-above made the stacked case wrong in exactly the way you describe: pools at 5 and 40 with the query at 20 answered 40.
Fixed in c207019 by asking which group owns the floor beneath the point, via the same downward cast through m_triGroup that AreaInfo already uses, and taking that group's liquid. Nearest-surface survives only as a fallback for when nothing is under the point at all, and without the directional bias.
The test now covers z=20 between the two pools, which is precisely the case the previous fixture avoided by probing only below-both and above-both.
| #if !defined(MYSQL_VERSION_ID) || MYSQL_VERSION_ID < 80034 | ||
| mysql_options(mysqlInit, MYSQL_OPT_RECONNECT, "1"); | ||
| #endif |
There was a problem hiding this comment.
With MySQL client 8.0.34 or newer this removes the repository's only reconnect mechanism, while Execute, Query, and the periodic Ping() contain no explicit reconnection path. After wait_timeout, a database restart, or another transient disconnect, every affected connection therefore remains unusable until the server process restarts; avoiding deprecated silent transaction reconnects requires a replacement reconnect strategy rather than disabling recovery globally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct -- reverted in c207019.
I checked before reverting and you are right that there is no other recovery path: Database::Ping() runs SELECT 1 and discards the result, and Execute/Query log mysql_error and return false without inspecting CR_SERVER_GONE_ERROR. Removing the option on 8.0.34+ traded a stderr warning for connections that stay dead after a wait_timeout or a database restart.
MYSQL_OPT_RECONNECT is unconditional again, and the comment now records why it must stay and names handling CR_SERVER_GONE_ERROR as the way to silence the deprecation warning honestly, rather than by dropping recovery.
| if (ReadTile(g_tileDir + "/" + TileFileName(mapId, tx, ty))) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
A same-version malformed tile with nonempty but undersized v9, v8, or liquid vectors still makes ReadTile() return a non-null object because it checks that each declared vector fits in the remaining file, not that its cardinality matches the fixed grid dimensions. This probe therefore reports the tile as valid, after which TerrainHeight() and LiquidAt() index those vectors as full V9_SIDE/GRID_PER_TILE arrays and can read out of bounds; the semantic vector-size checks need to run before HasTile() accepts the result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in c207019.
RVec validates that a declared count fits in the remaining bytes, which is a truncation check, not a shape check -- the readers index these as fixed grids (TerrainHeight walks v9 at a stride of V9_SIDE and guards only against empty). ReadTile now requires each present grid to be exactly V9_SIDE * V9_SIDE or GRID_PER_TILE * GRID_PER_TILE, empty still being legal.
This is the same defect I had just fixed for the WMO liquid grid in the previous commit and failed to generalise to the tile's own arrays.
| if (mcnkSize < MCNK_HEADER) | ||
| { | ||
| return; |
There was a problem hiding this comment.
For an ADT containing a syntactically bounded but shorter-than-128-byte MCNK, this guard merely skips the chunk after ParseAdt() has already set sawMcnk; the parser then returns success with zero-initialized height arrays and BakeMap() writes and counts a flat tile as successfully extracted. The bounds check must propagate failure, or at least prevent the skipped chunk from satisfying hasTerrain, so corrupt input reaches the extractor's new nonzero exit path instead of silently producing bad terrain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in c207019.
The guard stopped the out-of-bounds read and left something worse behind: the height arrays are zero-filled before the chunk walk, so a skipped record is not a gap, it is 8x8 cells of flat ground at 0.0 — written, and counted as a successful map square by the very exit-code change it was sitting beside.
ReadMcnk returns bool now and a short header fails ParseAdt, so the tile is never written and the run exits non-zero.
Turning "skip a chunk" into "fail the parse" is only safe if retail data contains no record it rejects, which a fixture cannot establish, so it was run against a real 5.4.8 client: map 530 bakes 826 tiles at rc=0, plus nine instance maps, with no chunk rejected.
Codex's follow-up findings on the first pass, plus two failures CI caught that the first pass caused. The findings: - BakeMap returned 0 when a global-WMO map's only tile could not be loaded, so the partial-bake fix still exited 0 on exactly the map that had nothing. The WDT says whether a global WMO is declared (hasGlobalWmo); that, not the tile that failed to load, is what separates "could not bake it" from "there was none". - LiquidLocal preferred the surface at or above the query, which answers the stacked case backwards: pools at 5 and 40 with the player at 20 reported them swimming in the one upstairs. Proximity was never containment. It now asks which group owns the floor beneath the point, by the same downward cast AreaInfo uses, and only falls back to nearest -- without the directional bias -- when nothing is under it. - ReadTile checked that each vector FITS the file, never that it is the size every reader indexes it at. A same-version tile with a short v9 passed the startup probe and read off the end on the first height query. This is the identical defect fixed in the WMO liquid grid one commit ago and not generalised then. - The MCNK header guard skipped the chunk, and the height arrays are zero-filled before the walk -- so a short record became 8x8 cells of flat ground at 0.0, baked and counted as success. It fails the parse now. - MYSQL_OPT_RECONNECT is unconditional again. Nothing else here recovers a dropped connection, so removing it on client 8.0.34+ traded a stderr warning for every connection staying dead after a wait_timeout. The comment now says so, and names handling CR_SERVER_GONE_ERROR as the way to silence the warning honestly. What CI caught: - SqlOperation's Execute/ExecuteLocked split broke CountingOperation in the database tests, which override Execute. The previous commit claimed there were no subclasses outside src/shared/Database; the grep behind that claim had filtered out the tests subdirectory that contains one. MSVC, clang and gcc all rejected it. - The Docker build copied the config from /etc, because a relative CONF_INSTALL_DIR resolved against the prefix and installed it to <prefix>/../etc. Making that path absolute -- correct, and the point of the fix -- broke a COPY written around the old behaviour, which was redundant anyway since the whole prefix is already copied. Verified on both toolchains: MSVC 2022 and clang 19.1.7 (FreeBSD), 111 tests green on each. The z=20 case the new liquid test adds is the one the first fix got wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review the last commit and verify it fixes issues please. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c207019452
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the first query -- so a present grid must be exactly the size it is indexed at. | ||
| constexpr size_t CORNERS = size_t(V9_SIDE) * V9_SIDE; | ||
| constexpr size_t CELLS = size_t(GRID_PER_TILE) * GRID_PER_TILE; | ||
| auto sized = [](const auto& v, size_t n) { return v.empty() || v.size() == n; }; |
There was a problem hiding this comment.
Fresh evidence from this fix is the new sized() predicate: when a same-version tile declares hasTerrain or hasLiquid but stores a zero count for the corresponding grids, v.empty() still makes ReadTile() return a non-null tile. That lets FusedTerrain::HasTile() pass startup for a malformed cache, while later TerrainHeight()/LiquidAt() treat the empty vectors as no data and silently answer nothing; empty should only be accepted when the matching hasTerrain/hasLiquid flag is false.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
sized() accepted v.empty() unconditionally, so the shape check answered a different
question from the one it was written for: it proved a present grid has the right
cardinality, and said nothing about whether a grid the header CLAIMS is there is present
at all. A tile declaring hasTerrain with a zero count passed ReadTile, passed the
HasTile startup probe, and then answered every height query with nothing, which reads
as a map that is simply flat.
The predicate is now two-directional: a grid is present exactly when its flag says so,
and when present it is exactly the size the readers index it at.
auto grid = [](const auto& v, bool present, size_t n)
{
return v.size() == (present ? n : size_t(0));
};
The other direction matters as much and was not in the finding: a tile carrying a full
heightmap under a flag that says it has none is equally malformed, and every reader skips
on the flag alone, so it is equally silent.
Before tightening it I checked the writer really does guarantee both directions, since
getting this wrong rejects every tile at startup rather than a malformed one:
MpqTileSource::LoadAdt sets hasTerrain and moves v9/v8 together, sizes all five
liquid vectors from liquidShow.size() under if (tile->hasLiquid), and
LoadGlobalWmo returns a tile with every grid legitimately empty and every flag clear
except isGlobalWmo.
Test added: a tile written with hasTerrain and no grids, one with hasLiquid and no
liquid grids, and one carrying a heightmap with the flag cleared. All three round-trip
through WriteTile and must come back null.
Verified on FreeBSD 15 with clang 19.1.7: 116 tests, 0 failed.
| constexpr uint32_t MCLQ_BYTES = 8 + 81 * 8 + 64; | ||
| if (!offsMclq || sizeMclq <= 8 || offsMclq + 8 + MCLQ_BYTES > span) | ||
| { | ||
| return true; // no MCLQ is the 5.4.8 norm, and the heights are read |
There was a problem hiding this comment.
When a corrupt ADT has a full MCNK header but no complete MCVT subchunk (and, on 5.4.8 data, no legacy MCLQ), this success return still lets the parse continue after the height arrays were zero-filled. BakeMap() then sees hasTerrain and writes a flat tile with exit status 0 instead of tripping the new failure path, so the MCVT probe above needs to fail the MCNK when the height block is absent or too short.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and the guard you are pointing at was treating a symptom.
hasTerrain was set from sawMcnk -- that an MCNK TAG went past -- which is a statement
about the file's structure, not about whether any height was read. The heightmap is
zero-filled before the chunk walk, so everything that fails to fill a block leaves flat
ground at 0.0 rather than a gap, and all of it reached a written tile:
- a chunk with no MCVT, or one too short to hold 145 floats (your finding);
- a chunk absent from the file altogether;
- two chunks claiming the same IndexX/IndexY, which leaves a third index unwritten;
- a truncated file, which the chunk loop leaves by BREAKING, silently keeping whatever
it managed to read.
So MCVT is now required -- a chunk that cannot supply heights fails the parse -- and the
parse additionally requires that all 16x16 chunks did supply them. An ADT is a fixed grid
of 256 disjoint 9x9 blocks; anything less is not a tile with a hole in it, it is a tile
with a lie in it. ReadMcnk marks its own index and ParseAdt refuses unless every index
is marked.
Two things I checked before making MCVT mandatory, because on 5.4.8 this could have
rejected every split tile:
MpqTileSource::LoadAdtparses the root withAdtParts::Terrainand_obj0with
AdtParts::Objects. The objects pass haswantTerrain == falseand never enters the
MCNK branch, so the MCNK records in_obj0-- which legitimately carry MCRF/MCRD and
no MCVT -- are not affected. TheBothpath is only for pre-split files.hasTerrainis still only ever set, never cleared, so the objects pass cannot undo
what the terrain pass established.
Turning "skip the chunk" into "fail the parse" is only safe if retail data contains
nothing it rejects, and a fixture cannot establish that. So it was run against the real
5.4.8 client (build 18414 data, 46 archives), baking EVERY map in Map.dbc rather than a
sample:
220 maps: 187 ADT grids + 33 global WMOs
9,724 tiles written
rc=0, no chunk rejected, no tile reported FAILED or INCOMPLETE
So nothing in retail 5.4.8 trips either the mandatory MCVT or the all-256 requirement.
Both would have exited non-zero and refused to build nav if it had -- which is the point:
the failure is now loud instead of a flat square in the ground.
Two tests, both red before the fix: a chunk with a full 128-byte header and no MCVT, and
a tile of 255 chunks with one index missing.
A note on the existing fixtures, since it bit me: all 20 of them built a single MCNK and
asserted on the result, so they were asserting on something that was never a tile. They
now build a whole tile around the chunk under test. Getting that right took a second
pass -- V9 is 129x129 walked at a stride of 8, so adjacent chunks SHARE their border row
of corners, and padding emitted after the chunk under test flattened the very row two
tests were checking. Real neighbours store equal heights there and the order cannot
matter; flat padding does, so it is emitted first.
Verified on FreeBSD 15 with clang 19.1.7: 116 tests, 0 failed.
| // preferring the surface ABOVE, as this first did, gets that case wrong every time. | ||
| uint32_t tri = 0; | ||
| const Vec3 down{0.f, 0.f, -1.f}; | ||
| if (m_bvh.Raycast(m_soup, p, down, FLOOR_SEARCH, &tri) && tri < m_triGroup.size()) |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new room-selection ray starts from p.z, but the runtime caller passes the column top (getLiquidStatus() uses z + FLOOR_BURIED_LIFT, i.e. 50 yards above the player) as the liquid probe point. In stacked WMO rooms this can raycast from above an upper floor and select that group's pool for a player standing between floors, so the follow-up still reports the upstairs water as the current liquid; the containing-room check needs the player's/query Z, not the column sweep origin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this one is mine twice over -- the previous fix introduced it.
You are right that the ray starts from the wrong Z, and I checked the caller before
changing anything: getLiquidStatus asks for ColumnAt(x, y, z + FLOOR_BURIED_LIFT, z - FLOOR_SEARCH_DOWN) and FusedTerrain::ColumnAt then uses originWorld -- the top
of the sweep, 50 yards above the player -- as the liquid probe point. So the
containing-room cast ran from above the upper floor, exactly as you describe.
The comment sitting directly above that call had already stated the invariant I broke:
the height along the column "cannot change the pair those come out as". That was true
while LiquidLocal ignored Z, and my previous commit made it false without touching the
comment.
So the fix is not to pass a better Z down. A model cannot answer "which room" at all --
it is handed a point on the sweep column, not the queried position -- and Column.hpp
says so in its own header: one gather, many selections, so no caller is handed a
differently pre-selected answer. Putting a Z-dependent choice inside a Z-agnostic gather
was the actual defect; picking by nearest surface was only how it showed.
Fixed in the new commit:
LiquidLocalis nowLiquidsLocal, which APPENDS every group's surface and chooses
nothing. The downward cast and the nearest-surface fallback are both gone.- The choice moved to
Column::LiquidAt(z), which has the real query Z and already holds
the floors: everyRaycastAllhit enters the column as aStaticsurface. The rule is
the highest liquid surface reachable from the point without crossing a solid.
That also answers two cases nobody reported, both of which were HighestLiquid being
asked a containment question: a player in a cellar under a building was put in the lake
outside, and a player standing on a bridge was put in the river underneath it. The two
HeightCheck call sites asked the same question the same wrong way and moved with it.
GetTerrainType keeps HighestLiquid, since it is a 2D query with no point to contain.
Worth recording why the wrong fix went green: the stacked-rooms test built its groups
with an empty TriSoup, so there was no floor for the cast to hit and the assertion was
satisfied by the FALLBACK, never by the room logic it claimed to test. The replacement
tests the two halves separately -- the model reports both surfaces and returns the same
pair at every probe Z, and Column picks the room, with actual slabs in the column.
Verified on FreeBSD 15 with clang 19.1.7: 116 tests, 0 failed.
Codex's findings on c207019, plus what auditing around them turned up. Two of the three are defects the previous pass introduced. - WMO liquid selected the containing room by casting down from p.z, but the caller passes the SWEEP TOP, not the query: getLiquidStatus asks ColumnAt for z + FLOOR_BURIED_LIFT, 50 yards up. The comment above that call said the height along the column could not matter -- true until the previous pass made the model choose, and false from that moment. The fix is not to pass a better Z down. It is that a model cannot answer which room at all, and Column.hpp already says so: one gather, many selections. LiquidLocal is now LiquidsLocal and appends every surface; the choice moved to Column::LiquidAt(z), which has the real Z and already holds the floors, since every RaycastAll hit enters the column as a Static surface. The rule is the highest surface reachable without crossing a floor or a ceiling. That also answers two cases nobody reported: a player in a cellar under a building was put in the lake outside, and a player standing on a bridge was put in the river under it. Both were HighestLiquid answering a containment question. The two HeightCheck call sites asked the same question the same wrong way and moved too. GetTerrainType keeps HighestLiquid: it is a 2D query with no point to contain. - hasTerrain meant sawMcnk -- that a chunk TAG went past -- while the height grid is zero-filled before the walk. So a chunk with no MCVT, a chunk missing entirely, a duplicated index, or a truncated file that the chunk loop leaves by breaking, all left 9x9 corners of flat ground at 0.0 inside an otherwise real tile, written and counted as a successful map square. MCVT is required, and the parse now demands all 256 chunks supplied heights. Partial is not terrain. - ReadTile accepted an empty grid unconditionally, so a tile could DECLARE terrain or liquid and carry none: it passed the startup probe and then answered every query with nothing. Flag and grid must agree in both directions, which the writer guarantees -- verified through MpqTileSource and WriteTile, including the global-WMO path, where every grid is legitimately empty and the flags say so. Audited and found clean: ReadGroup already validates the WMO liquid grid exactly and bounds-checks flags at use, and LoadGlobalWmo sets isGlobalWmo only after the model has actually loaded. Five new tests. The old stacked-rooms test passed through the nearest-surface FALLBACK, not the room logic -- its fixture had no floors at all, which is precisely why the wrong fix went green. The 20 ADT fixtures each built a single MCNK, so they now build a whole tile around the chunk under test. That padding has to be emitted BEFORE the chunk under test, which took clang to notice: V9 is 129x129 walked at a stride of 8, so adjacent chunks share their border row of corners -- chunk (x, 1) and chunk (x, 2) both write row 16. Real neighbours store equal heights there and the order cannot matter; flat padding written last flattened the row two tests were asserting on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review the last commit does it fix issues raised by you earlier? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e31f6263a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| return nullptr; | ||
| } | ||
| ParseAdt(objBytes, adt, AdtParts::Objects, lvfOf); |
There was a problem hiding this comment.
When a split 5.x _obj0.adt exists but ParseAdt(..., AdtParts::Objects, ...) returns false, this result is ignored and the loader still returns a terrain tile. BakeMap only checks tile->hasTerrain, so an unreadable object ADT can be written and counted as a successful tile with all WMO/M2 placements missing, leaving static collision and indoor area data absent without a nonzero extractor exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed -- with a sharper test for the case than the one you name.
The result really was dropped: an _obj0 that exists and does not parse gave back a
tile with correct ground and no placements, which BakeMap counts as a success because
it only ever looks at hasTerrain. That is the exact quiet failure the comment eight
lines above this call warns about, and I quoted that same comment in an earlier reply
as evidence for something else without noticing the return value under it. It now
returns nullptr.
The neighbouring hole is worse and was not in the finding: an _obj0 that is MISSING.
split is decided by whether that file reads, so a 5.4.8 install missing one falls
through to AdtParts::Both on the root, finds no MWMO/MMDX/MODF/MDDF there, and bakes
perfect terrain with nothing standing on it -- and that is indistinguishable, at this
line, from a pre-Cataclysm monolithic tile, which must take exactly that path.
Asked of the MAP rather than the tile: if any tile the WDT declares has an object half,
this client writes them, so a tile without one is incomplete rather than old. It stops
at the first hit, so on split data it is one Contains lookup; 3.3.5a data, which has
none anywhere, still takes the monolithic path.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, and a full extraction of
every map in Map.dbc byte-identical to before -- so neither check fires on retail data.
| m_owner = pGo; | ||
| m_phaseMask = pGo->GetPhaseMask(); | ||
| UpdatePose(); | ||
| return true; |
There was a problem hiding this comment.
This initializes the model's owner, phase, and pose but leaves m_collidable at its default false. The initial spawn is later repaired by AddToWorld() calling UpdateCollisionState(), but in-world display changes such as destructible buildings calling SetDisplayId() rebuild and insert a fresh model without another collision-state refresh, so the damaged/destroyed replacement stops contributing to LOS/height until some later state or phase change happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed -- at the caller rather than in Initialize.
Your trace is right: m_collidable defaults to false, AddToWorld repairs the initial
spawn by calling UpdateCollisionState(), and SetDisplayId -> UpdateModel deletes
the old model, builds a fresh one and inserts it with no such call. A destructible
building swapping to its damaged or destroyed display is a SetDisplayId, not a spawn,
so from that moment the wall contributes nothing to LOS or height until some later state
or phase change happens to call UpdateCollisionState for another reason.
UpdateModel now calls it after inserting. That is the better place than initialising
m_collidable in Initialize: collidability is not a property of the model, it is
derived from the owner's current state via IsCollisionEnabled(), and phase mask has to
be set from the same decision -- UpdateCollisionState already does both, and doing it
where the model enters the map keeps one rule in one place. It is a no-op when the
object is not in the world, which is the other caller's case.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed.
| g_console.SetStage("dbc"); | ||
| ExtractDbc(mpq, opt.dest + "/dbc", opt.locale); |
There was a problem hiding this comment.
When mangos-extractor dbc or the default getmangos extraction runs against an unwritable/full destination, ExtractDbc() may copy zero or only some DBC files, but this call ignores the returned count and the program can still exit 0 after the old data was deleted. Propagate DBC copy/write failures to the final exit code so users do not end up with a reported-successful install that has missing dbc/ files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the same defect as the partial-bake one from the first pass, in a
stage I never went back to. That pass made tile failures reach the exit code and stopped
there; dbc, gomodels and the nav header were left exactly as they were. Fixing the
instance and not the class is on me.
ExtractDbc now answers with the number of databases it could NOT write -- the same
contract BakeMap already uses, and for the same reason: getmangos.sh deletes the
installed data BEFORE running this, so a run that copies half the DBCs and exits 0 leaves
nothing to restore. Both continue paths (mpq.Read failing, fopen failing) and a
short fwrite now count.
Writing nothing at all is also counted, since that is a failure with no failed file
behind it: an empty DBFilesClient glob, or a destination directory that could not be
created. create_directories takes an error_code here and never threw, so that case
used to produce a clean exit 0 with an empty dbc/.
The caller accumulates across the primary locale and the --all-locales loop, and aborts
before any tile work rather than after it. A locale with no archives at all stays a
warning and a skip, which is the pre-existing and correct behaviour -- a client simply
does not ship every translation.
Verified on FreeBSD 15 with clang 19.1.7: 117 tests, 0 failed, and a full extraction of
the real 5.4.8 client still completes with rc=0.
| if (!WriteFile(m_outDir + "/" + name, ¶ms, sizeof(params), nullptr, 0)) | ||
| { | ||
| return 0; |
There was a problem hiding this comment.
If the mmaps directory cannot be created or this .mmap header write fails, BakeMap() returns 0, which BakeAll() treats as a successful map that simply produced no tiles because only negative values are fatal. A nav-only or full extraction can therefore finish with exit code 0 and no mmap for that map; return a negative error here so the caller aborts instead of accepting an empty nav output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed -- returning -1, with the diagnostic that was missing.
Your reading of the contract is exact: BakeAll treats only a negative as fatal, so
return 0 here put "the mmaps directory could not be created" in the same bucket as "a
map that legitimately produced no tiles", and a nav-only or full run finished green with
no .mmap for that map.
Auditing around it turned up two more of the same shape in the same file, neither of
them in the finding:
BakeTilereturned false without settingtileErrorwhen the.mmtilewrite failed.
The worker only counts a failure when that flag is set, so an I/O failure AFTER the
mesh was built was filed as "open ocean, nothing to walk on". A full disk produced a
partial navmesh at exit 0 -- the same outcome as this finding, one level down.BakeAllskipped a global-WMO map whosew_tile would not read. That map is in the
list BECAUSE the tile was found on disk a moment earlier, so failing to read it now
means truncated or stale-format, and a global-WMO map is an entire dungeon vanishing
from the navmesh with nothing logged.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, and a full extraction of
the real 5.4.8 client byte-identical to before the change.
| // it is read only so an older or hand-made tile is not silently dry. MH2O, | ||
| // parsed after every MCNK, overrides whatever this writes. | ||
| constexpr uint32_t MCLQ_BYTES = 8 + 81 * 8 + 64; | ||
| if (!offsMclq || sizeMclq <= 8 || offsMclq + 8 + MCLQ_BYTES > span) |
There was a problem hiding this comment.
For a malformed pre-WotLK MCNK with offsMclq near UINT32_MAX, this 32-bit addition can wrap below span and pass the guard, after which mcnk + offsMclq + 8 points outside the ADT buffer and the extractor reads from invalid memory. Use a 64-bit sum for the MCLQ bounds check, like the MH2O and MCVT checks do, so corrupt client data fails the tile instead of crashing or baking garbage liquid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
offsMclq comes straight off the file, and summed in 32 bits a value near UINT32_MAX
wraps below span, so the guard passes and mcnk + offsMclq + 8 is dereferenced
outside the buffer -- read, not merely computed. Widened to a 64-bit sum, which is what
the MCVT and MH2O checks already did; this one was the odd guard out, sitting in the
function I had just rewritten around it.
Test added: an MCNK with ofsMCLQ at 0xFFFFFFF0 and a sizeMCLQ past the <= 8 guard.
The chunk is simply dry, which is the 5.4.8 norm anyway, and the tile still parses with
its heights intact.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, and a full extraction of
every map in Map.dbc byte-identical to before -- no retail chunk reaches this path.
| if (!m_archive.Read(WmoGroupPath(rootPath, g), groupBytes)) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
When a WMO root declares multiple groups and one group file is missing or ParseWmoGroup() rejects it, these branches silently skip that group and still return a non-empty model if any other group loaded. The tile/gomodel bake then exits successfully with part of the building's collision, liquids, and area data missing, so missing or malformed declared groups need to make the WMO load fail rather than producing a partial model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
NOT APPLIED AS WRITTEN, and the client is what settles it. Posting the measurement
rather than the argument, because I did apply this first and it broke the extraction.
false from ParseWmoGroup did not mean the group failed to parse. WmoParser.hpp
stated the contract in as many words -- "Returns false when the group carries neither
collidable geometry nor liquid" -- and those are the render-only, portal and ambient
groups that the loader deliberately does not carry, as its own comment two lines above
says. I read the finding, applied it, and only then measured.
Instrumented against the real 5.4.8 client:
56 groups across the whole client answer false
0 declared group files are missing from the archives
They are not small or broken: Stormwind group 82 of 281 (96 KB), both Ulduar towers
(125 KB), Icecrown Arthas Precipice (867 KB), 24 groups of Skywall. Failing the model
on them cost 11 game-object models -- silently downgraded from partial collision to
none, since BakeGoModels counts an empty model as "no collision" and exits 0 -- and
all of map 576 Nexus70, whose global WMO stopped baking and failed the whole run with
rc=1.
What was wrong was that the caller could not tell your case from that one. So the
contract changed rather than the policy:
enum class WmoGroupParse : uint8_t { Malformed, Empty, Loaded };
Malformed-- too short, no MOGP at all, or a chunk declaring more bytes than the
file holds -- fails the whole WMO, exactly as you ask. The root declares its group
count, so a group that is broken is a missing wing, not an absent one.Emptyis skipped, and the building still loads.- The truncation case is new: the chunk walk used to
breakout and answer with
whatever it had read, so a truncated download looked merely empty.
The half of your finding that was unambiguously right is applied without qualification:
a group file the archive does not have now fails the model. The probe shows that case
does not occur in retail, so it costs nothing and catches real corruption.
ParseWmoRoot's result was also being ignored a few lines above -- a root with no MOHD
left nGroups at 0 and cached an empty model as loaded. That one is fixed too.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, and a full extraction of
every map in Map.dbc is byte-identical to before the change -- same 15,710 files, same
cksum -- with map 576 baking and gomodels back to 5,532 written / 2,474 without
collision.
| } | ||
| else | ||
| { | ||
| g_console.Error("GameObjectDisplayInfo.dbc could not be read"); |
There was a problem hiding this comment.
If GameObjectDisplayInfo.dbc is missing or unreadable during a mangos-extractor gomodels run, this logs an error but then falls through to the normal completion path, where BakeNav(false) returns true and main exits 0. That leaves gomodels/ empty while reporting success, so the gomodel stage should propagate this failure instead of only printing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
The stage logged the error and then fell through to the ordinary completion path, where
BakeNav(false) returns true and main exits 0, leaving gomodels/ empty behind a
clean exit. Every door, lift, bridge and ship hull in the world is a gomodel, and tile
refuses to run without them -- so the failure surfaced later as a second, unrelated-
looking error rather than as itself. It returns 1 now.
This is the same defect as the partial-bake one from the first review pass, which is
the honest thing to say about it: that pass named the class -- a stage that does nothing
and exits 0 -- fixed it for tiles, and did not go back for dbc, gomodels, the nav
header, the .mmtile write, the unreadable global-WMO tile in BakeAll, or
BakeVesselMaps. All of those are propagated now, in this commit and the next one.
Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, and a full extraction of
the real 5.4.8 client still completes with rc=0.
…proves Codex's findings on 9e31f62. Three of them are the same class the FIRST pass claimed to have fixed -- "exits 0 after doing nothing" -- in stages I fixed the tile instance of and never went back to. That is the honest summary: the class was named and then only one member of it was treated. Applied: - ExtractDbc answers with the number of databases it could NOT write, the contract BakeMap already uses, and the caller aborts before any tile work. Both silent continues (mpq.Read failing, fopen failing) and a short fwrite now count, as does writing nothing at all -- an empty DBFilesClient glob or a destination that could not be created, which produced a clean exit 0 over an empty dbc/. - A gomodels run that cannot read GameObjectDisplayInfo.dbc logged an error and fell through to the success path, leaving gomodels/ empty behind exit 0. Every door, lift and bridge in the world is a gomodel. - The nav .mmap header write returned 0 on failure, and BakeAll treats 0 as a map that legitimately produced no tiles -- only negative is fatal. A nav bake could finish green with no navmesh for the map. It returns -1 and says why. - The split-tile loader threw away the _obj0 parse result, so an unreadable object ADT gave back correct ground and NO placements, which BakeMap counts as a success because it only looks at hasTerrain. That is the exact failure the comment eight lines above it warns about. - The MCLQ bounds check summed uint32, so an offset near UINT32_MAX wraps below span, passes the guard, and is then dereferenced. MCVT and MH2O widen to 64 bits; this one did not. Test added. - GameObject::UpdateModel inserted a freshly built model without refreshing its collision state, and a fresh model is non-collidable. Only AddToWorld refreshed it, so a destructible building swapping to its damaged or destroyed display -- a SetDisplayId, not a spawn -- contributed nothing to LOS or height until some later state or phase change happened to call it. NOT applied, because the client disproves it. The finding says a declared WMO group that ParseWmoGroup rejects must fail the model instead of being skipped. But false from that function does not mean the group failed to parse: WmoParser.hpp states the contract in as many words -- "Returns false when the group carries neither collidable geometry nor liquid" -- and those are the render-only groups the loader deliberately does not carry, as its own comment says. I applied it anyway before checking, and measured what it does to real 5.4.8 data: 56 groups across the client answer false, in files of 96KB to 867KB -- Stormwind group 82 of 281, both Ulduar towers, Icecrown, 24 groups of Skywall -- and NOT ONE declared group file is missing from the archives. It cost 11 game-object models, silently downgraded from partial collision to none, and all of map 576 Nexus70, whose global WMO stopped baking entirely and failed the whole extraction. The half of the finding that is real is kept: a group file the archive does not have IS an incomplete client, since the root declares the count. It fails the model now. The probe shows that case does not occur in retail, so it costs nothing and catches actual corruption. Verified on FreeBSD 15 with clang 19.1.7: 117 tests, 0 failed, and a full extraction of the real 5.4.8 client -- every map in Map.dbc, not a sample -- back to its exact pre-change baseline: 220 maps, 9,724 tiles, 5,532 gomodels with 2,474 having no collision, map 576 ok, rc=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grok's review of the extractor. Nine changes, and every one of them is about what happens when something is WRONG -- verified by the fact that on good data they change nothing at all: a full bake of the real 5.4.8 client before and after is byte-identical, same 15,710 files, same cksum 3643011709 658120 over the whole 12 GB. ParseWmoGroup now answers with three outcomes instead of two, because the two it had were the wrong two. A group with neither collidable geometry nor liquid and a group that is BROKEN both returned false, and the caller could not tell them apart -- so the previous pass had to pick one meaning for both and picked skip. Malformed (too short, no MOGP, a chunk declaring more bytes than the file holds) fails the whole WMO, since the root declares its group count and a missing wing is not an absent one. Empty is skipped, which is the ordinary case: render-only, portal and ambient groups are most of any building, 56 of them across retail. The exit-code class, which this branch has now named three times, has three more members: - BakeTile returned false without setting tileError when the .mmtile write failed, so the worker filed an I/O failure under "open ocean, nothing to walk on" and did not count it. A full disk produced a partial navmesh at exit 0. - BakeAll skipped a global-WMO map whose w_ tile would not read. The map is in that list BECAUSE the tile was found a moment earlier, so this is a truncated or stale file -- and a global-WMO map is an entire dungeon, silently absent from nav. - BakeVesselMaps returned the count it wrote and the caller dropped it. An empty list stays a warning, since `all` enables the stage and the shipped list may name nothing; a hull that was asked for and could not be written is now a failure. Two silent-corruption paths: - The three placement transforms built a default Transform and assigned .scale, bypassing the three-argument constructor whose entire purpose is to clamp. Its comment describes the exact damage: MDDF stores scale as uint16/1024, a malformed record gives 0, worldToLocal divides by it, and every ray comparison against the resulting NaN is false -- so the model is not hit rather than failing. The doodad case clamps the PRODUCT, since either factor alone can be fine. - Split-tile detection asked "can I read an _obj0 next to this root", which cannot separate a pre-Cataclysm monolithic tile from a 5.4.8 install missing one, and those need opposite answers. Asked of the map instead: if any tile it declares has an object half, the client writes them, and a tile without one is incomplete rather than old. The monolithic path still serves 3.3.5a data, which has none anywhere. And two smaller ones: an unrecognised liquid id with the DBC store loaded answered Water off the canonical fallback table, which makes unidentified magma or slime swimmable -- once the store is loaded it is the authority, and unknown is None. A WMO root with no MOHD had its parse result ignored, leaving nGroups at 0 and caching an empty model as loaded. Not every finding was applied. The GUI quoting, the --threads help text, MH2O multi-layer collapse, the StormLib 64-bit size, per-fail tile logging, negative caching and MmapTileHeader padding are all real but none of them can produce a wrong answer on this client; they are noted, not fixed here. Verified on FreeBSD 15 with clang 19.1.7: 119 tests, 0 failed, plus the byte-identical full extraction above -- 220 maps, 9,724 tiles, 5,532 gomodels with 2,474 having no collision, rc=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Grok's review of src/shared/terrain. All twelve findings answered -- ten changed, two answered with evidence that they are not defects. The bake output is byte-identical to before, same cksum 3643011709 658120 over 15,710 files, because none of this changes what good data produces. THE ONE THAT WAS HALF-DONE. The previous commit made the baker build placements through Transform's three-argument constructor, whose whole purpose is clamping a non-positive or non-finite scale. ReadTile then read the pose back as raw PODs and walked straight past it, so a tile with scale 0 or NaN reintroduced at load exactly what was closed at write. Same rule now applies on the way in, and a non-finite position, rotation or bounding box rejects the tile outright. TRUSTING A FILE THE WAY WE TRUST OUR OWN MEMORY. - Bvh::Adopt took a node array straight from disk. Build guarantees MAX_DEPTH, one parent per node, and indices into its own soup -- which is precisely what lets Raycast walk with a fixed stack and no bounds test, as the comment there says. None of that holds for a file. Adopt now proves the shape once: children in range, no node reached twice (a cycle or a shared child), depth within what the query stack is sized for, leaf ranges inside the triangle array. The raycast comment stays true instead of being weakened by a per-ray check that would silently drop subtrees. - Triangle vertex indices are checked against the soup at load, since TriSoup::At does no checking by design. - MLIQ's flags grid was never validated while its heights grid was. GroupLiquidAt reads it as `fi < flags.size() && ...`, so a short array does not fail -- it answers "not dry" past the end and floods rooms the client marks as having no water. - TerrainHeight and LiquidAt guarded on empty() while indexing at (ix + 1, iy + 1); they require the full grid now, which is what ReadTile already enforces and what a tile built in memory by a tool never went through. CORRECTNESS IN THE QUERY PATH. - Segment queries picked tiles by sampling the line every half tile. A tile the segment only clips a corner of gets no sample, and a model living solely on that tile does not exist for the ray: sight through a building, a fall through a bridge. Replaced with grid traversal, which enters every tile the segment touches. The test is a segment where tile (31,32) owns 2.5% of the length and none of the ten old samples lands in it -- red on the previous commit's code, green on this one. - PinCell incremented an int16_t without saturating: at 32,768 pins it wrapped negative, after which UnpinCell's `> 0` never decremented and the cell was both permanently unpinnable and evictable while referenced. uint32_t, saturating. - The absent-tile memo was never swept, so a tile that appears after start -- a re-bake, a late mount -- stayed invisible for the life of the process. - RaycastAll's thread_local scratch buffer is swapped out for the duration rather than used in place, so a second RaycastAll on the same thread cannot clear the buffer its caller is still filling. - TileIndex mapped the map's far edge to 64, one past the last tile, so the extreme corner of a 64x64 map answered "no tile" instead of the tile it is the corner of. Only that exact boundary folds; further out stays out of range. TWO FINDINGS THAT ARE NOT DEFECTS, both answered with measurement rather than argument. Cross-tile instances, rated P0: the claim is that a model is written only to the tile of its placement, so an overhang across an ADT line disappears. But the baker applies no tile-bounds filter at all -- it attaches whatever the ADT lists -- and the client lists a placement in EVERY ADT its footprint touches. Instrumented and counted on map 0 of the 5.4.8 client: 454 of 2,724 distinct placements are listed from more than one ADT, one Vashj'ir dome from sixteen. The coverage is already there, and filtering to "placed here" is what would create the hole. Recorded in the code, with the numbers, so it is not "fixed" later. The real corollary is the opposite one: we keep the duplicates, because nothing keys on MODF's uniqueId. WMO liquid carrying no deep flag: dark water is an ADT attribute -- the MCLQ per-cell dark bit, or an ocean cell whose MH2O instance carries the deep attribute -- and MLIQ has no equivalent, its tile-flag nibble saying only which tiles are dry. vmap never produced fatigue from WMO liquid either. Adding one means inventing the source, so the absence is documented on the field rather than filled in with a guess. Verified on FreeBSD 15 with clang 19.1.7: 120 tests, 0 failed, and a full extraction of the real 5.4.8 client byte-identical to the previous baseline -- 220 maps, 9,724 tiles, 5,532 gomodels with 2,474 having no collision, rc=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f85f72f4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (inst.model->Kind() == world::terrain::ModelKind::Wmo) | ||
| { | ||
| soup = &static_cast<const world::terrain::WmoModel*>(inst.model.get())->Soup(); | ||
| } |
There was a problem hiding this comment.
When a WMO group carries its own MLIQ surface, this path copies only the WMO collision soup into the solid mesh; the liquid soup is filled only from ADT tile liquid. The old generator emitted WMO liquids from group models and the runtime still reports them through WmoModel::LiquidsLocal, so indoor WMO pools/lava/slime that are not ADT liquid disappear from mmaps and swim-only or liquid-aware pathing cannot navigate them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
The two halves genuinely disagreed: a group's MLIQ is baked into the model and the
runtime answers with it -- FusedTerrain asks WmoModel::LiquidsLocal -- while the nav
build filled its liquid soup from ADT tile liquid alone. So swim and liquid-aware pathing
were told there is water in an indoor pool, a lava channel or a slime pit with no water
polygon anywhere under it.
AddModelLiquids walks the tile's WMO instances and emits each group's MLIQ grid through
the instance transform, with the area from the same LiquidArea mapping the ADT path
uses. Called for the tile and for a global-WMO map, which is where it matters most -- an
entire dungeon is one instance.
Three details, each checked rather than assumed:
- the dry test is the runtime's own,
(flags[i] & 0x0F) == 0x0F, so the mesh is swimmable
exactly where the server says liquid is, not merely nearby; - neither of
AddLiquid's two rules carries over. There is no terrain heightmap to
bury a surface under inside a building, and MLIQ has no deep bit at all -- the note on
WmoModel::Liquidsays so and explains why inventing one would mean inventing its
source; - neighbour tiles are unchanged.
AddNeighbourGeometryimports terrain and liquid cells
but deliberately not models, so importing neighbour WMO liquid would have duplicated
placements that already overlap the current tile.
LIQUID_TILE_SIZE moved from an anonymous namespace in WmoModel.cpp to the header as
WMO_LIQUID_TILE_SIZE, unchanged in value, so the runtime lookup and the bake index the
same grid with the same number instead of each holding a copy.
| if (WriteTile(tile, dest + "/" + GoModelFileName(entry.first))) | ||
| { | ||
| ++written; | ||
| } |
There was a problem hiding this comment.
When the gomodel destination is unwritable or the disk fills, WriteTile returns false here but the stage only skips incrementing written and still returns through the success path. A mangos-extractor gomodels run, or a full all run with no vessel entries, can therefore exit 0 with missing go_*.tile files, and the runtime then has no collision for doors, bridges, lifts, and similar game objects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and this is the same defect as the ExtractDbc one from the pass before it -- in
the stage next to it. Fixing the instance and not the class is on me, twice now.
BakeGoModels returned void. A failed WriteTile skipped the written++ and fell
through the success path, so mangos-extractor gomodels, or a full run, exited 0 with
go_*.tile files missing -- and every door, bridge, lift and cannon in the world is a
gomodel. The comment on the DBC branch immediately below the call already said what was at
stake ("nothing below this reads the stage's result"); the write half simply was not
covered by it.
It now answers with the number it could not write -- the contract ExtractDbc and
BakeMap already use -- and the caller aborts on a non-zero result the same way the DBC
stage does, before any tile work.
create_directories is counted too: it takes an error_code here and never threw, so an
unwritable destination produced a clean exit 0 with an empty gomodels/.
| if (pos + 8 + csize > size) | ||
| { | ||
| break; |
There was a problem hiding this comment.
When a split _obj0.adt is cut in the middle of a top-level object chunk such as MODF or MDDF, this break makes ParseAdt(..., AdtParts::Objects, ...) return true with the remaining placements silently absent. LoadAdt() then returns a terrain tile and BakeMap() counts it as successfully written, leaving WMO/M2 collision and indoor area data missing without reaching the extractor's failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed -- and the reason nobody had noticed is worth stating, because it
explains the other two of the same shape in this pass.
The terrain half of this parser has been guarded since the first pass: filled requires
all 256 MCNKs to have supplied heights, so a file cut anywhere in the grid already failed.
That guard is the whole reason break looked survivable here. Parsed with
AdtParts::Objects there are no MCNKs to come up short, so the one check that noticed a
truncated file was not on the path this finding is about: a split _obj0 cut inside MODF
or MDDF returned true with the remaining placements simply absent, LoadAdt handed back
a tile, and BakeMap counted it written because it only ever looks at hasTerrain.
The walk now records that it broke and the parse says so. One existing test asserted the
opposite contract -- AdtStopsOnTruncatedChunk did REQUIRE(ParseAdt(...)) and checked
only that no terrain was claimed -- so it was not quietly flipped: its siblings
(AdtMissingOneChunkFailsTheParse, AdtChunkWithoutMcvtFailsTheParse) fail the parse for
exactly this class, and it has been strengthened and renamed to match, with the reason in
the test.
New test for the case actually reported: a complete MODF followed by an MDDF declaring
more bytes than the file holds, parsed as AdtParts::Objects.
| const std::string path = WdtPath(mapId); | ||
| std::vector<uint8_t> bytes; | ||
| WdtData wdt; | ||
| if (path.empty() || !m_archive.Read(path, bytes) || !ParseWdt(bytes, wdt)) |
There was a problem hiding this comment.
When a Map.dbc entry's WDT is present but truncated or malformed, this collapses the read/parse failure to the same nullptr used for maps with no WDT; BakeMap() explicitly returns 0 for !wdt, so a full extraction can omit that map's entire tile/nav cache and still exit successfully. Keep an absent WDT distinct from a failed WDT read/parse and report the latter as a tile failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
The comment above BakeMap justifies the return 0 for exactly one of the two cases it
covers -- "a map with no WDT at all is not a failure -- Map.dbc lists identities that
never had terrain" -- and that is true and stays. What it does not cover is a WDT that
is there and will not read or parse, which is a truncated or stale-format client, and
which arrived at the same nullptr. A full extraction could drop that map's entire tile
and nav cache and exit 0.
Wdt() now records the maps whose WDT was present and failed, and MpqTileSource:: WdtUnreadable() answers the question Wdt() structurally cannot. BakeMap returns 1 for
those and logs the map, while an absent WDT stays silent.
The empty-path case (a map id with no directory in Map.dbc) is separated out and returns
before the read is attempted, so it cannot be counted as a broken file.
| if (pos + 8 + sz > n) | ||
| { | ||
| break; |
There was a problem hiding this comment.
When a WMO root is truncated after MOHD, for example in the MODN/MODD doodad metadata, this break still lets ParseWmoRoot return true because sawHeader remains set. WmoLoader::Load can then bake the root groups while AttachWmoDoodads sees partial or no doodads, so the M2 collision inside that building disappears with a successful extraction; treat a top-level root chunk overrun as a parse failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed.
sawHeader is set by MOHD, which is the first chunk -- so returning it said only that
the file began correctly, and the bulk of a root's bytes are MODN and MODD, exactly where
the finding puts the cut. WmoLoader::Load then baked the root groups against doodads
that were partial or absent: the building stands, everything inside it has no collision,
and the extraction reports success.
ParseWmoRoot now returns sawHeader && !truncated.
Worth noting for anyone reading this later: ReadDoodads walks the same buffer a second
time with its own break, so it stops in the same place -- it is not a second opinion on
whether the file is whole, which is why the answer has to come from the chunk walk.
New test: MOHD complete, then a MODN declaring 0x10000 bytes into a 16-byte tail.
| if (advance != MOGP_HEADER && pos + 8 + sz > n) | ||
| { | ||
| truncated = true; | ||
| break; |
There was a problem hiding this comment.
When a WMO group's MOGP container declares a size past EOF or shorter than its 68-byte header, this guard exempts it from the only chunk bounds check because advance was rewritten to MOGP_HEADER. ParseWmoGroup can therefore return Empty or Loaded from a truncated group, and WmoLoader::Load may bake the rest of the WMO with that group's collision or liquid missing; validate the declared MOGP size before walking its nested chunks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed -- and the audit around it found the sharper half, which was not in the
finding.
Your reading is exact: stepping into the container by its header is the whole point, so
advance is rewritten to MOGP_HEADER and the guard below then skips it -- meaning the
container's own declared size was validated nowhere. The 68 bytes it needs are usually
present, so nothing downstream noticed either.
The half that was not reported: the guard tested advance != MOGP_HEADER. That compares a
value, not a role. Any ordinary chunk -- MOPY, MOVI, MOVT, MLIQ -- whose size happened
to be exactly 68 bytes therefore exempted itself from the only bounds check in that loop,
container or not. It is now a isContainer flag set where the tag is matched, so the
exemption cannot be earned by a coincidence of size.
MOGP is validated where it is recognised: sz < MOGP_HEADER (too short to hold the header
it is about to be read for) or pos + 8 + sz > n (declares more than the file has) is
Malformed.
Two new tests, both watched fail first: a MOGP declaring 0x7FFFFFFF, and a MOPY declaring
exactly 68 bytes with 8 present.
Six P2 findings from the automated review of 9f85f72. All six were real; each is the same shape as the passes before it -- a failure the extractor was able to swallow and still exit 0 -- and the audit around them turned up one more that was not in the finding. PARSERS THAT ANSWERED TRUE ON A TRUNCATED FILE. Three of them, and the terrain half of the ADT parser was the only one that ever noticed: `filled` catches a cut-off MCNK grid, so nobody looked at the objects half, where there are no MCNKs to come up short. A split `_obj0` cut inside MODF or MDDF parsed "successfully" with its placements simply absent, and BakeMap wrote the tile. ParseWmoRoot returned sawHeader, and MOHD is the FIRST chunk -- a root cut in MODN/MODD, which is where a root's bytes actually are, left the building standing with no collision on anything inside it. MOGP is exempt from the group loop's bounds check because stepping into the container by its header is the whole point, so its own declared size was checked nowhere. Not in the finding: the exemption tested `advance != MOGP_HEADER`, which is a VALUE, so any ordinary chunk whose size happened to be exactly 68 bytes exempted itself from the only bounds check in that loop. It is now a flag on the container. STAGES THAT DID NOT REACH THE EXIT CODE. BakeGoModels returned void and counted a failed write as one simply not written -- every door, bridge and lift in the world is a gomodel, and getmangos.sh deletes the installed data before this runs. It now answers with the number it could not write, the contract ExtractDbc and BakeMap already use, and the caller aborts on it. A WDT that is PRESENT and will not parse collapsed to the same nullptr as a map that never had one, and BakeMap is right to pass over the latter. Kept apart now: absent is silence, broken is a tile failure. WMO LIQUID REACHED THE NAVMESH FROM NOWHERE. A group's MLIQ is baked into the model and the runtime answers with it (FusedTerrain -> LiquidsLocal), while the nav build rasterised ADT liquid only -- so swim and liquid-aware pathing were told there is water in an indoor pool, a lava channel or a slime pit with no water polygon under it. Emitted through the instance transform, using the runtime's own dry test so the mesh is swimmable exactly where the server says liquid is. Neither of AddLiquid's two rules carries over and both were checked: there is no terrain to bury a surface under inside a building, and MLIQ has no deep bit -- see the note on WmoModel::Liquid. Verified on FreeBSD 15 / clang 19.1.7: 124 tests, 0 failed. The five new cases were watched RED first -- reverted the two parser fixes and they fail 5/5, restored and they pass. The first attempt at that check proved nothing: `git checkout --` restores from the INDEX, and `git apply --3way` had staged the fixes, so the "reverted" build still contained them and came back green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
One terrain engine, in src/shared/terrain. FusedTerrain answers with a column of surfaces instead of a single height, so terrain, WMO floors, liquid and dynamic game objects all come back from one query. DynamicCollision puts doors and lifts in that same column.
One extractor replaces the three separate tools. mangos-extractor bakes DBCs, game-object models, terrain tiles, vessel decks and the navmesh, in that order, each from the output of the last. The navmesh is built from the baked tiles, so the pathfinder walks the surface collision actually answers with.
Client support is 5.4.8. MCNK offset 0x14 is read as holes_high_res, not as an offset. Hole maps are 64-bit per chunk. MCVT is located by scanning the sub-chunks. MH2O resolves through the LiquidObject chain, and object 42 is depth-only ocean. The archive set is the Mists one, with all eighteen update builds.
The update archives now patch each other, not only the bases. Without that, 1374 files across 29 maps resolved to pre-release builds.
Three Recast accuracy values from #250 are restored. On slope, navmesh points more than a yard off the floor fall from 40.5% to 10.9%.
mangos-height-check scores a bake against a position corpus, separating liquid, model floors and unsettled probes rather than averaging them in. 107 tests cover the parsers, the tile format and the collision model.
Removed: src/game/vmap, src/tools/Extractor_projects, src/tools/Extractor_Binaries and the GridMap class.
Measured on a 5.4.8 client: 9724 tiles over 288 maps. Of 2164 recorded client positions, 98.70% land within 2 yards of the baked floor, 99.14% on dry ground.