-
Notifications
You must be signed in to change notification settings - Fork 4
Add changeStreetNLanes functions
#397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
+ Coverage 85.30% 85.59% +0.28%
==========================================
Files 53 53
Lines 5941 6067 +126
Branches 652 658 +6
==========================================
+ Hits 5068 5193 +125
- Misses 862 863 +1
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds functionality to dynamically change the number of lanes on streets in a road network simulation. The PR introduces methods to modify lane counts both at the Street level and RoadNetwork level (by ID or by name), with optional speed factor adjustments to simulate scenarios like construction zones.
Changes:
- Added
Street::changeNLanes()method to modify lane count with optional speed adjustment - Added
RoadNetwork::changeStreetNLanesById()andRoadNetwork::changeStreetNLanesByName()methods - Updated
dsf::queueanddsf::priority_queueclasses with move semantics and deleted copy operations - Added comprehensive test coverage for the new functionality
- Exposed the new methods to Python through pybind11 bindings
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dsf/mobility/Street.hpp | Added changeNLanes() method declaration and private helper m_updateLaneMapping() |
| src/dsf/mobility/Street.cpp | Implemented changeNLanes() and refactored lane mapping logic into helper method |
| src/dsf/mobility/RoadNetwork.hpp | Added changeStreetNLanesById() and changeStreetNLanesByName() method declarations |
| src/dsf/mobility/RoadNetwork.cpp | Implemented RoadNetwork-level lane change methods and added logging to setStreetStatusById() |
| src/dsf/utility/queue.hpp | Added move constructors/operators and deleted copy operations; added using directives for base constructors |
| src/dsf/bindings.cpp | Exposed new lane change methods to Python API |
| test/mobility/Test_street.cpp | Added comprehensive tests for Street::changeNLanes() |
| test/mobility/Test_graph.cpp | Added comprehensive tests for RoadNetwork lane change methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pStreet->changeNLanes(nLanes); | ||
| if (speedFactor.has_value()) { | ||
| pStreet->setMaxSpeed(pStreet->maxSpeed() * speedFactor.value()); | ||
| } |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Street::changeNLanes method already accepts an optional speedFactor parameter, but this implementation calls changeNLanes without it and then separately calls setMaxSpeed. Consider passing the speedFactor directly to changeNLanes instead: pStreet->changeNLanes(nLanes, speedFactor);. This would simplify the code and use the designed API more directly.
| if (speedFactor.has_value()) { | ||
| m_maxSpeed = m_maxSpeed * speedFactor.value(); | ||
| } | ||
| m_exitQueues.resize(m_nLanes); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When decreasing the number of lanes using m_exitQueues.resize(m_nLanes), agents in the removed queues will be lost. The queues contain std::unique_ptr<Agent>, so when the vector is resized down, the destructors will be called and the agents will be deleted. Consider handling these agents by moving them to remaining queues or throwing an error if there are agents present in the queues being removed.
| auto const& pStreet{edge(streetId)}; | ||
| pStreet->changeNLanes(nLanes); | ||
| if (speedFactor.has_value()) { | ||
| pStreet->setMaxSpeed(pStreet->maxSpeed() * speedFactor.value()); | ||
| } |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RoadNetwork maintains a total capacity (m_capacity) that is the sum of all street capacities. When a street's number of lanes is changed, its capacity changes, but the RoadNetwork's total capacity is not updated. This will cause the network's capacity to become out of sync with the actual sum of street capacities. Consider updating the network's capacity by calculating the delta (new capacity - old capacity) and adding it to m_capacity.
| auto const& pStreet = pair.second; | ||
| if (pStreet->name().find(streetName) != std::string::npos) { | ||
| pStreet->changeNLanes(nLanes); | ||
| if (speedFactor.has_value()) { | ||
| pStreet->setMaxSpeed(pStreet->maxSpeed() * speedFactor.value()); | ||
| } | ||
| ++nAffectedRoads; | ||
| } | ||
| }); |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RoadNetwork maintains a total capacity (m_capacity) that is the sum of all street capacities. When streets' number of lanes are changed, their capacities change, but the RoadNetwork's total capacity is not updated. This will cause the network's capacity to become out of sync with the actual sum of street capacities. Consider tracking capacity changes for all affected streets and updating the network's m_capacity accordingly.
| pStreet->changeNLanes(nLanes); | ||
| if (speedFactor.has_value()) { | ||
| pStreet->setMaxSpeed(pStreet->maxSpeed() * speedFactor.value()); | ||
| } |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Street::changeNLanes method already accepts an optional speedFactor parameter, but this implementation calls changeNLanes without it and then separately calls setMaxSpeed. Consider passing the speedFactor directly to changeNLanes instead: pStreet->changeNLanes(nLanes, speedFactor);. This would simplify the code and use the designed API more directly.
| using std::queue<T, Container>::queue; | ||
| typedef typename Container::iterator iterator; | ||
| typedef typename Container::const_iterator const_iterator; | ||
|
|
||
| queue() = default; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The using std::queue<T, Container>::queue; directive at line 10 conflicts with the explicit queue() = default; constructor at line 14. When you use a using declaration to inherit constructors, you should not also explicitly declare the same constructors. The using declaration already brings in the default constructor from the base class. This can lead to ambiguity or compilation issues. Remove either the using declaration or the explicit queue() = default; declaration.
| typedef typename Container::iterator iterator; | ||
| typedef typename Container::const_iterator const_iterator; | ||
|
|
||
| priority_queue() = default; |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The using std::priority_queue<T, Container, Compare>::priority_queue; directive at line 35 conflicts with the explicit priority_queue() = default; constructor at line 39. When you use a using declaration to inherit constructors, you should not also explicitly declare the same constructors. The using declaration already brings in the default constructor from the base class. This can lead to ambiguity or compilation issues. Remove either the using declaration or the explicit priority_queue() = default; declaration.
| priority_queue() = default; |
| priority_queue(const priority_queue&) = delete; | ||
| priority_queue& operator=(const priority_queue&) = delete; | ||
|
|
||
| // c is a protected member of std::queue, which is the underlying container |
Copilot
AI
Jan 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "c is a protected member of std::queue" but should state "c is a protected member of std::priority_queue" since this is in the priority_queue class definition.
| // c is a protected member of std::queue, which is the underlying container | |
| // c is a protected member of std::priority_queue, which is the underlying container |
No description provided.