Skip to content

Commit

Permalink
Don't update node positions and nodes in ways in vector dataset
Browse files Browse the repository at this point in the history 
This can cause issues where a visible node cannot be clicked since it
does not exist in the dataset.

In addition, some lint issues have been fixed.

Signed-off-by: Taylor Smock <[email protected]>
  • Loading branch information
@tsmock
tsmock committed Aug 8, 2024
1 parent a1a4eec commit 772dd5e
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 86 deletions.
35 changes: 35 additions & 0 deletions pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,41 @@
</archive>
</configuration>
</plugin>
<plugin>
<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>2.43.0</version>
<configuration>
<java>
<!-- Avoid large formatting commits. -->
<ratchetFrom>origin/master</ratchetFrom>
<eclipse>
<version>4.21.0</version>
<file>${project.basedir}/config/format/code_format.xml</file>
</eclipse>
<removeUnusedImports/>
<endWithNewline/>
<indent>
<spaces>true</spaces>
<spacesPerTab>4</spacesPerTab>
</indent>
<importOrder>
<order>java,javax,</order>
</importOrder>
<trimTrailingWhitespace/>
<licenseHeader>
<content>// License: GPL. For details, see LICENSE file.</content>
</licenseHeader>
</java>
</configuration>
<executions>
<execution>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
93 changes: 47 additions & 46 deletions src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/DataMouseListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import java.util.Optional;
import java.util.concurrent.locks.Lock;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;

import javax.swing.event.MouseInputAdapter;
Expand Down Expand Up @@ -85,14 +86,14 @@ private static void mouseClickedInner(final MouseEvent e, final MVTLayer layer,
LatLon click = MainApplication.getMap().mapView.getLatLon(e.getX(), e.getY());
Optional<INode> nodes = searchNodes(layer, searchBBox).stream().distinct().filter(AbstractPrimitive::isDrawable)
.filter(INode.class::isInstance).map(INode.class::cast).filter(i -> i.getUniqueId() > 0)
.sorted(Comparator.comparingDouble(i -> i.distanceSq(click))).findFirst();
.min(Comparator.comparingDouble(i -> i.distanceSq(click)));
if (nodes.isPresent()) {
// This is needed since Mapillary ids are only unique within a tile.
layer.getData().setSelected(Collections.singletonList(nodes.get()));
} else if (layer instanceof MapillaryLayer) {
} else if (layer instanceof MapillaryLayer mapillaryLayer) {
if (e.getClickCount() >= MapillaryProperties.DESELECT_CLICK_COUNT.get()) {
layer.getData().clearSelection();
((MapillaryLayer) layer).setCurrentImage(null);
mapillaryLayer.setCurrentImage(null);
}
} else {
Collection<VectorWay> ways = layer.getData().searchWays(searchBBox);
Expand All @@ -115,64 +116,64 @@ public void mouseMoved(MouseEvent e) {
final BBox searchBBox = getSmallBBox(e.getPoint());
for (MVTLayer layer : MainApplication.getLayerManager().getLayersOfType(MVTLayer.class)) {
if (layer instanceof MapillaryLayer || layer instanceof PointObjectLayer) {
final Lock readLock = layer.getData().getReadLock();
if (readLock.tryLock()) {
layer.getData().getPrimitivesById(layer.getData().getHighlighted().toArray(new PrimitiveId[0]))
.forEach(vectorPrimitive -> vectorPrimitive.setHighlighted(false));
try {
if (searchBBox == null) {
layer.getData().setHighlighted(Collections.emptyList());
continue;
}
if (layer instanceof MapillaryLayer) {
MapillaryNode node = ((MapillaryLayer) layer).getImage();
if (node != null && node.getSequence() != null) {
node.getSequence().getNodes().forEach(n -> n.setHighlighted(false));
}
}
List<? extends AbstractPrimitive> nodes = searchNodes(layer, searchBBox);
if (!nodes.isEmpty()) {
layer.getData().setHighlighted(nodes.stream().filter(IPrimitive::isDrawable)
.map(IPrimitive::getPrimitiveId).collect(Collectors.toSet()));
nodes.forEach(node -> node.setHighlighted(true));
layer.invalidate();
continue;
}
Collection<VectorWay> ways = SubclassFilteredCollection.filter(
layer.getData().searchWays(searchBBox),
way -> convertToWaySegmentBBox(way).anyMatch(searchBBox::intersects));
if (!ways.isEmpty()) {
layer.getData().setHighlighted(
ways.stream().map(IPrimitive::getPrimitiveId).collect(Collectors.toSet()));
layer.invalidate();
} else if (!layer.getData().getHighlighted().isEmpty()) {
layer.getData().setHighlighted(Collections.emptyList());
}
} finally {
readLock.unlock();
performHighlight(layer, searchBBox);
}
}
}

private static void performHighlight(MVTLayer layer, BBox searchBBox) {
final Lock readLock = layer.getData().getReadLock();
if (readLock.tryLock()) {
layer.getData().getPrimitivesById(layer.getData().getHighlighted().toArray(new PrimitiveId[0]))
.forEach(vectorPrimitive -> vectorPrimitive.setHighlighted(false));
try {
if (searchBBox == null) {
layer.getData().setHighlighted(Collections.emptyList());
return;
}
if (layer instanceof MapillaryLayer mapillaryLayer) {
MapillaryNode node = mapillaryLayer.getImage();
if (node != null && node.getSequence() != null) {
node.getSequence().getNodes().forEach(n -> n.setHighlighted(false));
}
}
List<? extends AbstractPrimitive> nodes = searchNodes(layer, searchBBox);
if (!nodes.isEmpty()) {
layer.getData().setHighlighted(nodes.stream().filter(IPrimitive::isDrawable)
.map(IPrimitive::getPrimitiveId).collect(Collectors.toSet()));
nodes.forEach(node -> node.setHighlighted(true));
layer.invalidate();
return;
}
Collection<VectorWay> ways = SubclassFilteredCollection.filter(layer.getData().searchWays(searchBBox),
way -> convertToWaySegmentBBox(way).anyMatch(searchBBox::intersects));
if (!ways.isEmpty()) {
layer.getData()
.setHighlighted(ways.stream().map(IPrimitive::getPrimitiveId).collect(Collectors.toSet()));
layer.invalidate();
} else if (!layer.getData().getHighlighted().isEmpty()) {
layer.getData().setHighlighted(Collections.emptyList());
}
} finally {
readLock.unlock();
}
}
}

private static <N extends INode, W extends IWay<N>> Stream<BBox> convertToWaySegmentBBox(W way) {
// TODO: Stream.iterate would be good here (Java 9)
Stream.Builder<BBox> builder = Stream.builder();
for (int i = 0; i < way.getNodesCount() - 1; i++) {
return IntStream.iterate(0, i -> i < way.getNodesCount() - 1, i -> i + 1).mapToObj(i -> {
BBox bbox = new BBox(way.getNode(i));
bbox.add(way.getNode(i + 1));
builder.add(bbox);
}
return builder.build();
return bbox;
});
}

private static List<? extends AbstractPrimitive> searchNodes(MVTLayer layer, BBox searchBBox) {
if (searchBBox == null) {
return Collections.emptyList();
}
if (layer instanceof MapillaryLayer) {
final MapillaryNode image = ((MapillaryLayer) layer).getImage();
if (layer instanceof MapillaryLayer mapillaryLayer) {
final MapillaryNode image = mapillaryLayer.getImage();
if (image != null) {
final List<AbstractPrimitive> nodes;
if (image.getSequence() != null) {
Expand Down
28 changes: 10 additions & 18 deletions src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/layer/MapillaryLayer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,15 +351,15 @@ private void paintWithLock(final Graphics2D g, final MapView mv, final Bounds bo
final double distPer100Pixel = mv.getDist100Pixel();
final String sequenceKey = MapillaryImageUtils.getSequenceKey(selectedImage);
for (IWay<?> seq : getData().searchWays(box.toBBox()).stream()
.sorted(Comparator.comparingInt(IWay::getRawTimestamp)).distinct().collect(Collectors.toList())) {
.sorted(Comparator.comparingInt(IWay::getRawTimestamp)).distinct().toList()) {
if (!Objects.equals(sequenceKey, MapillarySequenceUtils.getKey(seq))) {
drawSequence(g, mv, seq, selectedImage, originalTransform, distPer100Pixel);
}
}

// Paint single images (see GH #219)
for (INode node : getData().searchNodes(box.toBBox()).stream().filter(node -> !node.isReferredByWays(1))
.sorted(Comparator.comparingInt(INode::getRawTimestamp)).distinct().collect(Collectors.toList())) {
.sorted(Comparator.comparingInt(INode::getRawTimestamp)).distinct().toList()) {
drawImageMarker(originalTransform, selectedImage, g, node, distPer100Pixel, false, null);
}

Expand Down Expand Up @@ -435,9 +435,7 @@ private Color getColor(final INode node) {
MapillaryImageUtils.getDate(node);
}
return this.dateScale.getColor(node.getRawTimestamp());
case VELOCITY_FOOT:
case VELOCITY_BIKE:
case VELOCITY_CAR:
case VELOCITY_FOOT, VELOCITY_BIKE, VELOCITY_CAR:
final double normalized = calculateVelocity(node);
if (Double.isNaN(normalized)) {
return this.velocityScale.getNoDataColor();
Expand Down Expand Up @@ -545,9 +543,9 @@ private void drawImageMarker(final AffineTransform originalTransform, final INod

if (offset && selectedImg == img) {
final ILatLon drawnCoordinates = OffsetUtils.getOffsetLocation(img);
final Point offsetP = drawnCoordinates instanceof INode
final Point offsetP = drawnCoordinates instanceof INode node
// INode implementations may optimize getEastNorth, so prefer that where possible.
? mv.getPoint(((INode) drawnCoordinates).getEastNorth())
? mv.getPoint(node.getEastNorth())
: mv.getPoint(drawnCoordinates);
paintDirectionIndicator(g, directionC, img, originalTransform, offsetP, i);
g.setTransform(originalTransform);
Expand Down Expand Up @@ -689,11 +687,6 @@ public String getChangesetSourceTag() {
return isApplicable() ? "Mapillary" : null;
}

@Override
public boolean isMergable(Layer other) {
return false;
}

@Override
public void mergeFrom(Layer from) {
throw new UnsupportedOperationException("This layer does not support merging yet");
Expand Down Expand Up @@ -750,7 +743,7 @@ public void layerAdded(LayerAddEvent e) {
@Override
public void layerRemoving(LayerRemoveEvent e) {
List<DataSet> currentDataSets = MainApplication.getLayerManager().getLayersOfType(OsmDataLayer.class).stream()
.map(OsmDataLayer::getDataSet).collect(Collectors.toList());
.map(OsmDataLayer::getDataSet).toList();
for (Map.Entry<DataSet, Set<INode>> entry : imageViewedMap.entrySet()) {
if (!currentDataSets.contains(entry.getKey())) {
imageViewedMap.remove(entry.getKey());
Expand Down Expand Up @@ -852,7 +845,7 @@ public void selectionChanged(
}
}

new MapillaryNodeDownloader(node, MapillaryLayer.getInstance()::setCurrentImage).execute();
new MapillaryNodeDownloader(node, this::setCurrentImage).execute();
}
}

Expand Down Expand Up @@ -899,15 +892,14 @@ public List<? extends IImageEntry<?>> getSelection() {

@Override
public boolean containsImage(IImageEntry<?> imageEntry) {
if (!(imageEntry instanceof MapillaryImageEntry)) {
if (!(imageEntry instanceof MapillaryImageEntry entry)) {
return false;
}
MapillaryImageEntry entry = (MapillaryImageEntry) imageEntry;
if (Objects.equals(entry.getImage(), this.image)) {
return true;
}
if (entry.getImage() instanceof VectorNode) {
return this.getData().containsNode((VectorNode) entry.getImage());
if (entry.getImage()instanceof VectorNode vectorNode) {
return this.getData().containsNode(vectorNode);
}
return this.getData().getPrimitiveById(entry.getImage().getPrimitiveId()) != null;
}
Expand Down
22 changes: 0 additions & 22 deletions ...ava/org/openstreetmap/josm/plugins/mapillary/gui/workers/MapillarySequenceDownloader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,11 @@
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Lock;
import java.util.function.Consumer;

import org.openstreetmap.josm.data.vector.VectorDataSet;
import org.openstreetmap.josm.data.vector.VectorNode;
import org.openstreetmap.josm.plugins.mapillary.data.mapillary.MapillaryDownloader;
import org.openstreetmap.josm.plugins.mapillary.data.mapillary.MapillaryNode;
import org.openstreetmap.josm.plugins.mapillary.data.mapillary.MapillarySequence;
import org.openstreetmap.josm.plugins.mapillary.gui.layer.MapillaryLayer;
import org.openstreetmap.josm.plugins.mapillary.utils.MapillaryImageUtils;
import org.openstreetmap.josm.spi.preferences.Config;
import org.openstreetmap.josm.tools.JosmRuntimeException;
Expand Down Expand Up @@ -181,24 +177,6 @@ protected void done() {
@Override
protected void process(List<MapillarySequence> chunks) {
super.process(chunks);
VectorDataSet ds = MapillaryLayer.getInstance().getData();
// Technically a writeLock would be better, but we cannot get that with the current VectorDataSet
// implementation.
final Lock dsLock = ds.getReadLock();
try {
dsLock.lock();
for (MapillarySequence seq : chunks) {
for (MapillaryNode oldNode : seq.getNodes()) {
VectorNode oldPrimitive = (VectorNode) ds.getPrimitiveById(oldNode);
if (oldPrimitive != null) {
oldPrimitive.putAll(oldNode.getKeys());
oldPrimitive.setCoor(oldNode.getCoor());
}
}
}
} finally {
dsLock.unlock();
}
// The counter just avoids many resets of the imagery window in short order
if (!chunks.isEmpty() && counter.getAndAdd(chunks.size()) < 3) {
this.updater.accept(chunks.get(chunks.size() - 1));
Expand Down

0 comments on commit 772dd5e

Please sign in to comment.