Fix crash and performance issues in _dirs command - #209
Open
nschimme wants to merge 14 commits into
Open
Conversation
- Fix heap-use-after-free in MapData::shortestPathSearch by ensuring room handles are copied, not referenced, before vector modification. - Optimize shortestPathSearch by passing search node vector by const-reference to recipients, avoiding redundant copying. - Fix assertion failure in ShortestPathEmitter that prevented matching the starting room (distance 0).
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideRefactors the shortest path search to use ID-based nodes and explicit result objects, fixes a crash caused by dangling references and over‑strict assertions in the _dirs command, optimizes pathfinding data structures, and adds a dedicated unit test binary for shortest path behavior. Sequence diagram for updated _dirs shortest path search and emissionsequenceDiagram
actor User
participant Parser
participant MapData
participant Map
participant ShortestPathEmitter
User ->> Parser: enter _dirs command
Parser ->> MapData: shortestPathSearch(origin, filter, recipient, max_hits, max_dist)
activate MapData
MapData ->> Map: getRoomHandle(origin.id)
Map -->> MapData: RoomHandle origin
loop Dijkstra_like_search
MapData ->> Map: getRoomHandle(current_room_id)
Map -->> MapData: RoomHandle current
MapData ->> Map: getRoomHandle(neighbor_room_id)
Map -->> MapData: RoomHandle neighbor
MapData ->> MapData: compute cost and update sp_nodes
alt neighbor matches filter
MapData ->> ShortestPathEmitter: receiveShortestPath(Map map, ShortestPathResult result)
end
end
deactivate MapData
activate ShortestPathEmitter
ShortestPathEmitter ->> Map: getRoomHandle(result.id)
Map -->> ShortestPathEmitter: RoomHandle dest
ShortestPathEmitter ->> Parser: sendToUser("Distance X: name")
ShortestPathEmitter ->> Parser: sendToUser("dirs: compressed_path")
deactivate ShortestPathEmitter
Parser -->> User: display distance and directions
Updated class diagram for shortest path result and recipient hierarchyclassDiagram
class Map
class RoomId {
+uint32_t asUint32()
}
class ExternalRoomId {
+uint32_t asUint32()
}
class ServerRoomId {
+uint32_t asUint32()
}
class ShortestPathResult {
+RoomId id
+double dist
+vector~ExitDirEnum~ path
}
class ShortestPathRecipient {
<<interface>>
+~ShortestPathRecipient()
+void receiveShortestPath(const Map &map, ShortestPathResult result)
-virtual void virt_receiveShortestPath(const Map &map, ShortestPathResult result)
}
class ShortestPathEmitter {
+~ShortestPathEmitter()
-void virt_receiveShortestPath(const Map &map, ShortestPathResult result)
-Parser parser
}
class Parser
ShortestPathEmitter --|> ShortestPathRecipient
ShortestPathRecipient ..> Map : uses
ShortestPathRecipient ..> ShortestPathResult : uses
ShortestPathResult --> RoomId : identifies_room
Parser ..> ShortestPathEmitter : owns_or_uses
Map ..> RoomId : identifies_rooms
ExternalRoomId --|> RoomId
ServerRoomId --|> RoomId
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Changing
ShortestPathRecipient::virt_receiveShortestPathto takeconst QVector<SPNode>&conflicts with the comment and likely behavior inShortestPathEmitterwherespnodeis modified; if mutation ofSPNodeinstances is required, consider taking a non-const reference to the vector or redesigning that usage to avoid mutating through a const container.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `ShortestPathRecipient::virt_receiveShortestPath` to take `const QVector<SPNode>&` conflicts with the comment and likely behavior in `ShortestPathEmitter` where `spnode` is modified; if mutation of `SPNode` instances is required, consider taking a non-const reference to the vector or redesigning that usage to avoid mutating through a const container.
## Individual Comments
### Comment 1
<location path="src/parser/abstractparser.cpp" line_range="395-400" />
<code_context>
private:
- void virt_receiveShortestPath(QVector<SPNode> spnodes, const int endpoint) final
+ void virt_receiveShortestPath(const QVector<SPNode> &spnodes, const int endpoint) final
{
- assert(0 < endpoint && endpoint < spnodes.size());
+ assert(0 <= endpoint && endpoint < spnodes.size());
// Caution: spnode is modified here.
const SPNode *spnode = &spnodes[endpoint];
</code_context>
<issue_to_address>
**issue (bug_risk):** Signature now takes a const QVector, but the implementation comment indicates the elements are modified.
If `*spnode` is still mutated, this change will either not compile or force const_casts, which is confusing for callers. Either keep the parameter non-const if mutation is intended, or refactor the implementation to work on a copy so the parameter can safely remain const.
</issue_to_address>
### Comment 2
<location path="src/mapdata/shortestpath.h" line_range="29-32" />
<code_context>
private:
- virtual void virt_receiveShortestPath(QVector<SPNode> spnodes, int endpoint) = 0;
+ virtual void virt_receiveShortestPath(const QVector<SPNode> &spnodes, int endpoint) = 0;
public:
- void receiveShortestPath(QVector<SPNode> spnodes, const int endpoint)
+ void receiveShortestPath(const QVector<SPNode> &spnodes, const int endpoint)
{
virt_receiveShortestPath(spnodes, endpoint);
</code_context>
<issue_to_address>
**question (bug_risk):** Changing from pass-by-value to const-reference tightens lifetime expectations for `spnodes`.
Previously, passing `QVector<SPNode>` by value let implementations own and freely retain or modify their copy. With `const QVector<SPNode>&`, the data is now non-owning and only valid for the duration of the call. Any implementation that stores references/pointers into `spnodes` or assumes it outlives the function may now be unsafe. Please verify existing implementers of this interface don’t rely on longer lifetimes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #209 +/- ##
==========================================
- Coverage 25.40% 25.40% -0.01%
==========================================
Files 519 519
Lines 43102 43110 +8
Branches 4698 4705 +7
==========================================
Hits 10952 10952
- Misses 32150 32158 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing
ShortestPathRecipient::receiveShortestPathandvirt_receiveShortestPathto takeconst QVector<SPNode> &introduces a lifetime requirement on the caller; consider enforcing this more explicitly (e.g., by passing a value, a shared container, or a view/span type) or documenting that the vector must outlive the recipient callback to avoid future dangling references. - In
MapData::shortestPathSearch, now thatthisris intentionally a copy, you may want to declare it asconst RoomHandle thisr = ...;to make its immutability explicit and prevent accidental modifications that could obscure the reason for copying.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `ShortestPathRecipient::receiveShortestPath` and `virt_receiveShortestPath` to take `const QVector<SPNode> &` introduces a lifetime requirement on the caller; consider enforcing this more explicitly (e.g., by passing a value, a shared container, or a view/span type) or documenting that the vector must outlive the recipient callback to avoid future dangling references.
- In `MapData::shortestPathSearch`, now that `thisr` is intentionally a copy, you may want to declare it as `const RoomHandle thisr = ...;` to make its immutability explicit and prevent accidental modifications that could obscure the reason for copying.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId instead of references to QVector elements that can be invalidated. - Fix crash in ShortestPathEmitter by allowing matches for the current room (relaxing assertion for distance-0 results). - Optimize shortest path search by using std::vector, std::priority_queue, and a flat array for the visited set. - Modernize ShortestPathRecipient interface to pass results by const ref. - Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId instead of references to elements in a reallocating vector. - Resolve assertion failure in ShortestPathEmitter by allowing results for the current room (relaxed assertion to endpoint >= 0). - Optimize shortest path search: - Replaced QVector/QSet with std::vector and flat uint8_t array. - Implemented proper min-heap with std::priority_queue. - Updated search logic to skip reporting the origin room (index 0). - Extract magical cost constants into named constexpr variables. - Fix -Wsign-conversion and ensure clang-format compliance. - Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId instead of references to QVector elements that can be invalidated. - Fix crash in ShortestPathEmitter by relaxing assertion for distance-0 results, allowing matches for the current room. - Modernize shortest path search logic: - Replaced QVector/QSet with std::vector and flat uint8_t array. - Implemented proper min-heap with std::priority_queue. - Updated search to intentionally skip reporting the origin room. - Improved maintainability of movement costs: - Extracted costs into named constexpr variables. - Refactored terrain_cost() to use the X_CASE macro pattern. - Ensure -Wsign-conversion and clang-format compliance. - Add missing <unordered_set> include in groupwidget.cpp.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The use of
visitedas astd::vector<uint8_t>(map.getRoomsCount() + 1)keyed byRoomId::asUint32()is a bit opaque—consider adding a brief comment or helper to document the invariant betweenRoomIdvalues andgetRoomsCount()(and why+1is safe) to prevent future off‑by‑one or out‑of‑range assumptions. - There is a lot of casting between
intandstd::size_tfor shortest‑path node indices (e.g., insp_nodes,endpoint, andparent), which makes the code harder to reason about; consider standardizing on an unsigned index type (or a dedicated alias) and using a sentinel/optional instead of-1so you can drop the repeated casts and signed/unsigned checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The use of `visited` as a `std::vector<uint8_t>(map.getRoomsCount() + 1)` keyed by `RoomId::asUint32()` is a bit opaque—consider adding a brief comment or helper to document the invariant between `RoomId` values and `getRoomsCount()` (and why `+1` is safe) to prevent future off‑by‑one or out‑of‑range assumptions.
- There is a lot of casting between `int` and `std::size_t` for shortest‑path node indices (e.g., in `sp_nodes`, `endpoint`, and `parent`), which makes the code harder to reason about; consider standardizing on an unsigned index type (or a dedicated alias) and using a sentinel/optional instead of `-1` so you can drop the repeated casts and signed/unsigned checks.
## Individual Comments
### Comment 1
<location path="src/mapdata/shortestpath.cpp" line_range="118-127" />
<code_context>
+ std::vector<uint8_t> visited(map.getRoomsCount() + 1, 0);
</code_context>
<issue_to_address>
**issue (bug_risk):** RoomId-to-index mapping for `visited` assumes a tight ID range and can break visited semantics for out-of-range IDs.
This relies on `room_id.asUint32() <= map.getRoomsCount()` (plus the `+1` margin). If `RoomId` values are sparse or can exceed `getRoomsCount()`, any `room_id.asUint32() >= visited.size()` will never be tracked as visited, so those rooms may be re-expanded indefinitely and Dijkstra’s finalization invariant breaks. Unless the tight ID range is guaranteed, consider either sizing `visited` from the maximum internal ID, reverting to a set-based representation, or explicitly handling out-of-range IDs (e.g., early-continue with a clear policy) instead of silently skipping them.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId instead of references to QVector elements that can be invalidated. - Resolve assertion failure in ShortestPathEmitter by allowing results for the current room (relaxed assertion to endpoint < size). - Optimize shortest path search: - Switched from breadth-first search to Dijkstra with std::priority_queue. - Replaced QVector/QSet with std::vector and the idiomatic RoomIdSet. - Updated search logic to skip reporting the origin room (spidx != 0). - Standardize indices using SPNodeIdx (uint32_t) and INVALID_SPNODE_IDX. - extract magical cost constants into named constexpr variables. - Refactor terrain_cost() to use the X_CASE macro pattern. - Ensure -Wsign-conversion and clang-format compliance. - Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId
instead of references to elements in a reallocating vector.
- Resolve assertion failure in ShortestPathEmitter by allowing results
for the current room (relaxed assertion to endpoint < size).
- Optimize shortest path search:
- Switched from breadth-first search to Dijkstra with std::priority_queue.
- Replaced QVector/QSet with std::vector and the idiomatic RoomIdSet.
- Performed internal path reconstruction to pass results via
ShortestPathResult, making the recipient interface safer.
- Updated search logic to skip reporting the origin room (spidx != 0).
- Improve code quality and maintainability:
- Standardized indices using SPNodeIdx (uint32_t) and INVALID_SPNODE_IDX.
- Extracted magical cost constants into named constexpr variables.
- Refactored terrain_cost() to use the X_CASE macro pattern.
- Ensure -Wsign-conversion and clang-format compliance.
- Add unit tests for shortestPathSearch in TestShortestPath.cpp.
- Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId
instead of references to QVector elements that can be invalidated.
- Resolve assertion failure in ShortestPathEmitter by allowing results
for the current room (relaxed assertion to endpoint < size).
- Optimize shortest path search:
- Switched from breadth-first search to Dijkstra with std::priority_queue.
- Replaced QVector/QSet with std::vector and the idiomatic RoomIdSet.
- Performed internal path reconstruction to pass results via
ShortestPathResult, making the recipient interface safer and more performant.
- Updated search logic to skip reporting the origin room (spidx != 0).
- Improve code quality and maintainability:
- Standardized indices using SPNodeIdx (uint32_t) and INVALID_SPNODE_IDX.
- Extracted magical cost constants into named constexpr variables.
- Refactored terrain_cost() to use the X_CASE macro pattern.
- Ensure -Wsign-conversion and clang-format compliance.
- Add comprehensive unit tests for shortestPathSearch in TestShortestPath.cpp.
- Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId
instead of references to QVector elements that can be invalidated.
- Resolve assertion failure in ShortestPathEmitter by allowing results
for the current room (relaxed assertion to endpoint < size).
- Optimize shortest path search:
- Switched from breadth-first search to Dijkstra with std::priority_queue.
- Replaced QVector/QSet with std::vector and the idiomatic RoomIdSet.
- Performed internal path reconstruction to pass results via
ShortestPathResult, making the recipient interface safer and more performant.
- Improve code quality and maintainability:
- Standardized indices using SPNodeIdx (uint32_t) and INVALID_SPNODE_IDX.
- Extracted magical cost constants into named constexpr variables.
- Refactored terrain_cost() to use the X_CASE macro pattern.
- Ensure -Wsign-conversion and clang-format compliance.
- Add comprehensive unit tests for shortestPathSearch in TestShortestPath.cpp.
- Add missing <unordered_set> include in groupwidget.cpp.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider using
std::size_t(or the container’ssize_type) forSPNodeIdxinstead ofstd::uint32_t, or at least adding a safeguard, to avoid potential truncation/overflow if the number of path nodes grows beyond 2^32−1. - In
MapData::shortestPathSearch, you now duplicate path reconstruction logic that previously lived in the recipient; if future consumers need different path projections (e.g., including nodes, not just directions), it might be worth centralizing or exposing a helper to avoid reimplementing the reconstruction in multiple places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using `std::size_t` (or the container’s `size_type`) for `SPNodeIdx` instead of `std::uint32_t`, or at least adding a safeguard, to avoid potential truncation/overflow if the number of path nodes grows beyond 2^32−1.
- In `MapData::shortestPathSearch`, you now duplicate path reconstruction logic that previously lived in the recipient; if future consumers need different path projections (e.g., including nodes, not just directions), it might be worth centralizing or exposing a helper to avoid reimplementing the reconstruction in multiple places.
## Individual Comments
### Comment 1
<location path="src/parser/abstractparser.cpp" line_range="395-398" />
<code_context>
private:
- void virt_receiveShortestPath(QVector<SPNode> spnodes, const int endpoint) final
+ void virt_receiveShortestPath(const Map &map, ShortestPathResult result) final
{
- assert(0 < endpoint && endpoint < spnodes.size());
+ const auto room = map.getRoomHandle(result.id);
+ const auto name = room ? room.getName() : RoomName{};
- // Caution: spnode is modified here.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Swallowing a missing room silently could make underlying data issues harder to detect.
Since virt_receiveShortestPath now falls back to an empty RoomName when map.getRoomHandle(result.id) fails, any missing room will appear as a "normal" case. Because the search code should only emit results for existing rooms, hitting this path likely indicates a data inconsistency. Consider asserting or logging a warning instead of silently using an empty name, so these issues are visible during development and debugging.
Suggested implementation:
```cpp
void virt_receiveShortestPath(const Map &map, ShortestPathResult result) final
{
const auto room = map.getRoomHandle(result.id);
if (!room) {
qWarning() << "ShortestPathEmitter::virt_receiveShortestPath received result for non-existent room id"
<< result.id;
return;
}
const auto name = room.getName();
parser.sendToUser(SendToUserSourceEnum::FromMMapper,
"Distance " + std::to_string(result.dist) + ": " + name.toStdStringUtf8()
+ "\n");
```
If `qWarning` is not already used in this file, ensure the appropriate Qt debug header is included, e.g. `#include <QDebug>`, near the top of `src/parser/abstractparser.cpp`. This will make the missing-room cases visible during development and debugging while avoiding undefined behavior in release builds.
</issue_to_address>
### Comment 2
<location path="tests/TestShortestPath.cpp" line_range="124-129" />
<code_context>
+ QVERIFY(optFilter.has_value());
+ TestRecipient recipient;
+
+ MapData::shortestPathSearch(r1_handle, recipient, *optFilter, 1, 0);
+
+ QCOMPARE(recipient.results.size(), 1ULL);
+ QCOMPARE(recipient.results[0].path.size(), 2ULL);
+ QCOMPARE(recipient.results[0].path[0], ExitDirEnum::NORTH);
+ QCOMPARE(recipient.results[0].path[1], ExitDirEnum::EAST);
+
+ // Test including start room
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering `max_dist` cutoff behavior in `shortestPathSearch`
The updated code adds explicit `max_dist` handling, but this test always uses `max_dist = 0` (no limit), so that code path isn’t covered. Please add a case with a finite `max_dist` that permits the first step but not the second (or vice versa), and assert that farther rooms are excluded. This will protect against regressions in cost accumulation and the `max_dist` early-return behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+124
to
+129
| MapData::shortestPathSearch(r1_handle, recipient, *optFilter, 1, 0); | ||
|
|
||
| QCOMPARE(recipient.results.size(), 1ULL); | ||
| QCOMPARE(recipient.results[0].path.size(), 2ULL); | ||
| QCOMPARE(recipient.results[0].path[0], ExitDirEnum::NORTH); | ||
| QCOMPARE(recipient.results[0].path[1], ExitDirEnum::EAST); |
There was a problem hiding this comment.
suggestion (testing): Add a test covering max_dist cutoff behavior in shortestPathSearch
The updated code adds explicit max_dist handling, but this test always uses max_dist = 0 (no limit), so that code path isn’t covered. Please add a case with a finite max_dist that permits the first step but not the second (or vice versa), and assert that farther rooms are excluded. This will protect against regressions in cost accumulation and the max_dist early-return behavior.
- Fix heap-use-after-free in MapData::shortestPathSearch by using RoomId
instead of references to QVector elements that can be invalidated.
- Resolve assertion failure in ShortestPathEmitter by allowing results
for the current room (relaxed assertion to endpoint < size).
- Optimize shortest path search:
- Switched from breadth-first search to Dijkstra with std::priority_queue.
- Replaced QVector/QSet with std::vector and the idiomatic RoomIdSet.
- Performed internal path reconstruction to pass results via
ShortestPathResult, making the recipient interface safer and more performant.
- Explicitly handle max_dist cutoff and ensure correct cost accumulation.
- Improve code quality and maintainability:
- Standardized indices using std::size_t and INVALID_SPNODE_IDX.
- Extracted magical cost constants into named constexpr variables.
- Refactored terrain_cost() to use the X_CASE macro pattern.
- Added warning logging to ShortestPathEmitter for data inconsistencies.
- Ensure -Wsign-conversion and clang-format compliance.
- Add comprehensive unit tests (including max_dist) in TestShortestPath.cpp.
- Add missing <unordered_set> include in groupwidget.cpp.
Resolved a heap-use-after-free in MapData::shortestPathSearch caused by holding a reference to a QVector element while the vector reallocated. Changes: - Re-implemented shortest path search using Dijkstra's algorithm with std::priority_queue (min-heap) for efficiency. - Decoupled path reconstruction from the recipient interface; search now returns a self-contained ShortestPathResult. - Standardized use of RoomId in search nodes to minimize memory footprint and avoid dangling handles. - Replaced QVector/QSet with std::vector and RoomIdSet for core search logic. - Fixed a crash in ShortestPathEmitter assertion when matches were found in the starting room (distance 0). - Included <QDebug> in roomid.h to fix build failures across platforms caused by incomplete type deduction in Qt 6 operator overloads. - Added comprehensive regression tests in tests/TestShortestPath.cpp. Fixes MUME#519
- Resolve heap-use-after-free in MapData::shortestPathSearch by avoiding dangling references during vector reallocation. - Implement Dijkstra's algorithm with std::priority_queue for robust pathfinding. - Include the starting room in search results if filter criteria are met. - Add regression tests in tests/TestShortestPath.cpp. - Fix Qt 6 build issues by including <QDebug> in src/map/roomid.h. - Fix missing <unordered_set> include in src/group/groupwidget.cpp.
Owner
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Apr 18, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
MapData::shortestPathSearch, exits pointing to non-existent rooms are now silently skipped; consider keeping at least aqWarning()here to preserve diagnostics for corrupted or out-of-sync maps while still avoiding crashes. - The fixed
INITIAL_NODES_CAPACITYof 1024 for the shortest-path node vector may be suboptimal for very small or very large maps; you might want to derive this from map size or make it configurable to avoid over-allocation or repeated growth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MapData::shortestPathSearch`, exits pointing to non-existent rooms are now silently skipped; consider keeping at least a `qWarning()` here to preserve diagnostics for corrupted or out-of-sync maps while still avoiding crashes.
- The fixed `INITIAL_NODES_CAPACITY` of 1024 for the shortest-path node vector may be suboptimal for very small or very large maps; you might want to derive this from map size or make it configurable to avoid over-allocation or repeated growth.
## Individual Comments
### Comment 1
<location path="tests/TestShortestPath.cpp" line_range="19-28" />
<code_context>
+#include <QDebug>
+
std::ostream &operator<<(std::ostream &os, const RoomId id)
{
return os << "RoomId(" << id.value() << ")";
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert on the computed distances to validate movement cost handling, not just directions
This block depends on the indoor terrain cost (0.75 per step) and the new cost constants, but the test only verifies the direction sequence. Please also assert the computed distance (e.g. `QCOMPARE(recipient.results[0].dist, 1.5);` or a fuzzy FP check) so regressions in `terrain_cost` or `getLength` are caught.
</issue_to_address>Hi @nschimme! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+19
to
+28
| { | ||
| return Abbrev("terrain", 1); | ||
| } | ||
| Abbrev getParserCommandName(RoomLightEnum) | ||
| { | ||
| return Abbrev("light", 1); | ||
| } | ||
| Abbrev getParserCommandName(RoomRidableEnum) | ||
| { | ||
| return Abbrev("ridable", 1); |
There was a problem hiding this comment.
suggestion (testing): Also assert on the computed distances to validate movement cost handling, not just directions
This block depends on the indoor terrain cost (0.75 per step) and the new cost constants, but the test only verifies the direction sequence. Please also assert the computed distance (e.g. QCOMPARE(recipient.results[0].dist, 1.5); or a fuzzy FP check) so regressions in terrain_cost or getLength are caught.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
MapData::shortestPathSearch, whenmap.getRoomHandle(nextrId)fails the code now silentlycontinues; consider restoring at least aqWarning(without asserting) so unexpected broken exits remain diagnosable at runtime instead of failing quietly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MapData::shortestPathSearch`, when `map.getRoomHandle(nextrId)` fails the code now silently `continue`s; consider restoring at least a `qWarning` (without asserting) so unexpected broken exits remain diagnosable at runtime instead of failing quietly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Resolve heap-use-after-free in MapData::shortestPathSearch by refactoring to a safe Dijkstra implementation using indices and value-copies. - Support including the starting room in search results by relaxing emitter constraints. - Modernize terrain cost logic and search node management. - Address build failures on non-Linux platforms by ensuring QDebug is a complete type in roomid.h and providing out-of-line operator definitions. - Add missing <unordered_set> include in groupwidget.cpp.
- Fix heap-use-after-free in shortestPathSearch by refactoring to Dijkstra with value-copies.
- Parallelize room filtering in genericFind using thread_utils to reduce latency.
- Integrate parallel pre-filtering into shortestPathSearch to speed up target identification.
- Optimize path reconstruction to avoid std::reverse by pre-counting and back-filling.
- Add DECL_TIMER performance instrumentation.
- Support including the starting room in results ("dirs: (here)").
- Fix build issues on non-Linux platforms for RoomId and missing includes.
nschimme
force-pushed
the
master
branch
2 times, most recently
from
April 20, 2026 18:27
e8139f3 to
c119262
Compare
nschimme
force-pushed
the
master
branch
2 times, most recently
from
May 21, 2026 23:06
ae664f2 to
bcd8fca
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixed a crash in the
_dirscommand caused by a dangling reference and an overly restrictive assertion. Specifically:MapData::shortestPathSearch, changedthisrfrom a reference to a copy of theRoomHandleto prevent invalid memory access when the underlyingQVectorreallocates during apush_back.ShortestPathEmitter::virt_receiveShortestPath, updated the assertion to allow the current room (index 0) to be a valid search result.ShortestPathRecipientto take the search tree vector by constant reference, preventing expensive copies and detachment during pathfinding.Verified the fixes build and all existing tests pass.
PR created automatically by Jules for task 15391324769196083821 started by @nschimme
Summary by Sourcery
Improve robustness and performance of shortest path search and expose path results in a simpler format.
Bug Fixes:
Enhancements:
Tests: