[pull] master from mangosone:master - #262
Merged
Merged
Conversation
Six defects found by diffing this tree's Geometry module against a revised edition of the same code. They share a shape: the bad answer is a NaN or an infinity, every comparison against a NaN is false, and so nothing reports an error -- a ray misses, a creature is not in the arc, a distance is simply not less than anything. Placement::HasInArc normalized the ARC. An angle that names a direction and an angle that names a width are different things wearing the same type, and NormalizeOrientation wraps into the half-open [0, 2*PI): a full circle came back as zero, was halved to zero, and matched only a target dead ahead. That is reachable today -- IsInBack(other, dist, arc) asks for HasInArc(2*PI - arc), so an arc of zero meant "everything is behind me", including a creature standing in plain sight. Now arc <= 0 is nothing, arc >= 2*PI is everything, and the half-width is arc/2 without a wrap. Vector3::direction and Quat::unitize divided by the length of a zero-length value: 1/sqrt(0) is infinity and the result is all NaN. Zero in, zero out, and the zero quaternion becomes the identity. Geometry::wrap converted the quotient to int through iFloor. This is reachable from a client packet -- MoveSpline normalizes the orientation the client reports -- and a NaN or a value more than 2^31 intervals from lo made that conversion undefined behaviour, not a wrong angle. It now floors in double and fails closed to lo for non-finite input or a non-positive interval. Values in the range every caller uses are unchanged. Placement's bare-point overloads (DistanceTo, WithinDist, WithinRange, WithinBox) answered for an UNPLACED placement as though it stood at the origin of a map it is not on -- an item in a bag measuring 8843 yards from Stormwind rather than nowhere. They now fail closed like every other cross-frame answer, which is what the class documents. Closer and InBand compared strictly, so WithinDist(other, DistanceTo(other)) was false by construction: DistanceTo returns the separation minus the extents, and feeding it back asked whether a number is less than itself. Both limits are closed now. Aabb::expand(const Aabb&) folded an EMPTY box (lo = +max, hi = -max) into the accumulator and widened it to the whole float range, whose extent overflows to infinity -- after which every SAH split of the BVH costs the same and the tree stops separating. It now ignores an invalid box, and valid() checks all three axes rather than x alone. Aabb::intersectsRay dropped an axis whose reciprocal was not finite, because std::max(t0, NaN) returns t0. An infinite reciprocal still rejects correctly through the infinities; a NaN one -- what a zero-length direction normalises to -- dropped the axis for every origin, so a degenerate ray matched every node it was offered. The guard measures the origin against the PADDED bounds, so the broadphase stays exactly as permissive as before and never tightens below rayTri's tolerance, which the comment there warns against. Deliberately NOT changed: twoPi()/halfPi() keep their g3dlite literals. The header states the values are reproduced verbatim so the math stays numerically identical to the former dependency, and 2*pi() rounds to the same float anyway. Tests: GeometryMathTest.cpp (new) pins wrap, the zero direction, the zero quaternion, the empty-box accumulator and the degenerate ray; PlacementTest gains the full-circle arc, the unplaced item, and the closed reach limit. Four of the new assertions are red on the code before this commit. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Ported from H0zen/mangos_two f039c893 (branch geometry-fixes). The five Geometry headers and PlacementTest.cpp are byte-identical between the two cores at this commit, verified before applying, so the change went in unmodified. Only src/tests/CMakeLists.txt was edited by hand: 01's test list is legitimately different (no LFGLogicTest), so only the one-line GeometryMathTest.cpp entry was taken rather than the surrounding context. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Defects found by diffing this tree's terrain engine and baker against a revised edition of the same code, plus the review findings on the mangos-four port of it (mangosfour/Server#78). The shape they share: a .tile is bytes from outside the process, and the query path trusts them completely. READING A TILE Bvh::Adopt took a node array off disk and moved it in. Nothing downstream re-checks: Raycast pushes both children unconditionally onto a stack sized from MAX_DEPTH, indexes the array with whatever it popped, and reads a leaf's triangle run without a bounds test. All three are safe only because Build cannot emit an array that breaks them -- a file can, and the format has no checksum. Adopt now verifies the array is a depth-first tree over the triangles it indexes and rejects it otherwise; TriSoup::IndicesValid screens the triangles the same way, once, because At() cannot afford a check per ray. BuildNode cut off at depth > MAX_DEPTH, so the deepest node it made was MAX_DEPTH + 1 -- against a comment on that constant saying it is the deepest node Build will create and that the traversal stack is sized from it. ReadTile accepted grids of any size while TerrainHeight and LiquidAt index them by fixed arithmetic and never look at .size(); a WMO group count bounded by MAX_MODELS (2^20) where reality is a few hundred; a liquid grid whose counts disagree with its own vectors; and an instance scale read straight into the field, bypassing the clamping constructor whose comment says a non-positive or non-finite scale is screened once, where a model is placed. Zero off disk divides to infinity in worldToLocal and the model then misses every ray. WriteTile ignored fclose, which is what flushes: a full disk left a truncated tile that looks complete. FusedTerrain::HasTile answered the start-up question "does this map have terrain" by opening the file. A truncated or stale tile passed, then failed inside ReadTile, and every height, liquid and collision query silently answered nothing. It reads the tile now -- once per checked grid, never on a query path. QUERYING CollectSegmentInstances sampled the sight line every half tile. A sample step only guarantees the tiles it lands in, so a segment clipping the CORNER of a tile was missed entirely and the wall standing in it occluded nothing -- from those angles only. Replaced by a supercover tile walk, which advances to the next boundary and cannot skip a tile the segment enters. TileIndex truncated toward zero in front of the shift, so the 4.16-yard strip past the far corner of the map answered with the tile at the opposite edge. WmoModel::LiquidLocal converted to int before range-testing (undefined for a point far outside the group, which is most of the model most of the time) and interpolated four corners of a height grid it never checked the shape of. GoModelStore::Get held its mutex across the disk read, serialising every game-object spawn in the world behind one open. FusedTerrain::PinCell incremented an int16 without a ceiling; TerrainInfo::Load pinned per reference while Unload released only on the last, so N owners of one grid left it pinned N-1 times and the sweep never evicted it. BAKING The extractor logged tile failures and returned 0. The install script deletes the previous data before running it and reads that status as a complete extraction, so a partial bake left a world with holes and said nothing; the nav pass enumerates the tiles that WERE written, so it cannot notice either. Bake failures now reach the exit code, and a map that declares a global WMO and cannot bake it counts as a failure rather than being confused with a map that declares none. A short MCNK header was read anyway (the fields reach 0x74); a chunk with no usable MCVT still counted as read, and the zero-filled grid it left behind bakes as flat ground at sea level. Neither contributes to the chunk count now, and a tile missing any of its 256 chunks is not written. A truncated ADT or a WDT without MPHD/MAIN fails the parse outright. An MLIQ whose corner grid is not one wider than its tile grid is dropped. An MPQ file whose size needs the high dword is refused rather than read truncated. Placements go through the clamping Transform constructor. NavMeshBuilder wrote flags[i] = areas[i] ? 1 : 0. Detour filters on flags, never on the area id, and PathFinder's masks are the NAV_* bits -- so every polygon in the world claimed to be ground: a swimmer's WATER|MAGMA|SLIME mask matched nothing and a walker was cleared across magma. VERSIONS Tile format 1 -> 2 and MMAP_VERSION 6 -> 7. Neither layout changed; what changed is which files are accepted and what a polygon flag means. A stale bake would otherwise load and answer wrongly for as long as it stays on disk -- silence for tiles, bad pathing for the navmesh -- so the version is what makes it say so. TileSerializerTest's hand-built header moves with it: left at 1 it would have been rejected on the version alone and stopped exercising the hostile-count guard it exists for. DELIBERATELY NOT CHANGED. ParseAdt still accepts an ADT with fewer than 256 chunks and reports the count instead, because this parser is also handed deliberately minimal ADTs by ClientParserTest; the baker is what refuses. Two review findings from the four port are real here and are NOT fixed: WMO liquid still answers from the first group in serialised order rather than the one containing the query (stacked pools), and the WMO area probe still accepts an instance up to 300 yards below the query point. Both need a real client to verify against, and the first is a redesign of the column API. Tests: BVH adopt rejections, triangle index screening, and the TileIndex floor in TerrainModelTest; the ADT chunk count and the truncated-chunk contract in ClientParserTest. AdtStopsOnTruncatedChunk changes meaning -- it asserted that a chunk claiming 0x7FFFFFFF bytes parses as an empty tile, which the function's own contract calls a structurally broken file. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Ported from H0zen/mangos_two 70e9e72f (branch terrain-fixes). 14 of the 20 files are byte-identical between the two cores and applied unmodified. The rest were checked individually rather than forced: TileSerializer.cpp -- the two cores differ ONLY in the tile MAGIC ("MNG1" here, "MNG2" in 02), which is each core's own bake signature and must not be unified. That line is context for the first hunk, so the hunk was applied by hand and the magic left alone; the file is now identical to 02's apart from it. GridMap.cpp, MoveMapSharedDefines.h, Extractor.cpp and StormLibArchive.cpp carry pre-existing 2.4.3 divergences -- LiquidType.dbc has no SoundBank column before 3.3.5a so the family comes from Type with a different encoding, and AREA_FLAG_INSIDE/OUTSIDE do not exist in 2.4.3 so WMO group flags are the only authority. The hunks are disjoint from those regions: the divergence measures the same 38/2/10/41 lines before and after the port. NOTE FOR DEPLOYMENT: this bumps the tile format to VERSION 2, and a version 1 bake is now rejected outright -- no height, no liquid, no collision for that tile. The extractor must be re-run before this branch is served. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DISABLE_TYPE_VMAP rows are read at start-up, validated against Map.dbc, logged per flag, and reloadable with `.reload disables`. Nothing has ever read them: IsVMAPDisabledFor is defined and called by nobody, so all four flags -- AREAFLAG, HEIGHT, LOS, LIQUIDSTATUS -- are a documented database feature that does nothing. A server that sets one to work around a bad WMO gets no change and no error, which is the worst of the three possible outcomes. Reported against the mangos-four port of this terrain engine (mangosfour/Server#78) and true here for the same reason: the collision rewrite replaced the vmap entry points that used to consult them, and the consult did not come with. WHERE THE ANSWER LIVES. TerrainInfo, which is per MAP ID -- and the disable is per map id, not per instance, so every instance of a dungeon agrees by construction. The row is read once, when the terrain is built, because the alternative is a container lookup inside every height, liquid and sight query on every map-update thread. `.reload disables` pushes the new value through TerrainManager::RefreshCollisionDisables; a reload that did not would change the table and nothing else for every map already in memory. The lookup itself is new: IsDisabledFor reaches its container through operator[], which INSERTS when a type has no rows, so calling it from map threads would be a write racing a reload's clear. GetCollisionDisablesFor only reads. WHAT EACH FLAG DOES, unchanged from what it meant when this was a vmap: HEIGHT drops the baked model floors and leaves the ADT heightmap standing. LIQUIDSTATUS drops liquid authored inside a WMO and leaves the ADT's own. LOS makes baked geometry stop blocking, and NOT the dynamic tree -- a map with sight disabled still has doors, and Map::IsInLineOfSight asks those separately. Both halves of the sight answer are covered: IsInLineOfSight returns clear AND NearestHitFraction returns past-the-end, or a disabled map would report clear sight and still stop a spell at the wall it had just said was not there. AREAFLAG makes GetAreaInfo report no WMO, which is what its callers already read as "outdoors, no group here". DECOUPLING. The terrain engine takes a mask of WHICH SURFACE KINDS to gather (SOURCE_TERRAIN / SOURCE_STATIC / SOURCE_STATIC_LIQUID / SOURCE_LIVE) and is told nothing about why one is excluded. It does not learn that a database table exists; TerrainInfo::SurfaceSources is the whole translation, and it is the only place that names both vocabularies. Runtime game-object geometry is not baked collision and is never dropped by either flag. Tests: ModelMapTest pins that excluding a source removes that source's surfaces and leaves the others -- a model-backed map is empty without SOURCE_STATIC and unchanged without SOURCE_STATIC_LIQUID. The database half cannot be tested without a database. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Ported from H0zen/mangos_two eb56284e (branch collision-disables). Only GridMap.h, FusedTerrain.hpp and ModelMapTest.cpp are byte-identical between the cores here; the rest were checked in place rather than trusted to the patch. DisableMgr.h was the one hunk that would not apply -- 01 has no blank line after IsVMAPDisabledFor -- so the declaration was inserted by hand at the same point in the namespace. World.cpp and ReloadCommands.cpp diverge heavily between the cores (936 and 130 lines), so both call sites were read rather than assumed: RefreshCollisionDisables lands directly after DisableMgr::LoadDisables in World::SetInitialWorldSettings, and after LoadDisables/CheckQuestDisables in HandleReloadDisablesCommand, which is what the change intends in either core. ReloadCommands.cpp gains the GridMap.h include for sTerrainMgr rather than inheriting it. Symbols verified to resolve after the port: TerrainInfo::RefreshCollisionDisables and TerrainManager::RefreshCollisionDisables (GridMap.h), DisableMgr:: GetCollisionDisablesFor (DisableMgr.cpp), sTerrainMgr (GridMap.h). NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A tile carries a format version, so a bake from an older extractor is refused on
sight. That is the only thing a version can catch. Everything else that goes
wrong with a data set produces files that are structurally perfect:
a tile truncated by a full disk or an interrupted copy,
a map whose tiles came half from one bake and half from another,
a stale file an older run left behind under a name the new one did not write,
a sector that rotted years after the bake.
Each of those loads, and answers wrongly -- or answers nothing -- in one corner
of one map. From the outside that is a server bug that nobody can reproduce,
because the reporter and the developer are not reading the same bytes.
So the baker now records a SHA-256 for every file it writes, and the server
re-hashes them at start-up.
FORMAT. `<64 hex> <path>` per line, sorted, LF, at <DataDir>/data.manifest --
deliberately the layout `sha256sum -c` reads, so an administrator can check a
data set with a tool that was not written here and does not have to be trusted:
cd <DataDir> && sha256sum -c data.manifest
No header and no comment line, because either would break that property. The
manifest is identified by its name and location; every file it covers carries
its own format version inside it.
WHERE THE CODE LIVES. `dataintegrity`, a library of its own under src/shared,
carrying OpenSSL and nothing else. Not part of `shared`: the baker has to WRITE
what the server READS, and `shared` exports MySQL publicly -- linking it into
the extractor to reach two files would drag a database client into a tool that
has never needed one. One implementation, two consumers, so the format cannot
drift from itself. OpenSSL's EVP rather than a vendored hash, because the
project already requires OpenSSL 3.x and the only thing a hand-written SHA-256
buys is ownership of a primitive whose failure mode is a digest that looks fine.
WHAT IT IS NOT. Authentication. The manifest sits beside the files it describes,
so whoever can replace a tile can replace the line about it. This detects
damage.
BEHAVIOUR. DataIntegrityCheck: 0 off, 1 report (default), 2 refuse to start.
A data set with NO manifest is reported once and accepted -- it was baked before
this existed, or by hand, and refusing to start over that would be wrong. A file
present on disk that the manifest does not list is not an error either: a data
directory holds things the baker did not write. Hashing runs one file per worker
across the cores, which is a few seconds for a couple of gigabytes, once.
The baker writes the manifest only on a path that FINISHED, and over the whole
data directory rather than the part that run touched -- a tiles-only run
re-lists the game-object models beside them, because the manifest describes the
SET, not the run. This sits on top of the exit-code work in the terrain-fixes
branch: before it, a bake that failed still reported success, and a manifest
written over that would have certified the holes.
Tests: the digests are checked against the PUBLISHED FIPS 180-4 vectors,
including the million-'a' case that exercises the block loop rather than one
padded block -- a hash that agrees with itself proves nothing, and this one has
to agree with sha256sum. Then a manifest round trip asserting the exact
sha256sum-shaped bytes, a one-byte change, a deleted file, an unlisted file that
must NOT count as damage, an absent manifest, and a malformed line.
NOT COMPILED -- no build was run, per the standing rule that none is invoked
without an explicit request.
Ported from H0zen/mangos_two a0d9745c (branch data-integrity). That branch sits on
top of terrain-fixes, already ported in the previous commit, so only this delta was
taken rather than the branch as a whole.
The four new files (DataIntegrity/{DataManifest,Sha256}.{cpp,h}) and
DataIntegrityTest.cpp are additions and went in as-is; World.h, WorldConfig.cpp and
the extractor's CMakeLists applied unmodified. Four files needed hand work because
they diverge between the cores, and each was placed by reading 01 rather than by
forcing the hunk:
World.cpp -- 01 carries an extra `sLog.outString("Core data")` between the tile
existence check and string loading, so VerifyDataIntegrity() goes immediately
after the tile check, which is where 02 puts it relative to that check.
mangosd.conf.dist.in -- the two configs diverge by ~4000 lines (2.4.3 vs 4.3.4);
only the DataIntegrityCheck block and its default were taken, at the same point
after DataDir.
src/shared/CMakeLists.txt -- 01 orders shared's PUBLIC link list differently, so
`dataintegrity` was added after MySQL::MySQL to keep 02's relative placement. The
target definition itself is verbatim. mangos_openssl_strict is confirmed to exist
in 01 (CMakeLists.txt:198).
src/tests/CMakeLists.txt -- 01's test list has no LFGLogicTest, so only the
DataIntegrityTest.cpp entry and the `dataintegrity` link were taken.
Symbols verified after the port: World::VerifyDataIntegrity (World.h:606),
CONFIG_UINT32_DATA_INTEGRITY_CHECK (World.h:231, defaulted in WorldConfig.cpp:680).
NOT COMPILED -- no build was run, per the standing rule that none is invoked
without an explicit request.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ility ashore Brought over from the `alignment` branch, which developed it as an interleaved chain of commits rather than as a separable feature. Taken here as the net state of that work, regrouped by theme. A vessel's map is not a teleport destination. A player reaches a deck by BOARDING the vessel, and the deck's map is never named to the client -- not in a teleport and not in SMSG_LOGIN_VERIFY_WORLD, which is the packet that would otherwise hand the client a map id it cannot resolve. Two earlier attempts to state this as a refusal were reverted on the branch; what survives is the positive form. Boarding from the water the vessel sails no longer crashes. Chat does not cross a vessel boundary: a deck is its own map, so range on it is a deck-local question, and a shore listener is not in it. Vessels are retracted from decks as well, not only from the world. A deck object's visibility change is relayed to the shore. The deck is a map of its own, so an update that stays inside it is invisible to everyone not aboard. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…one clock Brought over from the `alignment` branch as the net state of an interleaved chain, regrouped by theme. The world heartbeat and the map updates no longer share one tick. Master's loop drives them separately, so a long map update no longer delays everything the world tick owes the client, and the diff each side is handed is its own rather than the whole loop's. SimulationTime is the one clock the server answers timing questions from, instead of each call site reading its own. CMSG/SMSG_TIME_SYNC is implemented against it, so the client's idea of server time is corrected rather than inferred. Movement packets carry their real delay instead of a fabricated one. An earlier attempt on the branch predicted future movement and was reverted; what remains removes the fake latency that the client's own interpolation was already compensating for, which is why movement read as smooth and arrived late. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The remainder of the `alignment` branch, after the vessel/visibility work and the tick split were taken as their own commits. Grouped here because it is what is left, not because it is one change: terrain and extractor hardening, the config parser, taxi splines, and a networking fix. Terrain: liquid interpolation moved into WmoModel::GroupLiquidAt; the tile-crossing walk in FusedTerrain now traverses the grid instead of sampling the segment at fixed steps, which used to miss any tile a segment only clipped the corner of -- sight straight through a building, or a fall through a bridge. Extractor: MLIQ, WMO and DBC parsing reject malformed input rather than baking it. Config gains a test suite (ConfigTest.cpp). Networking: Nagle is disabled on the IOCP path, so a small packet is no longer held back waiting for a peer that is waiting for it. Taxi: continuous multipoint spline flight, and the fall tracker is reset on flight so landing does not read as a fall. THREE CONFLICTS IN THIS AREA WERE RESOLVED BY COMBINING, NOT BY CHOOSING -- see the merge notes below; each is a case where this branch and the ported mangos_two work had fixed different halves of the same function. Terrain.hpp TileIndex: two defects at opposite ends of the same axis. The port floors so a coordinate in (-1, 0) stops answering as tile 0; this branch folds the exact far edge so 8192 stops answering as tile 64. Both are kept -- flooring leaves an exact 8192 exactly 8192, and the fold's guard is false for every negative index. WmoModel: this branch extracted the per-group liquid lookup into GroupLiquidAt and lost all three of the port's guards in the move -- the heights/tiles size check that H() indexes on, the positively-stated range test that lets a NaN fall out, and the ordering that ranges in float BEFORE converting to int. The extraction is kept and the guards are back inside it. GridMap: both trees implemented the collision disables. The cached read is kept and the direct DisableMgr::IsVMAPDisabledFor calls are not: that function reaches its container through operator[], which INSERTS when the type is absent, so asking it from a map-update thread is a write racing `.reload disables` on the world thread. Line of sight now routes through NearestHitFraction so the disable is applied in one place and the sight test cannot disagree with the hit position, and `ignore` travels with it. Also: FusedTerrain's pin counter is uint32_t in this tree, not the int16_t the ported comment claimed, so the saturation ceiling and that comment were both corrected. Accelerators carried two identical Bvh::Adopt definitions after the merge -- a duplicate symbol git reported no conflict for -- and TileSerializer carried the same index check twice under two names; one of each was removed. The Eluna submodule pointer is deliberately NOT moved by this commit. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MSG_CORPSE_QUERY resolves a corpse left in an instance to that instance's entrance, taken from Map.dbc. It checked only that CorpseMapID was a real map and not that the entrance coordinates existed, so a map whose Corpse_0/Corpse_1 are zero was answered with (0, 0) on the entrance continent. ObjectMgr::GetClosestGraveYard already screens exactly this case -- the guard here is the same condition, and its absence is why the two disagreed. The player's own position stayed correct, because the graveyard path has the guard; only the arrow pointed somewhere else, which is what makes it read as a client bug. On Kalimdor the origin is the Mulgore/Stonetalon/Barrens junction, about 1000 yards from the Stonetalon Mountains graveyard. Reported as "died in Ragefire Chasm, corpse showed in Stonetalon Mountains" -- RFC's own entry is intact (CorpseMapID 1, entrance 1816.76 / -4423.37, verified against the extracted Map.dbc), so a data set where it is not is what produces that report. In a clean 2.4.3 Map.dbc the maps with a valid CorpseMapID and zero coordinates are 533 Naxxramas, 550/552/553/554 the Tempest Keep wings, and 598 Sunwell5Man. With the guard, those fall back to the corpse's real position inside the instance instead of a fabricated one. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reported as: a sheeped mob breaks and attacks while STILL polymorphed, with the debuff visibly counting down on it. CreatureAI::HandleMovementOnAttackStart opened with an unconditional MotionMaster::MovementExpired(false). That pops whatever generator is on top -- including the ConfusedMovementGenerator that Unit::SetConfused pushed for a polymorph. Its Finalize clears UNIT_STAT_CONFUSED, the MoveChase two lines down pushes a chase, and nothing anywhere in that path touches the aura. So the holder stays applied: the transform keeps the sheep model and the duration keeps ticking on the client, while the creature walks over and melees. Nothing else guards it. MotionMaster::MoveChase takes no crowd-control state into account at all; MoveFollow, immediately below it in the same file, does check UNIT_STAT_LOST_CONTROL -- and even that mask is FLEEING|CONTROLLED and does not include CONFUSED. The pop was the whole mechanism. This line is local to this core: the same function in 00_mangos_zero (line 551) and 02_mangos_two (line 239) goes straight to the IsCombatMovement test with no pop, which is why the report does not reproduce there. Removing it restores that shape. Damage still breaks polymorph normally -- Unit::DealDamage removes the whole aura holder for any spell carrying AURA_INTERRUPT_FLAG_DAMAGE, and that path is untouched. Not established, and deliberately not guessed at here: which call actually reaches AttackStart on a polymorphed creature. A DoT tick, an AoE or an assist call all fit "after a few seconds the sheep runs at you". Whatever it is, it no longer strips the crowd control on the way through. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reported as: cannot cast from the shore or off a dock, you have to wade two yards in
before the bobber will go out.
Spell::EffectTransmitted tested the water at the BOBBER's x/y but at the CASTER's z:
IsInWater(fx, fy, m_caster->Where().Z() + 1.f, &liqData)
TerrainInfo::IsInWater is true only for IN_WATER|UNDER_WATER, and getLiquidStatus
grants those only when delta = (surface - z) * 10 > 0 -- that is, when the probe height
is BELOW the surface. Feeding it the caster's own elevation therefore asks whether the
FISHERMAN is submerged, not whether there is water where the bobber lands. Standing on
a bank or a dock he is above the surface, the status comes back ABOVE_WATER, and the
cast is refused. Wading in until he is knee- or waist-deep drops his z below the
surface and it starts working, which is exactly the shape of the report.
IsAboveWater asks it the right way round: true when there is liquid in that column and
its surface lies below the probe. That holds from a dock, from a bank and in the
shallows; no liquid at all still fails; and it returns the surface height, which is
where the bobber floats anyway, so the separate GridMapLiquidData is no longer needed.
00_mangos_zero has the identical defect. 02_mangos_two does not -- it already asks via
IsAboveWater, which is where this shape comes from.
02's full form probes at Z + waistHeight + 0.5f with waistHeight from
GetModelMidpoint(GetDisplayId()), and adds SPELL_FAILED_ONLY_ABOVEWATER for a caster
submerged past the waist. That is deliberately NOT ported: GetModelMidpoint reads
CreatureModelData.dbc, which this core does not load at all, and this core's
CreatureDisplayInfoEntry does not even read the modelID column that would reach it.
Adding a DBC load and changing a format string to gain a more anatomically exact
threshold is a much larger and riskier change than the defect warrants. The fixed
+1 yard lift closes the defect; only the "too deep to fish" cutoff stays coarse.
NOT COMPILED -- no build was run, per the standing rule that none is invoked
without an explicit request.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every claim below was checked against the tree before being changed, and the ones that still hold were left alone. `src/tools/Extractor_projects` was listed as a submodule. It is not one -- .gitmodules has exactly four entries (dep, src/realmd, src/modules/SD3, src/modules/Eluna, win), and src/CMakeLists.txt records that the submodule was removed and the in-tree baker replaced it. Anyone following the old line would look for a submodule that is not there and miss that src/tools/extractor/ is ordinary source. `BUILD_TOOLS=0`: the paragraph said the asset extractors "are not used". They are: src/CMakeLists.txt adds tools/extractor UNCONDITIONALLY, with the reason written in place -- the .tile files it bakes are what src/shared/terrain reads at runtime. The flag now only chooses whether the executable is built. The Movemap generator it named is gone too; navmesh generation is inside the extractor. The documented CI flags did not match the workflow. CI passes -DBUILD_TOOLS=1 (not 0) plus -DWITH_TESTS=1 -DWITH_NET_TESTS=0, and configures with Ninja via cmake -S/-B. The old block mattered more than a stale line usually does: read literally, it said extractor changes are not compiled by CI, when in fact every CI run builds them. Added, because both are deploy-time traps that cost real time when hit: the tile format version means a stale bake is refused outright, so the extractor must be rebuilt AND re-run; and the extractor is what writes the data.manifest that World::VerifyDataIntegrity reads, so a boot-time "unverified" notice means the bake predates the manifest rather than that something is broken. The `mkdir _build` invocation was replaced with an out-of-tree one, matching the standing rule that build artefacts never land in the working tree. Left alone deliberately: the architecture note naming Master::WorldLoop as the world heartbeat. That member still exists and still is the heartbeat -- the world/map tick split landed inside World and MapManager, not as a second loop in Master -- so the note is not stale, and rewriting it on the strength of a nearby change would be a guess. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ParseIdList kept only `id > 0`, so map 0 -- Eastern Kingdoms -- was silently dropped from LoadAllGridsOnMaps, the one setting whose whole purpose is naming maps to force-load. An administrator who set it to "0" got no grids loaded and no diagnostic. Widening to `id >= 0` alone would not do: std::strtol returns 0 both for the token "0" and for a token that is not a number at all, so every malformed entry would have become map 0. The conversion is now checked through its end pointer -- it must consume something, and must consume all of it -- which accepts 0 while still rejecting a malformed or negative token, and is stricter than the old parser about trailing junk. Found in the review of mangosfour/Server#78 (src/shared/Utilities/IdList.h), where it was reported and left unfixed. Verified present here before changing: this core carries the identical predicate, and LoadAllGridsOnMaps is a documented setting in mangosd.conf.dist.in. NOT COMPILED -- no build was run, per the standing rule that none is invoked without an explicit request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MANGOS_WORLD_VER 2026071300 -> 2026080900. It reaches mangosd.conf.dist.in as @MANGOS_WORLD_VER@ and SystemConfig.h.in as MANGOSD_CONFIG_VERSION, which World::LoadConfigSettings compares against the running config's ConfVersion and warns on when it is lower. This branch earns the bump three times over, and the warning is the only thing that tells an administrator to look: MapUpdateInterval's default changed from 100 to 10 -- a ten-fold change in map update cadence, arriving with the world/map tick split. An existing conf keeps its own value, so without the warning the change is invisible until someone reads the diff. Two keys added: DataIntegrityCheck (start-up SHA-256 verification of the baked data set) and MovementPacketDelay. Seven keys removed from both the template and the code: MinHonorKills, MaintenanceDay, Network.Threads, Network.OutKBuff, Network.OutUBuff, Network.TcpNodelay and Eluna.CompatibilityMode. An existing conf that still lists them starts fine -- unknown keys are ignored -- but they no longer do anything. Only MANGOS_WORLD_VER is moved. MANGOS_REALM_VER and MANGOS_AHBOT_VER stay where they are: realmd.conf.dist.in and ahbot.conf.dist.in are untouched on this branch, and bumping a version whose file did not change would train administrators to dismiss the warning. NOT COMPILED -- no build was run for this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Static analysis flagged `text.find(prefix) == 0` as a prefix test better written with string::starts_with. The reading is right; the remedy is not available here, and the alternative the same report offers would quietly weaken the test. starts_with is C++20. This project builds -std=c++17 with GNU extensions off, so applying the suggestion literally breaks GCC, Clang and MSVC alike. `rfind(p, 0) == 0` is the C++17 spelling of the same thing and is what the check now uses: it attempts position 0 and no other, so it does not scan on failure either, which was the performance half of the finding. The other half matters more. The report suggests comparing against npos "if the intention is to check for absence" -- but `== 0` here is not a presence test at all. `dbc/Map.dbc` sorts before `tiles/...`, and the manifest is required to be sorted because that is the layout `sha256sum -c` reads; the assertion is that this line is FIRST. The check immediately below it is the one that only asks for presence, and already uses `!= npos`. Rewriting the two to match would have deleted the only sort-order assertion in the test while leaving it green. A comment now says which of the two each check is, so the next reader does not "unify" them. Behaviour is unchanged; this is the same predicate written to state its intent. NOT COMPILED -- no build was run for this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )