-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
RoadLane offset approx algorithm #155
Conversation
Thanks for making the PR, see my response here - if it is just due to small/0-angle continuity, perhaps we can do some very light check to see if the angles are so small that we can forgo the approximation altogether. Just an idea |
The latest update fixes the IDE freezing issue and it should also fix problems where RoadLanes doubled back on themselves. It is believed that both issues were caused by the projected angles being too close to 90 degrees, which can result in point positions that are too large. One way to re-create the problem scenario is to simply add a new RoadPoint using the buttons in the Inspector panel. The default distance and handle values are such that the resulting segment's prev and next mag handles are very close to overlapping and it doesn't take much to move them into a 90 degree orientation with one another. Validation logic was added to see if the handles were close to 90 degrees. If the angle is 90 degrees +/- a certain "margin", then the old handle projection method is used. Otherwise, the new algorithm is used. Currently, the margin is set to 10 degrees, which prevents exceedingly high point values. But, the margin can always be increased, if needed. |
Oh, and Transition Lanes should work fine! |
Great, this is working well on my side, thanks for the quick work to address this. Do you see any outstanding work on this one @bdog2112? Feel free to flip to ready for review and add me once you do. |
These changes are ready to go. Ran GUT and 1 test failed. Traced the history of the test failure back to checkin ed79d21. All tests passed prior to that checkin. Failure doesn't appear to be related to this PR. |
Thanks @bdog2112, yup I'm aware of that failing test in my other PR as well, I'll take care of that one when I have a moment. Will mark this as review ready and see if I have any comments to add. |
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.
Approving this - made two comments, but I can make the quick change and then aim to merge this shortly. Tested out in Wheel Steal, and appears to work as needed. Nice work!
var offset_s = tan(angle_s) * out_offset | ||
var pt_f = a_gbasis.z * (vec_ab.length() + offset_q) | ||
var pt_g = -d_gbasis.z * (vec_cd.length() + offset_s) | ||
var rad_ninety_deg = 1.5707963267949 # 90 degrees in radians |
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.
May as well do something like this, I think it'll be faster on runtime anyways and more exact potentially
var rad_ninety_deg = 1.5707963267949 # 90 degrees in radians | |
const rad_ninety_deg := PI/2 # 90 degrees in radians |
# and distance. | ||
func offset_curve(road_seg: Spatial, road_lane: Path, in_offset: float, out_offset: float, rp0: Spatial, rp1: Spatial): | ||
|
||
var src = road_seg.curve |
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 general here and below, would suggest implied typing or explicit where required. This is a somewhat heavy process and so having type hints will help godot make the more efficient compile choices.
var src = road_seg.curve | |
var src := road_seg.curve |
Curve offsets are nicely drawn. However, something must be done about Lane Transitions. They plot perfectly fine. But, the IDE freezes when the "+Next RoadPoint" and "+Prior RoadPoint" buttons are used. Said buttons don't freeze the IDE when there are no Lane Transitions.