Skip to content

Commit

Permalink
fix #19465: Make "Overlapping ways" less aggressive
Browse files Browse the repository at this point in the history 
- new preference overlapping-ways.ignore-layer with default false
- new preference overlapping-ways.only-known-linear with default true
- new message texts
- improved list in method hasAreaTags(). Probably many more candidates could be added.
- still produces info message Ways share segment when overlapping-ways.only-known-linear is changed to false. I tend to think that this test can be removed. It's kind of a mop up message for ways which are not clearly linear or area. 
- fix typo in TestError javadoc

git-svn-id: https://josm.openstreetmap.de/svn/trunk@17350 0c6e7542-c601-0410-84e7-c038aed88b3b
  • Loading branch information
GerdP authored and GerdP committed Nov 24, 2020
1 parent a96c8c7 commit 4c7fd82
Show file tree
Hide file tree
Showing 3 changed files with 149 additions and 76 deletions.
7 changes: 5 additions & 2 deletions src/org/openstreetmap/josm/data/osm/OsmPrimitive.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1117,13 +1117,16 @@ public static Set<Relation> getParentRelations(Collection<? extends OsmPrimitive
* @since 6491
*/
public final boolean hasAreaTags() {
return hasKey("landuse", "amenity", "building", "building:part")
return hasKey("building", "landuse", "amenity", "shop", "building:part", "boundary", "historic", "place")
|| hasTag("area", OsmUtils.TRUE_VALUE)
|| hasTag("waterway", "riverbank")
|| hasTag("highway", "rest_area", "services", "platform")
|| hasTag("railway", "platform")
|| hasTagDifferent("leisure", "picnic_table", "slipway", "firepit")
|| hasTag("natural", "water", "wood", "scrub", "wetland", "grassland", "heath", "rock", "bare_rock",
"sand", "beach", "scree", "bay", "glacier", "shingle", "fell", "reef", "stone",
"mud", "landslide", "sinkhole", "crevasse", "desert");
"mud", "landslide", "sinkhole", "crevasse", "desert")
|| hasTag("aeroway", "aerodrome");
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/org/openstreetmap/josm/data/validation/TestError.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public Builder messageWithManuallyTranslatedDescription(String message, String d
/**
* Sets the error message.
*
* @param message The the message of this error group
* @param message The message of this error group
* @param marktrDescription The {@linkplain I18n#marktr prepared for i18n} description of this error
* @param args The description arguments to be applied in {@link I18n#tr(String, Object...)}
* @return {@code this}
Expand Down
216 changes: 143 additions & 73 deletions src/org/openstreetmap/josm/data/validation/tests/OverlappingWays.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import static org.openstreetmap.josm.data.validation.tests.CrossingWays.HIGHWAY;
import static org.openstreetmap.josm.data.validation.tests.CrossingWays.RAILWAY;
import static org.openstreetmap.josm.data.validation.tests.CrossingWays.WATERWAY;
import static org.openstreetmap.josm.tools.I18n.tr;

import java.util.ArrayList;
Expand All @@ -11,11 +12,13 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import org.openstreetmap.josm.data.osm.Node;
import org.openstreetmap.josm.data.osm.OsmPrimitive;
Expand All @@ -24,10 +27,12 @@
import org.openstreetmap.josm.data.osm.Way;
import org.openstreetmap.josm.data.osm.WaySegment;
import org.openstreetmap.josm.data.preferences.ListProperty;
import org.openstreetmap.josm.data.preferences.sources.ValidatorPrefHelper;
import org.openstreetmap.josm.data.validation.Severity;
import org.openstreetmap.josm.data.validation.Test;
import org.openstreetmap.josm.data.validation.TestError;
import org.openstreetmap.josm.gui.progress.ProgressMonitor;
import org.openstreetmap.josm.spi.preferences.Config;
import org.openstreetmap.josm.tools.MultiMap;
import org.openstreetmap.josm.tools.Pair;

Expand All @@ -42,13 +47,22 @@ public class OverlappingWays extends Test {
/** Bag of all way segments */
private MultiMap<Pair<Node, Node>, WaySegment> nodePairs;

private boolean onlyKnwonLinear;
private boolean includeOther;
private boolean ignoreLayer;

protected static final int OVERLAPPING_HIGHWAY = 101;
protected static final int OVERLAPPING_RAILWAY = 102;
protected static final int OVERLAPPING_WAY = 103;
protected static final int OVERLAPPING_WATERWAY = 104;
protected static final int OVERLAPPING_HIGHWAY_AREA = 111;
protected static final int OVERLAPPING_RAILWAY_AREA = 112;
protected static final int OVERLAPPING_WAY_AREA = 113;
protected static final int OVERLAPPING_WATERWAY_AREA = 114;
protected static final int DUPLICATE_WAY_SEGMENT = 121;
protected static final int OVERLAPPING_HIGHWAY_LINEAR_WAY = 131;
protected static final int OVERLAPPING_RAILWAY_LINEAR_WAY = 132;
protected static final int OVERLAPPING_WATERWAY_LINEAR_WAY = 133;

protected static final ListProperty IGNORED_KEYS = new ListProperty(
"overlapping-ways.ignored-keys", Arrays.asList(
Expand All @@ -68,102 +82,152 @@ public OverlappingWays() {
public void startTest(ProgressMonitor monitor) {
super.startTest(monitor);
nodePairs = new MultiMap<>(1000);
includeOther = isBeforeUpload ? ValidatorPrefHelper.PREF_OTHER_UPLOAD.get() : ValidatorPrefHelper.PREF_OTHER.get();
onlyKnwonLinear = Config.getPref().getBoolean("overlapping-ways.only-known-linear", true);
ignoreLayer = Config.getPref().getBoolean("overlapping-ways.ignore-layer", false);
}

private static boolean parentMultipolygonConcernsArea(OsmPrimitive p) {
return p.referrers(Relation.class)
.anyMatch(Relation::concernsArea);
.anyMatch(Relation::isMultipolygon);
}

@Override
public void endTest() {
Map<List<Way>, Set<WaySegment>> seenWays = new HashMap<>(500);

Collection<TestError> preliminaryErrors = new ArrayList<>();
for (Set<WaySegment> duplicated : nodePairs.values()) {
int ways = duplicated.size();
if (duplicated.size() <= 1)
continue;
if (ignoreLayer) {
analyseOverlaps(duplicated, seenWays);
} else {
// group by layer tag value (which is very likely null)
Map<String, Set<WaySegment>> grouped = new HashMap<>();
for (WaySegment ws : duplicated) {
// order in set is important
grouped.computeIfAbsent(OsmUtils.getLayer(ws.way), k -> new LinkedHashSet<>()).add(ws);
}
grouped.values().forEach(group -> analyseOverlaps(group, seenWays));
}
}
nodePairs = null;

if (ways > 1) {
List<OsmPrimitive> prims = new ArrayList<>();
List<Way> currentWays = new ArrayList<>();
Collection<WaySegment> highlight;
int highway = 0;
int railway = 0;
int area = 0;
super.endTest();
}

for (WaySegment ws : duplicated) {
if (ws.way.hasKey(HIGHWAY)) {
highway++;
} else if (ws.way.hasKey(RAILWAY)) {
railway++;
private void analyseOverlaps(Set<WaySegment> duplicated, Map<List<Way>, Set<WaySegment>> seenWays) {
int ways = duplicated.size();
if (ways <= 1)
return;

List<Way> currentWays = duplicated.stream().map(ws -> ws.way).collect(Collectors.toList());
Collection<WaySegment> highlight;
if ((highlight = seenWays.get(currentWays)) != null) {
/* this combination of ways was seen before, just add highlighted segment */
highlight.addAll(duplicated);
} else {
int countHighway = 0;
int countRailway = 0;
int countWaterway = 0;
int countOther = 0;
int numAreas = 0;
for (WaySegment ws : duplicated) {
boolean isArea = ws.way.concernsArea();
if (ws.way.hasKey(HIGHWAY)) {
if (!isArea) {
countHighway++;
}
Boolean ar = OsmUtils.getOsmBoolean(ws.way.get("area"));
if (ar != null && ar) {
area++;
} else if (ws.way.hasKey(RAILWAY)) {
if (!isArea) {
countRailway++;
}
if (ws.way.concernsArea() || parentMultipolygonConcernsArea(ws.way)) {
area++;
ways--;
} else if (ws.way.hasKey(WATERWAY)) {
if (!isArea) {
countWaterway++;
}

prims.add(ws.way);
currentWays.add(ws.way);
}
// These ways not seen before
// If two or more of the overlapping ways are highways or railways mark a separate error
if ((highlight = seenWays.get(currentWays)) == null) {
String errortype;
int type;

if (area > 0) {
if (ways == 0 || duplicated.size() == area) {
continue; // We previously issued an annoying "Areas share segment" warning
} else if (highway == ways) {
errortype = tr("Highways share segment with area");
type = OVERLAPPING_HIGHWAY_AREA;
} else if (railway == ways) {
errortype = tr("Railways share segment with area");
type = OVERLAPPING_RAILWAY_AREA;
} else {
errortype = tr("Ways share segment with area");
type = OVERLAPPING_WAY_AREA;
}
} else if (highway == ways) {
errortype = tr("Overlapping highways");
type = OVERLAPPING_HIGHWAY;
} else if (railway == ways) {
errortype = tr("Overlapping railways");
type = OVERLAPPING_RAILWAY;
} else {
errortype = tr("Overlapping ways");
type = OVERLAPPING_WAY;
} else {
if (ws.way.getInterestingTags().isEmpty() && parentMultipolygonConcernsArea(ws.way))
isArea = true;
if (!isArea && isOtherLinear(ws.way)) {
countOther++;
}

Severity severity = type < OVERLAPPING_HIGHWAY_AREA ? Severity.WARNING : Severity.OTHER;
preliminaryErrors.add(TestError.builder(this, severity, type)
.message(errortype)
.primitives(prims)
.highlightWaySegments(duplicated)
.build());
seenWays.put(currentWays, duplicated);
} else { /* way seen, mark highlight layer only */
highlight.addAll(duplicated);
}
if (isArea) {
numAreas++;
}
}
}
if (numAreas == ways) {
// no linear object, we don't care when areas share segments
return;
}

// see ticket #9598 - only report if at least 3 segments are shared, except for overlapping ways, i.e warnings (see #9820)
for (TestError error : preliminaryErrors) {
if (error.getSeverity() == Severity.WARNING || error.getHighlighted().size() / error.getPrimitives().size() >= 3) {
boolean ignore = error.getPrimitives().stream().anyMatch(IGNORED);
if (!ignore) {
errors.add(error);
}

// If two or more of the overlapping ways are highways or railways mark a separate error
String errortype;
int type;
int allKnownLinear = countHighway + countRailway + countWaterway + countOther;
final Severity severity;
if (countHighway > 1) {
errortype = tr("Overlapping highways");
type = OVERLAPPING_HIGHWAY;
severity = Severity.ERROR;
} else if (countRailway > 1) {
errortype = tr("Overlapping railways");
type = OVERLAPPING_RAILWAY;
severity = Severity.ERROR;
} else if (countWaterway > 1) {
errortype = tr("Overlapping waterways");
type = OVERLAPPING_WATERWAY;
severity = Severity.ERROR;
} else if (countHighway > 0 && countHighway < allKnownLinear) {
errortype = tr("Highway shares segment with linear way");
type = OVERLAPPING_HIGHWAY_LINEAR_WAY;
severity = Severity.WARNING;
} else if (countRailway > 0 && countRailway < allKnownLinear) {
errortype = tr("Railway shares segment with linear way");
type = OVERLAPPING_HIGHWAY_LINEAR_WAY;
severity = Severity.WARNING;
} else if (countWaterway > 0 && countWaterway < allKnownLinear) {
errortype = tr("Waterway shares segment with linear way");
type = OVERLAPPING_WATERWAY_LINEAR_WAY;
severity = Severity.WARNING;
} else if (!includeOther || onlyKnwonLinear) {
return;
} else if (countHighway > 0) {
errortype = tr("Highway shares segment with other way");
type = OVERLAPPING_HIGHWAY_AREA;
severity = Severity.OTHER;
} else if (countRailway > 0) {
errortype = tr("Railway shares segment with other way");
type = OVERLAPPING_RAILWAY_AREA;
severity = Severity.OTHER;
} else if (countWaterway > 0) {
errortype = tr("Waterway shares segment with other way");
type = OVERLAPPING_WATERWAY_AREA;
severity = Severity.OTHER;
} else {
errortype = tr("Ways share segment");
type = OVERLAPPING_WAY;
severity = Severity.OTHER;
}

List<OsmPrimitive> prims = new ArrayList<>(currentWays);
errors.add(TestError.builder(this, severity, type)
.message(errortype)
.primitives(prims)
.highlightWaySegments(duplicated)
.build());
seenWays.put(currentWays, duplicated);
}
}

super.endTest();
nodePairs = null;
private static boolean isOtherLinear(Way way) {
// it is assumed that area=* was evaluated before and is false
return (way.hasKey("barrier", "addr:interpolation", "route", "ford")
|| way.hasTag("natural", "tree_row", "cliff", "ridge")
|| way.hasTag("power", "line", "minor_line", "cable", "portal")
|| way.hasTag("man_made", "pipeline"));
}

protected static Set<WaySegment> checkDuplicateWaySegment(Way w) {
Expand Down Expand Up @@ -202,6 +266,12 @@ public void visit(Way w) {
return;
}

if (IGNORED.test(w))
return;

if (onlyKnwonLinear && (w.concernsArea() || w.getInterestingTags().isEmpty()))
return;

Node lastN = null;
int i = -2;
for (Node n : w.getNodes()) {
Expand Down

0 comments on commit 4c7fd82

Please sign in to comment.