-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix(application): complete VehicleRoute object if it is missing node ids or connection ids #472
base: main
Are you sure you want to change the base?
Conversation
…ids or connection ids
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.
Apart from minor taste things, LGTM
* @param route the potentially incomplete {@link VehicleRoute} | ||
* @return the completed {@link VehicleRoute} with list of node ids and connection ids | ||
* | ||
* @throws IllegalStateException if the route contains neither node ids nor connection ids |
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.
Hm... why do you use IllegalStateException here? I would go for IllegalArgumentException.
"The IllegalArgumentException is thrown in cases where the type is accepted but not the value, like expecting positive numbers and you give negative numbers."
But I also understand that a route without ids or connections should never be instantiated, that's why you check it here? In this case, I would expect that the VehicleRoute
constructor also performs a similar check, and then, yes, it would be illegal state at this location.
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.
As long as an exception is thrown I don't care too much. IllegalArgumentException
would fit equally here.
// therefore we use this wrapping TestRule which inits sim-kernel rule on the fly | ||
new SimulationKernelRule( | ||
null, null, navigationRule.getCentralNavigationComponent(), null | ||
).apply(base, description); |
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.
Hm, can you explain this to me on Monday when we're in the office?
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.
Description
If SumoAmbassador is started with a SUMO configuration which includes routes, then all routes from SUMO are read via TraCI and send to the RTI. The application ambassador stores these
VehicleRoute
s in an internal cache. If the application simulator already have a route internally with the same ID, it is overridden with the newVehicleRoute
object coming from SUMO.There is one major issue: All
VehicleRoute
objects created by SumoAmbassador only contain a list of connection IDs. The list of node IDs is always empty. This leads to the problem, that applications which want to use these routes cannot do stuff likegetOs().getNavigationModule().getCurrentRoute().getLastNodeId()
, which is useful for dynamic route calculation.This PR therefore adds the functionality, that the
VehicleRoute
object is always completed/filled with the missing list of nodes or connections, as long at least one of these properties exists. If both are already given, nothing happens.We still override the VehicleRoute in the internal cache of the application simulator, as this is the source of truth when coming from SUMO.
Issue(s) related to this PR
Affected parts of the online documentation
Changes in the documentation required?
No
Definition of Done
Prerequisites
Required
type(scope): description
(in the style of Conventional Commits)enhancement
, orbugfix
)origin/main
has been merged into your Fork.Requested (can be enforced by maintainers)
Special notes to reviewer