Skip to content
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.

Sign up for GitHub

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

Improve "bogus name" handling when setting StreetEdge's name during postprocessing #6321

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Improve "bogus name" handling when setting StreetEdge's name during postprocessing #6321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a178b9d
Set bogus name to false when updating a StreetEdge's name
leonardehrenfried Dec 8, 2024
f0dd4f2
Add test for name setting
leonardehrenfried Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions application/src/main/java/org/opentripplanner/street/model/edge/Edge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,15 +145,17 @@ public String getDefaultName() {
*/
public abstract I18NString getName();

// TODO Add comments about what a "bogus name" is.
/**
* Returns true if this edge has a generated name that is derived from its properties,
* like "path", "stairs" or "tunnel".
* <p>
* Returns false if the field reflects the real world name, like "Fifth Avenue",
* "Hauptstraße" or "Øvre Holmegate".
*/
public boolean hasBogusName() {
return false;
}

// The next few functions used to live in EdgeNarrative, which has now been
// removed
// @author mattwigway

public LineString getGeometry() {
return null;
}
Expand Down
14 changes: 9 additions & 5 deletions application/src/main/java/org/opentripplanner/street/model/edge/StreetEdge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import javax.annotation.Nullable;
import org.locationtech.jts.geom.LineString;
import org.locationtech.jts.geom.impl.PackedCoordinateSequence;
import org.opentripplanner.framework.geometry.CompactLineStringUtils;
Expand Down Expand Up @@ -82,14 +79,14 @@ public class StreetEdge

/**
* bicycleSafetyWeight = length * bicycleSafetyFactor. For example, a 100m street with a safety
* factor of 2.0 will be considered in term of safety cost as the same as a 200m street with a
* factor of 2.0 will be considered in terms of safety cost as the same as a 200m street with a
* safety factor of 1.0.
*/
private float bicycleSafetyFactor;

/**
* walkSafetyFactor = length * walkSafetyFactor. For example, a 100m street with a safety
* factor of 2.0 will be considered in term of safety cost as the same as a 200m street with a
* factor of 2.0 will be considered in terms of safety cost as the same as a 200m street with a
* safety factor of 1.0.
*/
private float walkSafetyFactor;
Expand Down Expand Up @@ -446,8 +443,15 @@ public I18NString getName() {
return this.name;
}

/**
* Update the name of the edge after it has been constructed. This method also sets the bogusName
* property to false, indicating to the code that maps from edges to steps that this is a real
* street name.
* @see Edge#hasBogusName()
*/
public void setName(I18NString name) {
this.name = name;
this.flags = BitSetUtils.set(flags, HASBOGUSNAME_FLAG_INDEX, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expression "has bogus name" in the context of an accessor method may imply that a "bogus name" is a thing that may be present or absent, in addition to a "normal name". Seeing these terms in the past I am not sure I realized that "bogusness" was only signalling the provenance of the name, not an extra name. If we want to keep using the term "bogus", is it a better description to say "Name is bogus" instead of "we have a bogus name"?

Taking this a step further, is there a more descriptive term for this than "bogus"? The names are more "inferred", "generic", or "placeholder" than "bogus". Or inverting the boolean condition, "normal" names are those explicitly specified by OSM name tags. Unfortunately "has explicit name from tag" is a little long. NAME_IS_GENERIC or NAME_IS_INFERRED?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree with this but I want to do it in another PR.

I prefer NAME_IS_INFERRED. We can also discuss if the boolean is the right way to model this or if getName should return a sealed interface to express the two types of names.

}

public boolean hasBogusName() {
Expand Down
1 change: 1 addition & 0 deletions .../src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ void postprocess() {
assertNotEquals(sidewalk.edge.getName(), pryorStreet.edge.getName());
builder.postProcess(new SidewalkNamer());
assertEquals(sidewalk.edge.getName(), pryorStreet.edge.getName());
assertFalse(sidewalk.edge.hasBogusName());
}

private static class ModelBuilder {
Expand Down
6 changes: 3 additions & 3 deletions .../test/java/org/opentripplanner/routing/algorithm/mapping/StatesToWalkStepsMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void enterStation() {
var walkSteps = buildWalkSteps(builder);
assertEquals(2, walkSteps.size());
var enter = walkSteps.get(1);
assertEquals(enter.getRelativeDirection(), ENTER_STATION);
assertEquals(ENTER_STATION, enter.getRelativeDirection());
}

@Test
Expand All @@ -54,7 +54,7 @@ void exitStation() {
var walkSteps = buildWalkSteps(builder);
assertEquals(3, walkSteps.size());
var enter = walkSteps.get(2);
assertEquals(enter.getRelativeDirection(), EXIT_STATION);
assertEquals(EXIT_STATION, enter.getRelativeDirection());
}

@Test
Expand All @@ -64,7 +64,7 @@ void signpostedPathway() {
var walkSteps = buildWalkSteps(builder);
assertEquals(2, walkSteps.size());
var step = walkSteps.get(1);
assertEquals(step.getRelativeDirection(), FOLLOW_SIGNS);
assertEquals(FOLLOW_SIGNS, step.getRelativeDirection());
assertEquals(sign, step.getDirectionText().toString());
}

Expand Down
68 changes: 44 additions & 24 deletions application/src/test/java/org/opentripplanner/street/model/edge/StreetEdgeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.opentripplanner.street.model.StreetTraversalPermission.ALL;
import static org.opentripplanner.street.model._data.StreetModelForTest.intersectionVertex;
import static org.opentripplanner.street.model._data.StreetModelForTest.streetEdge;
import static org.opentripplanner.street.model._data.StreetModelForTest.streetEdgeBuilder;
Expand All @@ -17,6 +18,8 @@
import org.locationtech.jts.geom.GeometryFactory;
import org.locationtech.jts.geom.LineString;
import org.locationtech.jts.geom.impl.PackedCoordinateSequence;
import org.opentripplanner.framework.geometry.GeometryUtils;
import org.opentripplanner.framework.i18n.I18NString;
import org.opentripplanner.routing.api.request.StreetMode;
import org.opentripplanner.routing.core.VehicleRoutingOptimizeType;
import org.opentripplanner.routing.util.ElevationUtils;
Expand All @@ -43,7 +46,7 @@ public class StreetEdgeTest {
private StreetSearchRequest proto;

@BeforeEach
public void before() {
void before() {
v0 = intersectionVertex("maple_0th", 0.0, 0.0); // label, X, Y
v1 = intersectionVertex("maple_1st", 2.0, 2.0);
v2 = intersectionVertex("maple_2nd", 2.0, 1.0);
Expand All @@ -64,9 +67,9 @@ public void before() {
}

@Test
public void testInAndOutAngles() {
void testInAndOutAngles() {
// An edge heading straight West
StreetEdge e1 = streetEdge(v1, v2, 1.0, StreetTraversalPermission.ALL);
StreetEdge e1 = streetEdge(v1, v2, 1.0, ALL);

// Edge has same first and last angle.
assertEquals(90, e1.getInAngle());
Expand All @@ -77,18 +80,16 @@ public void testInAndOutAngles() {
StreetVertex v = intersectionVertex("test2", 2.0, 2.0);

// Second edge, heading straight North
StreetEdge e2 = streetEdge(u, v, 1.0, StreetTraversalPermission.ALL);
StreetEdge e2 = streetEdge(u, v, 1.0, ALL);

// 180 degrees could be expressed as 180 or -180. Our implementation happens to use -180.
assertEquals(180, Math.abs(e2.getInAngle()));
assertEquals(180, Math.abs(e2.getOutAngle()));
}

@Test
public void testTraverseAsPedestrian() {
StreetEdge e1 = streetEdgeBuilder(v1, v2, 100.0, StreetTraversalPermission.ALL)
.withCarSpeed(10.0f)
.buildAndConnect();
void testTraverseAsPedestrian() {
StreetEdge e1 = streetEdgeBuilder(v1, v2, 100.0, ALL).withCarSpeed(10.0f).buildAndConnect();

StreetSearchRequest options = StreetSearchRequest
.copyOf(proto)
Expand All @@ -106,10 +107,8 @@ public void testTraverseAsPedestrian() {
}

@Test
public void testTraverseAsCar() {
StreetEdge e1 = streetEdgeBuilder(v1, v2, 100.0, StreetTraversalPermission.ALL)
.withCarSpeed(10.0f)
.buildAndConnect();
void testTraverseAsCar() {
StreetEdge e1 = streetEdgeBuilder(v1, v2, 100.0, ALL).withCarSpeed(10.0f).buildAndConnect();

State s0 = new State(v1, StreetSearchRequest.copyOf(proto).withMode(StreetMode.CAR).build());
State s1 = e1.traverse(s0)[0];
Expand All @@ -122,8 +121,8 @@ public void testTraverseAsCar() {
}

@Test
public void testModeSetCanTraverse() {
StreetEdge e = streetEdge(v1, v2, 1.0, StreetTraversalPermission.ALL);
void testModeSetCanTraverse() {
StreetEdge e = streetEdge(v1, v2, 1.0, ALL);

TraverseModeSet modes = TraverseModeSet.allModes();
assertTrue(e.canTraverse(modes));
Expand All @@ -146,7 +145,7 @@ public void testModeSetCanTraverse() {
* correctly during turn cost computation. 4. Traffic light wait time is taken into account.
*/
@Test
public void testTraverseModeSwitchBike() {
void testTraverseModeSwitchBike() {
var vWithTrafficLight = new LabelledIntersectionVertex("maple_1st", 2.0, 2.0, false, true);
StreetEdge e0 = streetEdge(v0, vWithTrafficLight, 50.0, StreetTraversalPermission.PEDESTRIAN);
StreetEdge e1 = streetEdge(
Expand Down Expand Up @@ -183,7 +182,7 @@ public void testTraverseModeSwitchBike() {
* the bike walking speed on the walking speed. 4. Traffic light wait time is taken into account.
*/
@Test
public void testTraverseModeSwitchWalk() {
void testTraverseModeSwitchWalk() {
var vWithTrafficLight = new LabelledIntersectionVertex("maple_1st", 2.0, 2.0, false, true);
StreetEdge e0 = streetEdge(
v0,
Expand Down Expand Up @@ -214,7 +213,7 @@ public void testTraverseModeSwitchWalk() {
* Test the bike switching penalty feature, both its cost penalty and its separate time penalty.
*/
@Test
public void testBikeSwitch() {
void testBikeSwitch() {
StreetEdge e0 = streetEdge(v0, v1, 0.0, StreetTraversalPermission.PEDESTRIAN);
StreetEdge e1 = streetEdge(v1, v2, 0.0, StreetTraversalPermission.BICYCLE);
StreetEdge e2 = streetEdge(v2, v0, 0.0, StreetTraversalPermission.PEDESTRIAN_AND_BICYCLE);
Expand Down Expand Up @@ -271,9 +270,9 @@ public void testBikeSwitch() {
}

@Test
public void testTurnRestriction() {
StreetEdge e0 = streetEdge(v0, v1, 50.0, StreetTraversalPermission.ALL);
StreetEdge e1 = streetEdge(v1, v2, 18.4, StreetTraversalPermission.ALL);
void testTurnRestriction() {
StreetEdge e0 = streetEdge(v0, v1, 50.0, ALL);
StreetEdge e1 = streetEdge(v1, v2, 18.4, ALL);
StreetSearchRequestBuilder streetSearchRequestBuilder = StreetSearchRequest.copyOf(proto);
streetSearchRequestBuilder.withArriveBy(true);
StreetSearchRequest request = streetSearchRequestBuilder.withMode(StreetMode.WALK).build();
Expand All @@ -285,13 +284,13 @@ public void testTurnRestriction() {
}

@Test
public void testElevationProfile() {
void testElevationProfile() {
var elevationProfile = new PackedCoordinateSequence.Double(
new double[] { 0, 10, 50, 12 },
2,
0
);
var edge = streetEdge(v0, v1, 50.0, StreetTraversalPermission.ALL);
var edge = streetEdge(v0, v1, 50.0, ALL);
StreetElevationExtensionBuilder
.of(edge)
.withElevationProfile(elevationProfile)
Expand All @@ -306,7 +305,7 @@ public void testElevationProfile() {
}

@Test
public void testBikeOptimizeTriangle() {
void testBikeOptimizeTriangle() {
// This test does not depend on the setup method - and can probably be simplified

Coordinate c1 = new Coordinate(-122.575033, 45.456773);
Expand All @@ -326,7 +325,7 @@ public void testBikeOptimizeTriangle() {
.withGeometry(geometry)
.withName("Test Lane")
.withMeterLength(length)
.withPermission(StreetTraversalPermission.ALL)
.withPermission(ALL)
.withBack(false)
// a safe street
.withBicycleSafetyFactor(0.74f)
Expand Down Expand Up @@ -400,4 +399,25 @@ public void testBikeOptimizeTriangle() {
double expectedWeight = timeWeight * 0.33 + slopeWeight * 0.33 + safetyWeight * 0.34;
assertEquals(expectedWeight, result.getWeight(), DELTA);
}

@Test
void setName() {
var path = I18NString.of("path");
var edge = new StreetEdgeBuilder<>()
.withFromVertex(v0)
.withToVertex(v1)
.withPermission(ALL)
.withGeometry(GeometryUtils.makeLineString(v0.getCoordinate(), v1.getCoordinate()))
.withName(path)
.withBogusName(true)
.buildAndConnect();

assertEquals(path, edge.getName());
assertTrue(edge.hasBogusName());

var mainStreet = I18NString.of("Main Street");
edge.setName(mainStreet);
assertEquals(mainStreet, edge.getName());
assertFalse(edge.hasBogusName());
}
}
Loading