-
Notifications
You must be signed in to change notification settings - Fork 4
Re-introduce Agent id attribute #398
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #398 +/- ##
=======================================
Coverage 85.55% 85.55%
=======================================
Files 53 53
Lines 6063 6064 +1
Branches 657 657
=======================================
+ Hits 5187 5188 +1
Misses 865 865
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:
|
|
|
||
| static void BM_Agent_SetSrcNodeId(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_SetStreetId(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_SetNextStreetId(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_SetSpeed(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_SetFreeTime(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_IncrementDistance(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_Reset(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| // Getter benchmarks - these are inline so very fast | ||
| static void BM_Agent_Getters(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
|
|
||
| static void BM_Agent_Itinerary(benchmark::State& state) { | ||
| dsf::mobility::Agent agent(0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| dsf::mobility::Agent agent(0, 0, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
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 PR re-introduces an explicit Agent id by adding it to dsf::mobility::Agent constructors and updating call sites across mobility tests, dynamics, and benchmarks.
Changes:
- Update
Agentto store anIdand require it in constructors. - Update
RoadDynamicsagent-construction helpers to pass an id when creating agents. - Update unit tests and benchmarks to use the new
Agentconstructor signatures.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/dsf/mobility/Agent.hpp |
Adds m_id and updates constructor signatures to require an id. |
src/dsf/mobility/Agent.cpp |
Implements updated constructors initializing m_id. |
src/dsf/mobility/RoadDynamics.hpp |
Updates templated addAgent/addAgents constraints and construction path to include agent ids. |
test/mobility/Test_agent.cpp |
Updates agent construction in unit tests to include ids. |
test/mobility/Test_street.cpp |
Updates agent construction in street-related tests to include ids. |
test/mobility/Test_dynamics.cpp |
Updates agent construction in dynamics tests to include ids. |
test/base/Test_node.cpp |
Updates agent construction used in intersection/node tests to include ids. |
benchmark/Bench_Agent.cpp |
Updates benchmark agent construction to include ids. |
benchmark/Bench_Intersection.cpp |
Updates benchmark code (currently contains API mismatches requiring fixes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void RoadDynamics<delay_t>::addAgents(std::size_t const nAgents, TArgs&&... args) { | ||
| for (size_t i{0}; i < nAgents; ++i) { | ||
| addAgent(std::make_unique<Agent>(this->time_step(), std::forward<TArgs>(args)...)); | ||
| addAgent(std::make_unique<Agent>( | ||
| this->m_nAddedAgents, this->time_step(), std::forward<TArgs>(args)...)); | ||
| } |
Copilot
AI
Feb 2, 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.
In addAgents, each loop iteration uses this->m_nAddedAgents as the id without incrementing it, so all agents created in the batch will share the same id. Consider reserving a range of ids up-front via fetch_add(nAgents) and using startId + i (or increment per iteration) to guarantee uniqueness.
| auto agent = std::make_unique<dsf::mobility::Agent>( | ||
| spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| intersection.addAgent(0.0, std::move(agent)); | ||
| intersection.addAgent(0, 0.0, std::move(agent)); | ||
| } |
Copilot
AI
Feb 2, 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.
This benchmark no longer matches the Agent and Intersection::addAgent APIs: Agent construction here is missing the new leading id parameter, and Intersection::addAgent takes (double angle, std::unique_ptr<Agent>) (no leading id). Update the agent construction to include an id and call intersection.addAgent(0.0, std::move(agent)).
| for (auto _ : state) { | ||
| dsf::mobility::Agent agent( | ||
| spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| 0, spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| intersection.setCapacity(100); | ||
| auto agent = std::make_unique<dsf::mobility::Agent>( | ||
| spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| 0, spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| intersection.setCapacity(100); | ||
| auto agent = std::make_unique<dsf::mobility::Agent>( | ||
| spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| 0, spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
| roundabout.setCapacity(100); | ||
| auto agent = std::make_unique<dsf::mobility::Agent>( | ||
| spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); | ||
| 0, spawnTime++, std::make_shared<dsf::mobility::Itinerary>(1, 1), 0); |
Check notice
Code scanning / Cppcheck (reported by Codacy)
MISRA 12.3 rule Note test
No description provided.