pyoscx / scenariogeneration

Python library to generate linked OpenDRIVE and OpenSCENARIO files
Mozilla Public License 2.0
255 stars 81 forks source link

Missing documentation for lane_offset in add_predecessor/successor() #84

Closed johschmitz closed 1 year ago

johschmitz commented 2 years ago

I am struggling a bit to implement the lane_offset for the direct junctions correctly for all cases. Now I found that it seems to be missing from the documentation. Could you explain how exactly the lane_offset is defined? Also does it include or exclude the center lane or how does it behave with respect to the center lane?

mander76 commented 2 years ago

Center lane is never linked, since it is not supposed to be a driving lane (width 0)

Will try to fix some more documentation about it, but in short: -1 means you move the lane one step to the right (+1 left), and it should be done on the road that is "shifting" from (connecting road or in the case of direct junction the outgoing road) This is not a bulletproof implementation however, there are probably some cases where this will fail, was a first attempt to do this and no problems (so far)..

johschmitz commented 2 years ago

Yes, I saw in the example that you apply it to the "outgoing roads", although the outgoing roads also could be an incoming roads. So I guess the definition is more like "apply it to the non-split roads"(?) What I don't understand is where you start counting? Where is the reference point? At the center lane of the split road? Or at the leftmost lane of the split road? If you start at the leftmost lane do you include the center lane in the count? For me it is important that it also works for roads that are all connected with their "end" connection point. If it helps I will come back with some minimal examples.

mander76 commented 2 years ago

aha, well then it's -1 for right lanes (should endup on -2), +1 for left lanes (should endup on +2)

johschmitz commented 2 years ago

So are you saying if I set lane_offset to 0 then center lanes are aligned? And for example if lane _offset is 0 and end of road is connected to end of road then lane 1 would be connected to lane -1?

mander76 commented 2 years ago

yes, that is probably a good way of putting it, technically lane_offset = 0 should be like a default behavior.

johschmitz commented 2 years ago

I am trying to build an example with 4 roads to challenge your implementation. direct_junctions_entry_exit.py.zip I can't get the offset of the second incoming road working so far.

I am also going to build a second example with roads connecting with their end contact point to the direct junction. So hopefully we can cover all cases and fix all bugs and/or update the API if some information is missing. One thing I was wondering about was if the lane_offset should be moved as a parameter to the create_direct_junction() instead of the add_successor()/add_predecessor() methods to be able to control it in one central location.

johschmitz commented 2 years ago

Here the other example: direct_junctions_entry_exit_successor.py.zip I hope we find a way to fix both.

mander76 commented 2 years ago

Nice! I'll try to have time to look into it, maybe not before the weekend though..

johschmitz commented 2 years ago

No problem, take your time. There are enough other topics to work on 😅. I figured there needs to be one common reference point for each direct junction. All the road's lane offsets need to be relative to that. If all lane offsets are 0 that means all center lanes "overlap". I believe you also need to define a direction, otherwise the sign (direction of offset) is not well defined. One way of defining the direction would be through specifying one of the connected roads as the reference for the direction. Probably you currently do that implicitly and use the first road in the list or something like that.

mander76 commented 2 years ago

I managed to get some time, but these roads doesn't make sence? Can you post a picture how you actually want them? the continuation roads should not be able to have the same number of lanes?

johschmitz commented 2 years ago

Well the idea is to build ONE direct junction with an on-ramp on one side an off-ramp (maybe entry/exit is not quite correct) on the other side. And that it should work for roads with a successor as well as a predecessor connection to the direct junction (any combination). I believe that should cover most cases if that works. Obviously my code examples have to be slightly tweaked after you fixed/implemented it. If it is still unclear I will draw it for you.

mander76 commented 2 years ago

the problem I have with this one is that you are trying to left lanes and right lanes together, so the driving direction will be kind of messed up in this example. If you can draw it it would probably help

johschmitz commented 2 years ago

Okay I think you are right. So just assume an off-ramp on both sides. But essentially I believe the linking is independent of traffic direction, isn't it?

mander76 commented 2 years ago

Something like this? image

johschmitz commented 2 years ago

Exactly, nice! Now it should work for all possible combinations of road directions and connections. Even overlaps of roads should be possible as currently discussed in OpenDRIVE 1.8 standardization.

mander76 commented 2 years ago

well, all possible combinations are not correct as far as I understand it, you still have to take into consideration the driving directions.. And I found an issue and that is the "first fixed" road.. it cannot be a road with an offset.. And I have no idea how to fix it sadly enough..

direct_junctions_entry_exit.zip .

johschmitz commented 2 years ago

About this first fixed road that's what I meant before. You need a reference point. So I was thinking that the reference point could be the center lane of each road. So if you set all the lane offsets to 0 then all the center lanes should all be aligned. And that is why I think it is easier to handle the offsets all together in the direct junction API itself and not separately for each road!

mander76 commented 2 years ago

well if you create the directy junction yourself, then it's not a problem then you use that definition to connect the roads, that algorithm has to be implemented of course..

johschmitz commented 2 years ago

Yes I can create it myself but currently there is no such API? Or am I wrong? Even if the API stays like it is all offsets should be possible. I think this is the basic requirement. Having a nice and simple API would be good as well but is less important.

mander76 commented 2 years ago

the problem is to have something simple :(

johschmitz commented 2 years ago

I agree it is not super easy, I am thinking about different ways but maybe we should find a solution together. I make suggestions and you say if it is possible or why not. My first idea: Define the offsets pairwise between each incoming and linked road pair of the direct junction. Just how many lanes their respective center lanes are apart. So for that I think there does not even need to be a "first fixed road". I think it could even work with the current API. Whenever you call add_successor() or add_predecessor() you have already defined the pairwise offsets I guess? What is the problem I don't see? Here is a drawing to help with the discussion: direct_junction

johschmitz commented 2 years ago

Okay I see the problem now: instead of something like this:

entry_road.add_successor(xodr.ElementType.junction,100,lane_offset=-1,direct_junction=[3])
continuation_road_in.add_successor(xodr.ElementType.junction,100,lane_offset=-1,direct_junction=[3,4])
continuation_road_out.add_successor(xodr.ElementType.junction,100,lane_offset=1,direct_junction=[1,2])
exit_road.add_successor(xodr.ElementType.junction,100,lane_offset=1,direct_junction=[2])

we would need something like this, which really gives us a pairwise connection and pairwise offsets:

entry_road.add_successor(xodr.ElementType.junction,100,lane_offset=-1,link_id=4)
continuation_road_in.add_successor(xodr.ElementType.junction,100,lane_offset=-1,link_id=3)
continuation_road_in.add_successor(xodr.ElementType.junction,100,lane_offset=-2,link_id=4)
continuation_road_out.add_successor(xodr.ElementType.junction,100,lane_offset=1,link_id=1)
continuation_road_out.add_successor(xodr.ElementType.junction,100,lane_offset=2,link_id=2)
exit_road.add_successor(xodr.ElementType.junction,100,lane_offset=1,link_id=2)

The offsets might be wrong, did not check but I hope you get the point.

johschmitz commented 2 years ago

I think there might be an even simpler way for you: keeping your API but having the lane_offset as a list instead of a scalar value.

Edit: Something like this:

entry_road.add_successor(xodr.ElementType.junction,100,lane_offsets=[-1],direct_junction=[3])
continuation_road_in.add_successor(xodr.ElementType.junction,100,lane_offsets=[-1,2],direct_junction=[3,4])
continuation_road_out.add_successor(xodr.ElementType.junction,100,lane_offsets=[1,1],direct_junction=[1,2])
exit_road.add_successor(xodr.ElementType.junction,100,lane_offsets=[1],direct_junction=[2])
johschmitz commented 2 years ago

Here is the result of me playing around with the code: https://github.com/pyoscx/scenariogeneration/pull/86 I feel that it would be good to better decouple the two cases:

Does not feel very nice. But I am sure that we need individual offsets for each link pair in a direct junction.

mander76 commented 2 years ago

I'll try to have a look at it!

MandolinMicke commented 2 years ago

Are these kind of direct junctions allowed in OpenDrive?

MandolinMicke commented 2 years ago

Ah, now I see, possibly in 1.8

MandolinMicke commented 2 years ago

But in the example there is no conservation of driving direction, so I highly doubt this is allowed in 1.8?

johschmitz commented 2 years ago

Overlap will probably be allowed but let's not focus on that now. The example in the pull request is not working fine for me either. I marked the pull request as draft because it needs fixing. Also in the drawing above I think the center lane of the lower off-ramp has to be in the opposite side of the road in order to fix the driving direction as you suggest. I was struggling to get it right in your code. The one thing I am pretty sure about though is that there needs to be a pairwise information. It simply can not work having two roads connected but just one offset I believe.

Also in general I do not think that it is forbidden to connect lanes with opposing driving directions. Often it doesn't make sense but I don't think it is illegal. Also in some cases, with bidirectional lanes for example, it will be needed.

mander76 commented 2 years ago

True it is not forbidden, but then it should be all lanes, then you just shift all signs in the lane links element. Haven't looked into bidirectional stuffs much though. I'm starting to agree with you with the pairwise thing, the single offset works for some limited cases only.

johschmitz commented 2 years ago

Yeah, the question though is how to nicely integrate it with the existing cases in your library which only require a single offset...

mander76 commented 2 years ago

I would be ok to change that since it's very limited and for direct junctions more a POC than a very nice implementation..

johschmitz commented 2 years ago

We have a bidirectional lane close to my place. It is a three lane road where the middle lane can be used in both directions to slow down and then cross. Quite a nasty idea if you ask me... But back to topic, I hope you have some kind of idea for fixing the API. Maybe there can be another input into the direct junction API itself. So when you say add_successor() you would only mention the direct junction and then you would create the direct junction and then something like dj.add_link() or so. Could even first create the direct junction so that you can reference it in the add_successor() as a Python object and don't have to fiddle around with the ID.

mander76 commented 2 years ago

Hmm, as you mention to create the direct is an option. When we did the first try at automatically create (normal) junctions it was easier to generate that one and connect the roads together without it. So it would be nice to have a similar API for normal junctions aswell, to increase the functionality to create normal junctions similarly.. However then the add_successor/add_predecessor is still needed since that junction entry doesn't contain enough information to connect the roads together :(

As a side note: The main goal for this package has always been to make it easier to create stuffs (using the generators). However it was never the goal to create anything with them. All classes are there to generate the xmls, however the automation is super hard to do "generic", especially for junctions...

johschmitz commented 2 years ago

You say there would not be enough information coming from add_successor but why can't you simply have a method that operates on the direct junction to add that information like

djunction = xodr.create_direct_junction()
djunction.add_link(incoming_road, linked_road, offset)

or something in that direction. So maybe don't try to make the API too convenient and too abstract but just "good enough".

mander76 commented 2 years ago

maybe, don't know if this kind of API will change anything compared to what is already there though..

MandolinMicke commented 2 years ago

I have started a small POC for a more complete junction creator, that has a more user friendly API (started with a default junction) so an example of a T-junction could look like this:


junction_creator = xodr.JunctionCreator(junction_id = 100)

road1 = xodr.create_road(xodr.Line(100),1)
road2 = xodr.create_road(xodr.Line(100),2)
road3 = xodr.create_road(xodr.Line(100),3)

road1.add_successor(xodr.ElementType.junction, 100)
road2.add_successor(xodr.ElementType.junction, 100)
road3.add_predecessor(xodr.ElementType.junction, 100)

junction_creator.add_incomming_road(road1, radius = 20, angle=0)
junction_creator.add_incomming_road(road2, radius = 30, angle=1*np.pi)
junction_creator.add_incomming_road(road3, radius = 20, angle=1*np.pi/2)

junction_creator.add_connection(road_one_id=1, road_two_id=2)
junction_creator.add_connection(road_one_id=1, road_two_id=3)
junction_creator.add_connection(road_one_id=2, road_two_id=3)

This is for when roads have the same amout of lanes and it's easy to connect. For more complicated junctions (including direct junctions) Additional optional input can be added to something like this:

junction_creator.add_connection(road_one_id=1, 
road_two_id=2, 
ane_one_id=-1, 
lane_two_id=1)

Thoughts?

johschmitz commented 2 years ago

Some questions:

MandolinMicke commented 2 years ago

Well, yes. so

johschmitz commented 2 years ago

Alright, so if you want to unify all junctions then when adding connections how do you differentiate between an inner junction connecting roads and normal road?

MandolinMicke commented 2 years ago

Well, my first idea is that when you add a connection, you create the connecting road (if it's not a direct junction)

Another example (where you add the exact positions instead):


junction_creator = xodr.JunctionCreator(id = 100, name = 'my_junction')

road1 = xodr.create_road(xodr.Line(100),1)
road2 = xodr.create_road(xodr.Line(100),2)
road3 = xodr.create_road(xodr.Line(100),3)

road1.add_predecessor(xodr.ElementType.junction, 100)
road2.add_predecessor(xodr.ElementType.junction, 100)
road3.add_predecessor(xodr.ElementType.junction, 100)

junction_creator.add_incomming_road_generic_junction(road1,x=0,y=0,heading=0)
junction_creator.add_incomming_road_generic_junction(road2,x=0,y=40,heading=np.pi)
junction_creator.add_incomming_road_generic_junction(road3,x=20,y=20,heading=-1.1*np.pi/2)

junction_creator.add_connection(road_one_id=1, road_two_id=2)
junction_creator.add_connection(road_one_id=1, road_two_id=3)
junction_creator.add_connection(road_one_id=2, road_two_id=3)

This way it is possible to add different ways of adding the roads to connect to a junction.

johschmitz commented 2 years ago

Why not just have a junction creator for each junction type? What is optimized when everything is consolidated into a single junction creator? Also what do we loose?

Edit: Aren't we running into some kind of leaky abstraction problem here?

MandolinMicke commented 2 years ago

I think there are quite alot of interdependencies, but lets see

MandolinMicke commented 2 years ago

f5ec261

A first attempt on a junction creator, and some fixes for adjust_roads_and_lanes to handle direct junctions independent on the "order" things are added. Now the direct junction input is a dict instead of a list (the linked road (id) and the road offset)

johschmitz commented 2 years ago

Some observations/recommendations/review comments:

johschmitz commented 2 years ago

I am a bit confused about these lines of the direct junction example:

junction_creator.add_connection(road1, road2, [-2,-1,1,2], [-2,-1,1,2], [road1, road2, road3])
junction_creator.add_connection(road1, road3, -3, -1, [road1, road2, road3])
road2.pred_direct_junction = {1:0}

Are these different ways to use the junction creator? It seems this "all_roads" parameters is not needed? I think I will first try to use the approach with the lane ID lists.

mander76 commented 2 years ago

The all_roads was used because I had a nasty bug, that was really hard to debug so had to add that input, will be removed.

johschmitz commented 2 years ago

Could you add an error message when the lane id lists in add_connection() are of different length?

mander76 commented 2 years ago

It is added, but not pushed..

johschmitz commented 2 years ago

Could you push the latest version to the branch for me to try it out?