Start refactoring intersection geometry code #140
Conversation
…Roads, not this InputRoad indirection. #136 This paves the way for Roads storing trim_start and trim_end fields. No behavioral change, per tests
…'ll likely be used in intersection geometry code.
| @@ -6,35 +6,40 @@ use abstutil::wraparound_get; | |||
| use geom::{Circle, Distance, InfiniteLine, PolyLine, Polygon, Pt2D, Ring, EPSILON_DIST}; | |||
|
|
|||
| use super::Results; | |||
| use crate::{InputRoad, IntersectionID, RoadID}; | |||
| use crate::{IntersectionID, Road, RoadID}; | |||
|
|
|||
| const DEGENERATE_INTERSECTION_HALF_LENGTH: Distance = Distance::const_meters(2.5); | |||
|
|
|||
| pub fn intersection_polygon( | |||
There was a problem hiding this comment.
Most of the changes here are mechanical; I'll point out subtleties
| @@ -128,28 +122,42 @@ fn generalized_trim_back( | |||
| } | |||
| } | |||
|
|
|||
| // Intersect every road's boundary lines with all the other lines. Only side effect here is to | |||
| // populate new_road_centers. | |||
| // Intersect every road's boundary lines with all the other lines. Update new_road_centers as | |||
There was a problem hiding this comment.
Bug alert!
I sank loads of time into figuring out why roads that loop like this broke. The problem was that every road is updated twice. When we calculate possible collision points and stuff, we want to use the original thing, not something that's been potentially modified by a previous round of the loop.
| // TODO Should we only do this for 2 roads? | ||
| let mut shortest_center = road_center.clone(); | ||
|
|
||
| if road_center.length() >= DEGENERATE_INTERSECTION_HALF_LENGTH + 3.0 * EPSILON_DIST { |
There was a problem hiding this comment.
Another subtlety. Repeatedly calling Transformation::GenerateIntersectionGeometry should be idempotent. This code always trims back every road from every intersection some minimal amount. Here we detect if trimming has happened before and avoid it again.
In some of my experimental branches, I'm getting rid of this rule and seeing improved results. The rule is actually just intended for degenerate intersections with exactly 2 roads. I'm feeling we should just special case those, just like we do with dead-ends.
| results.roads.get_mut(&id).unwrap().trimmed_center_line = pl; | ||
| } | ||
|
|
||
| calculate_polygon(results, input_road_lines) |
There was a problem hiding this comment.
Just starting to separate things a little bit. All of the routines in this file basically work in two phases. First trim roads. Then generate the polygon.
| @@ -278,3 +270,48 @@ impl StreetNetwork { | |||
| id | |||
| } | |||
| } | |||
|
|
|||
| /// The edge of a road, pointed into some intersection | |||
There was a problem hiding this comment.
I just moved this over from the sidewalk corner rendering code. In most of my next-step branches, I want to use this. We should keep adjusting how this works to get closer to the half-road view idea.
|
Per latest comments in #136, a few more changes need to happen and in a different order. I couldn't rebase this against I'll detangle this mess of ideas and get a simpler PR out... |
This is a small start to refactoring. Instead of using this legacy
InputRoadstruct, just take a copy of theRoadstruct as input. All information needed for the algorithm is in there, and so when we add things like trim distances, it means we don't have to copy data back and forth between two structs. It also internally simplifies some code to populate theResultsstruct immediately.There are a bunch of next steps, but I'll go discuss them in the issue separately. I think this is a solid first step with no diff in the tests. All of the next steps... might change some existing behavior!