Skip to content

Commit

Permalink
DuplicateLocationInPolyLineCheck: Fix osmlab#553, do not flag termina…
Browse files Browse the repository at this point in the history 
…ting loops (osmlab#560)

* DuplicateLocationInPolyLineCheck: Fix osmlab#553, do not flag terminating loops

* Stop flagging closed loops (Edges and LineItems)
* Continue flagging closed loops (Areas -- the last location in an area line
  _should not_ be the same as the first location in the area line)

Signed-off-by: Taylor Smock <[email protected]>

* DuplicateLocationInPolyLineCheck: Add location to flag

This makes it easier to find the problematic area.

Signed-off-by: Taylor Smock <[email protected]>
  • Loading branch information
@tsmock
tsmock authored Jun 2, 2021
1 parent cd47184 commit eb31259
Show file tree
Hide file tree
Showing 3 changed files with 388 additions and 41 deletions.
30 changes: 23 additions & 7 deletions ...va/org/openstreetmap/atlas/checks/validation/points/DuplicateLocationInPolyLineCheck.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.openstreetmap.atlas.checks.validation.points;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -25,8 +25,8 @@
*/
public class DuplicateLocationInPolyLineCheck extends BaseCheck<Long>
{
private static final List<String> FALLBACK_INSTRUCTIONS = Arrays
.asList("Repeated location found at {0} for feature id {1,number,#} ");
private static final List<String> FALLBACK_INSTRUCTIONS = Collections
.singletonList("Repeated location found at {0} for feature id {1,number,#} ");
private static final long serialVersionUID = 7403488805532662065L;

public DuplicateLocationInPolyLineCheck(final Configuration configuration)
Expand All @@ -45,15 +45,31 @@ protected Optional<CheckFlag> flag(final AtlasObject object)
{
final Set<Location> visitedLocations = new HashSet<>();
final Iterator<Location> locations = ((AtlasItem) object).getRawGeometry().iterator();
var firstHit = 0;
Location first = null;
while (locations.hasNext())
{
final Location currentLocation = locations.next();
final var currentLocation = locations.next();

if (first == null)
{
first = currentLocation;
}
else if (first.equals(currentLocation))
{
firstHit++;
}

if (visitedLocations.contains(currentLocation)
&& !this.isFlagged(object.getOsmIdentifier()))
&& !this.isFlagged(object.getOsmIdentifier())
&& (!first.equals(currentLocation) && locations.hasNext()
|| object instanceof Area || firstHit > 1))
{
this.markAsFlagged(object.getOsmIdentifier());
return Optional.of(createFlag(object, this.getLocalizedInstruction(0,
currentLocation.toString(), object.getOsmIdentifier())));
return Optional.of(this.createFlag(object,
this.getLocalizedInstruction(0, currentLocation.toString(),
object.getOsmIdentifier()),
Collections.singletonList(currentLocation)));
}
else
{
Expand Down
222 changes: 188 additions & 34 deletions ...rg/openstreetmap/atlas/checks/validation/points/DuplicateLocationInPolyLineCheckTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,23 @@
package org.openstreetmap.atlas.checks.validation.points;

import java.util.ArrayList;
import java.util.HashMap;
import static org.junit.Assert.assertEquals;

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
import org.openstreetmap.atlas.checks.flag.CheckFlag;
import org.openstreetmap.atlas.geography.Location;
import org.openstreetmap.atlas.geography.PolyLine;
import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
import org.openstreetmap.atlas.geography.atlas.Atlas;
import org.openstreetmap.atlas.geography.atlas.builder.AtlasBuilder;
import org.openstreetmap.atlas.geography.atlas.items.Area;
import org.openstreetmap.atlas.geography.atlas.items.Edge;
import org.openstreetmap.atlas.geography.atlas.items.Line;
import org.openstreetmap.atlas.geography.atlas.items.Node;
import org.openstreetmap.atlas.geography.atlas.items.Point;
import org.openstreetmap.atlas.geography.atlas.packed.PackedAtlasBuilder;
import org.openstreetmap.atlas.utilities.collections.Iterables;

Expand All @@ -21,34 +26,183 @@
*/
public class DuplicateLocationInPolyLineCheckTest
{
@Rule
public final DuplicateLocationInPolyLineCheckTestRule setup = new DuplicateLocationInPolyLineCheckTestRule();

@Rule
public final ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();

/**
* Reverse the polyline geometries in an atlas
*
* @param atlas
* The atlas with the lines to reverse
* @return A new atlas
*/
private static Atlas reverseLineItems(final Atlas atlas)
{
final AtlasBuilder atlasBuilder = new PackedAtlasBuilder();
for (final Point point : atlas.points())
{
atlasBuilder.addPoint(point.getIdentifier(), point.getLocation(), point.getTags());
}
for (final Node node : atlas.nodes())
{
atlasBuilder.addNode(node.getIdentifier(), node.getLocation(), node.getTags());
}
for (final Area area : atlas.areas())
{
atlasBuilder.addArea(area.getIdentifier(), area.asPolygon().reversed(), area.getTags());
}
for (final Line line : atlas.lines())
{
atlasBuilder.addLine(line.getIdentifier(), line.asPolyLine().reversed(),
line.getTags());
}
for (final Edge line : atlas.edges())
{
atlasBuilder.addEdge(line.getIdentifier(), line.asPolyLine().reversed(),
line.getTags());
}
return atlasBuilder.get();
}

@Test
public void testInvalidFigureEight()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(this.setup.getInvalidFigureEightLine(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* The simple area is invalid since the last location should not be the same as the first
* location
*/
@Test
public void testInvalidSimpleArea()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(this.setup.getInvalidSimpleArea(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* The simple area is invalid since the last location should not be the same as the first
* location
*/
@Test
public void testInvalidSimpleAreaReversed()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(reverseLineItems(this.setup.getInvalidSimpleArea()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testInvalidSimpleEdge()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(this.setup.getInvalidSimpleEdge(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testInvalidSimpleEdgeAlreadyChecked()
{
// Cannot use verifier -- it doesn't run the same id through twice
final DuplicateLocationInPolyLineCheck check = new DuplicateLocationInPolyLineCheck(
ConfigurationResolver.emptyConfiguration());
final List<CheckFlag> found = IntStream.range(0, 10)
.mapToObj(randomVar -> check.flags(this.setup.getInvalidSimpleEdge()))
.flatMap(iterable -> Iterables.asList(iterable).stream())
.collect(Collectors.toList());
assertEquals(1, found.size());
// Needed to avoid verifier complaints
this.testInvalidSimpleEdge();
}

@Test
public void testInvalidSimpleEdgeReversed()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(reverseLineItems(this.setup.getInvalidSimpleEdge()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testCheck()
{
final AtlasBuilder builder = new PackedAtlasBuilder();
final Map<String, String> tags = new HashMap<String, String>();
final Location location1 = Location.TEST_6;
final Location location2 = Location.TEST_2;
final Location location3 = Location.TEST_1;
final Location location4 = Location.TEST_6;
final List<Location> locations = new ArrayList<Location>();
locations.add(location1);
locations.add(location2);
locations.add(location3);
locations.add(location4);

final PolyLine polyLine = new PolyLine(locations);
builder.addNode(1, location1, tags);
builder.addNode(2, location2, tags);
builder.addNode(3, location3, tags);
builder.addNode(4, location4, tags);

builder.addEdge(1, polyLine, tags);

final Atlas atlas = builder.get();

final List<CheckFlag> flags = Iterables.asList(
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration())
.flags(atlas));
Assert.assertEquals(1, flags.size());
public void testInvalidSimpleLine()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(this.setup.getInvalidSimpleLine(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testInvalidSimpleLineReversed()
{
this.verifier.verifyExpectedSize(1);
this.verifier.actual(reverseLineItems(this.setup.getInvalidSimpleLine()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testValidServiceHighway()
{
this.verifier.verifyEmpty();
this.verifier.actual(this.setup.getValidSimpleServiceEdge(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

@Test
public void testValidServiceHighwayReversed()
{
this.verifier.verifyEmpty();
this.verifier.actual(reverseLineItems(this.setup.getValidSimpleServiceEdge()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* A loop is not invalid
*/
@Test
public void testValidSimpleEdgeLoop()
{
this.verifier.verifyEmpty();
this.verifier.actual(this.setup.getValidSimpleEdge(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* A loop is not invalid
*/
@Test
public void testValidSimpleEdgeLoopReversed()
{
this.verifier.verifyEmpty();
this.verifier.actual(reverseLineItems(this.setup.getValidSimpleEdge()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* A loop is not invalid
*/
@Test
public void testValidSimpleLineItemLoop()
{
this.verifier.verifyEmpty();
this.verifier.actual(this.setup.getValidSimpleLine(),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}

/**
* A loop is not invalid
*/
@Test
public void testValidSimpleLineItemLoopReversed()
{
this.verifier.verifyEmpty();
this.verifier.actual(reverseLineItems(this.setup.getValidSimpleLine()),
new DuplicateLocationInPolyLineCheck(ConfigurationResolver.emptyConfiguration()));
}
}
Loading

0 comments on commit eb31259

Please sign in to comment.