[Movement] Reject packed zero-length waypoint segments - #355
Conversation
Preflight smoothed waypoint chunks against the exact 4.3.4 packed linear-path reconstruction, using the mover position that MoveSplineInit will install as the first control point. Unsafe chunks fall back to existing per-leg routing so packet and server spline state remain aligned. This guard is intentionally scoped to the smoothed waypoint producer; generic routed single-leg paths remain outside it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: cd0fc8a7a5
ℹ️ 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".
| // PacketBuilder::WriteLinearPath sends the destination directly and every | ||
| // intermediate point as an offset from this midpoint. Compare the path the | ||
| // client reconstructs, including both uncompressed endpoint boundaries. | ||
| const Geometry::Vector3 midpoint = (launchPosition + points.back()) / 2.0f; |
There was a problem hiding this comment.
When points[1] differs from the launch position, this validates against the wrong quantization grid. In the inspected PacketBuilder::WriteLinearPath implementation (src/game/movement/packet_builder.cpp:142-155), real_path starts at spline.getPoint(1) and the midpoint uses real_path[0], while linear spline initialization copies controls beginning at index 0 (src/game/movement/spline.cpp:301-319), so the actual midpoint is (points[1] + points.back()) / 2, not (launchPosition + points.back()) / 2. Consequently an unsafe path can pass this check and still reconstruct with adjacent identical points on the client, defeating the protection this commit adds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rechecked against the actual spline data flow and kept launchPosition. MoveSplineInit::Launch replaces args.path[0] with real_position before init_spline; InitCatmullRom then copies args.path[0] into spline control point 1, which is exactly where PacketBuilder starts real_path. Thus getPoint(1) equals the launch position, not the pre-Launch points[1]. The projected-first-point regression covers this distinction, and an independent Claude trace reached the same result. Using points[1] here would model geometry that never reaches the wire.
| if (m_segment.size() <= 1 || m_legPoints.size() < 2 || | ||
| !IsWaypointSmoothingWireSafe(m_legPoints, launchPosition)) | ||
| { | ||
| ClearSegment(); | ||
| m_legPoints.clear(); |
There was a problem hiding this comment.
When the collapsing pair belongs to the routed geometry of the first waypoint leg rather than a seam between waypoint legs, clearing m_legPoints does not prevent the bad packet. PrepareMove then returns an intent without an explicit path, and MotionDriver::LayLeg reruns the route and passes query->Points() directly to MoveSplineInit (src/game/MotionGenerators/MotionDriver.cpp:166-200) without this wire-safety check or even the smoothing point filter. A route containing two corners that quantize identically therefore falls back to substantially the same unsafe packed spline and can still trigger the client failure this branch is intended to avoid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c73f891. MotionDriver now applies the packed-wire guard to query->Points() with the exact mover start before laying the fallback route, and reports the leg blocked when client reconstruction would collapse a segment. The isolated Release build and all 13 CTest entries pass.
There was a problem hiding this comment.
Follow-up in 2c831ff replaces the blocked-leg response from c73f891. Unsafe routed fallbacks are now sanitized at the exact launch packing grid: only client-side duplicate vertices are removed, distinct navmesh corners and the destination remain, and the route is launched rather than retried forever.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c73f891812
ℹ️ 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".
| // back through this general router. Apply the same packed-wire guard to | ||
| // that routed geometry so the fallback cannot recreate the collapsed | ||
| // client-side segment that was just rejected. | ||
| if (!IsWaypointSmoothingWireSafe(query->Points(), start)) |
There was a problem hiding this comment.
When a routed leg is relaid while the previous spline is still running, such as after target drift or a speed change, start is taken from owner.Where(), but MoveSplineInit::Launch replaces the first control point with move_spline.ComputePosition() instead (MoveSplineInit.cpp:130-153). Passing the stale stored position here means the first reconstructed packed point can coincide with the actual computed launch position while differing from start, so this check accepts the initial zero-length client segment it is intended to reject.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c831ff. MoveSplineInit now exposes the launch position through the same resolver used by Launch itself, including live-spline, vehicle-seat and pending-stop cases. MotionDriver sanitizes the route against that exact packing grid and removes only packed duplicate vertices. The full Release build and all 13 CTest entries pass.
| // client reconstructs, including both uncompressed endpoint boundaries. | ||
| const Geometry::Vector3 midpoint = (launchPosition + points.back()) / 2.0f; | ||
| Geometry::Vector3 previous = launchPosition; | ||
| for (size_t i = 1; i + 1 < points.size(); ++i) |
There was a problem hiding this comment.
SplineBase::InitLinear copies the controls starting at spline point 0, while PacketBuilder::WriteLinearPath sets real_path to spline point 1 and begins packing at real_path[1] (packet_builder.cpp:142-155), making points[2] the first reconstructed vertex; points[1] only contributes to the midpoint and is not itself sent as a path vertex. Starting this loop at index 1 invents a client-side vertex, so when decoded points[2] coincides with the launch position but decoded points[1] does not, the function accepts the actual zero-length initial segment; it can also reject safe paths based solely on the omitted point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rechecked against the actual index mapping and retained source index 1. InitCatmullRom copies source path[0] to spline point 1; PacketBuilder sets real_path to spline point 1, so its real_path[1] (the first packed vertex) is source path[1]. Starting at source index 2 would skip the first transmitted packed point. Commit 2c831ff adds a dedicated quarter-yard regression for this mapping; the full Release build and all 13 CTest entries pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c831ff811
ℹ️ 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".
| // MoveSplineInit replaces the path's first routed point with this exact mover | ||
| // position before PacketBuilder derives the packed-path midpoint. Preserve it | ||
| // while the loop advances `start` through later waypoint legs. | ||
| const Motion::Vector3 launchPosition = frame.MoverPosition(creature); |
There was a problem hiding this comment.
When a waypoint-controlled creature is seated on a vehicle, frame.MoverPosition(creature) returns Where(), but MoveSplineInit::Launch replaces path[0] with the vehicle seat position via ResolveLaunchLocation. This validates the explicit smoothed path against a different midpoint/quantization grid from the one sent, so an adjacent packed pair can still collapse and trigger the client corruption this guard is intended to prevent. Although the new ResolveLaunchPosition() call fixes routed fallback paths, the explicit smoothing branch still bypasses it; obtain the launch position using the same resolver before both routing and IsWaypointSmoothingWireSafe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in af80d7a. ResolveLaunchPosition is now a side-effect-free static selector shared by MoveSplineInit::Launch, MotionDriver routed fallbacks, and WaypointMovementGenerator explicit smoothing. Vehicle-seat, live-spline and pending-stop starts therefore use the same first wire point and packing grid before routing and validation. The full Release build and all 13 CTest entries pass.
…vent bunny-hop on missing navmesh tiles (mangosthree#355)
2 participants
Summary
Verification
mangosd.git diff --checkpassed.Dependency
This is the movement prerequisite for the separate Cata Warden redesign PR and should be squash-merged first.
This change is Reviewable.