-
Notifications
You must be signed in to change notification settings - Fork 70
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
Create DisallowedNextLinks attributes for turn restrictions #202
Conversation
When #204 has been merged, I will update these tests to JUnit 5, too. |
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.
Great work, thanks. Just some minor notes mainly because I'm no longer up to date with java development. I didn't look at the conversion implementation in depth but the tests look thorough.
@@ -65,6 +67,9 @@ public class OsmConverterConfigGroup extends ReflectiveConfigGroup { | |||
private boolean keepTagsAsAttributes = true; | |||
private boolean keepWaysWithPublicTransit = true; | |||
|
|||
@Parameter | |||
@Comment("If true: OSM turn restrictions are parsed and written as disallowedNextLinks attribute to the first link.") |
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.
I'm not familiar with this syntax, I assume there is no issue with mixing @Comment
and map.put(...)
in getComments()
?
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.
Yes, you can mix it. It was introduced here matsim-org/matsim-libs#2201
I kept migrating to the new @Parameter
annotation for future PRs.
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.
Ah, thanks 👍
@@ -111,6 +152,9 @@ public void convert(OsmConverterConfigGroup config) { | |||
readWayParams(); | |||
convertToNetwork(transformation); | |||
cleanNetwork(); | |||
if (config.parseTurnRestrictions) { |
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.
All other config values are accessed with get methods, is this part of the new ReflectiveConfigGroup class?
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.
Yes, fields annotated with @Parameter
can be accessed directly, read and write.
@@ -36,6 +36,7 @@ public static void convertGerasdorfArtificialLanesAndMaxspeed() { | |||
osmConfig.setOsmFile("test/osm/GerasdorfArtificialLanesAndMaxspeed.osm"); | |||
osmConfig.setOutputNetworkFile("test/osm/GerasdorfArtificialLanesAndMaxspeed.xml.gz"); | |||
osmConfig.setMaxLinkLength(1000); | |||
osmConfig.parseTurnRestrictions = true; |
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.
I guess this is to ensure no unintended side effects or regressions are happening. Please add a comment "turnRestrictions are not explicitly tested in this test" or something along those lines.
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 default is off, so I had to set it to true explicitly here. The testDisallowedNextLinks()
method further down checks whether the DisallowedNextLinks
object for a selected link is generated correctly. Could you rephrase what you mean?
Sorry, mistook this comment to be meant for the other test, sure, I'll add a comment!
The version bump PRs are ok 👍 and probably long overdue |
Thanks for your comments, they should be addressed so far. |
This enables the
OsmMultimodalNetworkConverter
to attachDisallowedNextLinks
(matsim-org/matsim-libs#2855) attributes to links containing OSM turn restriction information.This is part of an effort in supporting turn restrictions in MATSim, see matsim-org/matsim-libs#2829 and thus might fix #75.
Enable this feature with