From aa637225c804d908fcca0aca55593075050cc659 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 08:18:39 -0700 Subject: [PATCH 01/10] Clean up walker and points `ZoneWalker.replaceLastWaypoint()` does not need to return a value, so that has been removed. `ZoneWalker.getPath()` and `SelectionSet.getGridlessPath()` never return `null`, so mark them as such and clean up callers to not check for it. `AbstractPoint` is now sealed with `ZonePoint` and `CellPoint`. We do a lot of casework that already assumes only those two possibilities, and now that is formal. It will also allow for pattern matching. --- .../client/ui/zone/renderer/SelectionSet.java | 3 +- .../client/walker/AbstractZoneWalker.java | 12 ++++---- .../maptool/client/walker/NaiveWalker.java | 29 ++----------------- .../maptool/client/walker/ZoneWalker.java | 7 +++-- .../rptools/maptool/model/AbstractPoint.java | 2 +- .../net/rptools/maptool/model/CellPoint.java | 2 +- .../java/net/rptools/maptool/model/Path.java | 2 +- .../net/rptools/maptool/model/ZonePoint.java | 2 +- 8 files changed, 18 insertions(+), 41 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java index 036aaa7782..40f295defd 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java @@ -18,6 +18,7 @@ import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import javax.annotation.Nonnull; import net.rptools.maptool.client.MapTool; import net.rptools.maptool.client.ui.zone.RenderPathWorker; import net.rptools.maptool.client.walker.ZoneWalker; @@ -80,7 +81,7 @@ public SelectionSet( /** * @return path computation. */ - public Path getGridlessPath() { + public @Nonnull Path getGridlessPath() { return gridlessPath; } diff --git a/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java b/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java index 02ab712942..af39dd09d8 100644 --- a/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java +++ b/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java @@ -20,6 +20,7 @@ import java.util.List; import java.util.ListIterator; import java.util.Set; +import javax.annotation.Nonnull; import net.rptools.maptool.client.ui.zone.RenderPathWorker; import net.rptools.maptool.model.CellPoint; import net.rptools.maptool.model.Path; @@ -73,8 +74,8 @@ public void addWaypoints(CellPoint... points) { } } - public CellPoint replaceLastWaypoint(CellPoint point) { - return replaceLastWaypoint( + public void replaceLastWaypoint(CellPoint point) { + replaceLastWaypoint( point, false, Collections.singleton(TerrainModifierOperation.NONE), @@ -86,7 +87,7 @@ public CellPoint replaceLastWaypoint(CellPoint point) { } @Override - public CellPoint replaceLastWaypoint( + public void replaceLastWaypoint( CellPoint point, boolean restrictMovement, Set terrainModifiersIgnored, @@ -105,7 +106,7 @@ public CellPoint replaceLastWaypoint( this.tokenMbl = tokenMbl; if (partialPaths.isEmpty()) { - return null; + return; } PartialPath oldPartial = partialPaths.remove(partialPaths.size() - 1); @@ -115,7 +116,6 @@ public CellPoint replaceLastWaypoint( partialPaths.add( new PartialPath(oldPartial.start, point, calculatePath(oldPartial.start, point))); - return oldPartial.end; } public Path getPath(RenderPathWorker renderPathWorker) { @@ -123,7 +123,7 @@ public Path getPath(RenderPathWorker renderPathWorker) { return getPath(); } - public Path getPath() { + public @Nonnull Path getPath() { Path path = new Path<>(); synchronized (partialPaths) { diff --git a/src/main/java/net/rptools/maptool/client/walker/NaiveWalker.java b/src/main/java/net/rptools/maptool/client/walker/NaiveWalker.java index 3c1a686476..2826134304 100644 --- a/src/main/java/net/rptools/maptool/client/walker/NaiveWalker.java +++ b/src/main/java/net/rptools/maptool/client/walker/NaiveWalker.java @@ -16,20 +16,11 @@ import java.util.ArrayList; import java.util.List; -import java.util.Map; -import java.util.Set; import net.rptools.maptool.model.CellPoint; -import net.rptools.maptool.model.TokenFootprint; -import net.rptools.maptool.model.Zone; -public class NaiveWalker extends AbstractZoneWalker { - public NaiveWalker(Zone zone) { - super(zone); - } - - private double distance; +// Like a ZoneWalker, but only supports calculatePath(). +public class NaiveWalker { - @Override public List calculatePath(CellPoint start, CellPoint end) { List list = new ArrayList(); @@ -50,22 +41,6 @@ public List calculatePath(CellPoint start, CellPoint end) { count++; } - distance = (list.size() - 1) * 5; return list; } - - public double getDistance() { - return distance; - } - - @Override - public void setFootprint(TokenFootprint footprint) { - // Not needed/used here - System.out.println("Should not see this ever!"); - } - - @Override - public Map> getBlockedMoves() { - return null; - } } diff --git a/src/main/java/net/rptools/maptool/client/walker/ZoneWalker.java b/src/main/java/net/rptools/maptool/client/walker/ZoneWalker.java index 13ce99626f..4d1868d97a 100644 --- a/src/main/java/net/rptools/maptool/client/walker/ZoneWalker.java +++ b/src/main/java/net/rptools/maptool/client/walker/ZoneWalker.java @@ -17,6 +17,7 @@ import java.awt.geom.Area; import java.util.Map; import java.util.Set; +import javax.annotation.Nonnull; import net.rptools.maptool.client.ui.zone.RenderPathWorker; import net.rptools.maptool.model.CellPoint; import net.rptools.maptool.model.Path; @@ -29,9 +30,9 @@ public interface ZoneWalker { public void addWaypoints(CellPoint... point); - public CellPoint replaceLastWaypoint(CellPoint point); + public void replaceLastWaypoint(CellPoint point); - public CellPoint replaceLastWaypoint( + public void replaceLastWaypoint( CellPoint point, boolean restrictMovement, Set terrainModifiersIgnored, @@ -45,7 +46,7 @@ public CellPoint replaceLastWaypoint( public double getDistance(); - public Path getPath(); + public @Nonnull Path getPath(); public Path getPath(RenderPathWorker renderPathWorker); diff --git a/src/main/java/net/rptools/maptool/model/AbstractPoint.java b/src/main/java/net/rptools/maptool/model/AbstractPoint.java index 729d2a94af..20d73a1c04 100644 --- a/src/main/java/net/rptools/maptool/model/AbstractPoint.java +++ b/src/main/java/net/rptools/maptool/model/AbstractPoint.java @@ -16,7 +16,7 @@ import net.rptools.maptool.server.proto.drawing.IntPointDto; -public abstract class AbstractPoint implements Cloneable { +public abstract sealed class AbstractPoint implements Cloneable permits ZonePoint, CellPoint { public int x; public int y; diff --git a/src/main/java/net/rptools/maptool/model/CellPoint.java b/src/main/java/net/rptools/maptool/model/CellPoint.java index cfdcdf1f86..58d85ffbb7 100644 --- a/src/main/java/net/rptools/maptool/model/CellPoint.java +++ b/src/main/java/net/rptools/maptool/model/CellPoint.java @@ -26,7 +26,7 @@ * * @author trevor */ -public class CellPoint extends AbstractPoint { +public final class CellPoint extends AbstractPoint { public double distanceTraveled; // Only populated by AStarWalker classes to be used upstream public double diff --git a/src/main/java/net/rptools/maptool/model/Path.java b/src/main/java/net/rptools/maptool/model/Path.java index 22d900016f..8fcb47cfd6 100644 --- a/src/main/java/net/rptools/maptool/model/Path.java +++ b/src/main/java/net/rptools/maptool/model/Path.java @@ -126,7 +126,7 @@ public Path derive( path = (Path) processPath; } else if (!keyToken.isSnapToGrid() && followerToken.isSnapToGrid()) { - NaiveWalker nw = new NaiveWalker(zone); + NaiveWalker nw = new NaiveWalker(); Path processPath = new Path(); CellPoint prevPoint = grid.convert(new ZonePoint(startPoint.x, startPoint.y)); diff --git a/src/main/java/net/rptools/maptool/model/ZonePoint.java b/src/main/java/net/rptools/maptool/model/ZonePoint.java index 3b89a26244..f49a1e8cb4 100644 --- a/src/main/java/net/rptools/maptool/model/ZonePoint.java +++ b/src/main/java/net/rptools/maptool/model/ZonePoint.java @@ -14,7 +14,7 @@ */ package net.rptools.maptool.model; -public class ZonePoint extends AbstractPoint { +public final class ZonePoint extends AbstractPoint { public ZonePoint(int x, int y) { super(x, y); } From cd9d5a04b0f5cbe357c799122b8d22bde2a33b85 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 08:25:33 -0700 Subject: [PATCH 02/10] Clean up TokenMoveFunctions.getMovement() Avoid some case work and unnecessary `Path<>` creation. --- .../client/functions/TokenMoveFunctions.java | 44 +++++++------------ 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/functions/TokenMoveFunctions.java b/src/main/java/net/rptools/maptool/client/functions/TokenMoveFunctions.java index 30a14f7de1..b77c7a27e0 100644 --- a/src/main/java/net/rptools/maptool/client/functions/TokenMoveFunctions.java +++ b/src/main/java/net/rptools/maptool/client/functions/TokenMoveFunctions.java @@ -25,6 +25,7 @@ import java.math.BigDecimal; import java.text.NumberFormat; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -485,39 +486,27 @@ private String getMovement( Zone zone = zr.getZone(); Grid grid = zone.getGrid(); - Path gridlessPath; - /* - * Lee: causes an NPE when used on a newly dropped token. While a true solution would probably be to create a "path" based on the token's coords when it is dropped on the map, the easy out - * here would be to just return a "0". - * - * Final Edit: attempting to create a default path for new drops had undesirable effects. Therefore, let's opt for the easy fix - */ - int x = 0, y = 0; - - try { - x = source.getLastPath().getCellPath().get(0).x; - y = source.getLastPath().getCellPath().get(0).y; - } catch (NullPointerException e) { + List cellPath = + source.getLastPath() == null ? Collections.emptyList() : source.getLastPath().getCellPath(); + if (cellPath.isEmpty()) { return "0"; } if (useTerrainModifiers && !returnFractionOnly) { - if (source.getLastPath().getLastWaypoint() instanceof CellPoint) { - CellPoint cp = (CellPoint) source.getLastPath().getLastWaypoint(); + if (cellPath.getLast() instanceof CellPoint cp) { double trueDistance = cp.getDistanceTraveled(zone); - return new BigDecimal(trueDistance).stripTrailingZeros().toPlainString(); } } if (source.isSnapToGrid() && grid.getCapabilities().isSnapToGridSupported()) { if (zone.getGrid().getCapabilities().isPathingSupported()) { + var firstPoint = cellPath.getFirst(); List cplist = new ArrayList(); walker = grid.createZoneWalker(); - walker.replaceLastWaypoint(new CellPoint(x, y)); - for (AbstractPoint point : source.getLastPath().getCellPath()) { + walker.replaceLastWaypoint(new CellPoint(firstPoint.x, firstPoint.y)); + for (AbstractPoint point : cellPath) { CellPoint tokenPoint = new CellPoint(point.x, point.y); - // walker.setWaypoints(tokenPoint); walker.replaceLastWaypoint(tokenPoint); cplist.add(tokenPoint); } @@ -534,21 +523,18 @@ private String getMovement( // return Integer.toString(walker.getDistance()); } } else { - gridlessPath = new Path(); - for (AbstractPoint point : source.getLastPath().getCellPath()) { - gridlessPath.addPathCell(new ZonePoint(point.x, point.y)); - } double c = 0; - ZonePoint lastPoint = null; - for (ZonePoint zp : gridlessPath.getCellPath()) { + // Should be a ZonePoint, but the calculation just doesn't care. + AbstractPoint lastPoint = null; + for (var point : cellPath) { if (lastPoint == null) { - lastPoint = zp; + lastPoint = point; continue; } - int a = lastPoint.x - zp.x; - int b = lastPoint.y - zp.y; + int a = lastPoint.x - point.x; + int b = lastPoint.y - point.y; c += Math.hypot(a, b); - lastPoint = zp; + lastPoint = point; } c /= zone.getGrid().getSize(); // Number of "cells" c *= zone.getUnitsPerCell(); // "actual" distance traveled From b7d76a50ca9c19ff2527ddb5026436061ccd82aa Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 08:34:36 -0700 Subject: [PATCH 03/10] MeasureTool no longer modifies its path Instead it keeps its own last point that it can freely modify but that is kept out of the path until committed as a waypoint. Gridless path rendering is also cleaned up by using a Path2D. Even better would be to delegate to `ZoneRenderer.renderPath()` like we do for gridded paths, but that current assumes it needs to adjust for a token footprint, which `MeasureTool` does not require. --- .../maptool/client/tool/MeasureTool.java | 81 +++++++++---------- 1 file changed, 36 insertions(+), 45 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java b/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java index 348e1670fc..313abda586 100644 --- a/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java +++ b/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java @@ -14,6 +14,7 @@ */ package net.rptools.maptool.client.tool; +import com.google.common.collect.Iterables; import java.awt.Color; import java.awt.Cursor; import java.awt.Graphics2D; @@ -22,7 +23,9 @@ import java.awt.event.ActionEvent; import java.awt.event.KeyEvent; import java.awt.event.MouseEvent; +import java.awt.geom.Path2D; import java.text.NumberFormat; +import java.util.List; import java.util.Map; import javafx.application.Platform; import javafx.scene.ImageCursor; @@ -49,6 +52,8 @@ public class MeasureTool extends DefaultTool implements ZoneOverlay { private ZoneWalker walker; private Path gridlessPath; + private ZonePoint currentGridlessPoint; + private static Cursor measureCursor; private static javafx.scene.Cursor measureCursorFX; @@ -93,9 +98,6 @@ public String getInstructions() { } public void paintOverlay(ZoneRenderer renderer, Graphics2D g) { - if (walker == null && gridlessPath == null) { - return; - } if (walker != null) { renderer.renderPath(g, walker.getPath(), renderer.getZone().getGrid().getDefaultFootprint()); ScreenPoint sp = walker.getLastPoint().convertToScreen(renderer); @@ -103,47 +105,40 @@ public void paintOverlay(ZoneRenderer renderer, Graphics2D g) { int y = (int) sp.y - 10; int x = (int) sp.x + (int) (renderer.getScaledGridSize() / 2); GraphicsUtil.drawBoxedString(g, Double.toString(walker.getDistance()), x, y); - } else { - Object oldAA = SwingUtil.useAntiAliasing(g); - g.setColor(Color.black); - ScreenPoint lastPoint = null; - for (ZonePoint zp : gridlessPath.getCellPath()) { - if (lastPoint == null) { - lastPoint = ScreenPoint.fromZonePoint(renderer, zp); - continue; - } - ScreenPoint nextPoint = ScreenPoint.fromZonePoint(renderer, zp.x, zp.y); - g.drawLine((int) lastPoint.x, (int) lastPoint.y, (int) nextPoint.x, (int) nextPoint.y); - lastPoint = nextPoint; - } - + } else if (gridlessPath != null) { // distance double c = 0; + var path2D = new Path2D.Double(); ZonePoint lastZP = null; - for (ZonePoint zp : gridlessPath.getCellPath()) { + for (ZonePoint zp : + Iterables.concat(gridlessPath.getCellPath(), List.of(currentGridlessPoint))) { + var sp = ScreenPoint.fromZonePoint(renderer, zp.x, zp.y); if (lastZP == null) { - lastZP = zp; - continue; + path2D.moveTo(sp.x, sp.y); + } else { + path2D.lineTo(sp.x, sp.y); + int a = lastZP.x - zp.x; + int b = lastZP.y - zp.y; + c += Math.sqrt(a * a + b * b); } - int a = lastZP.x - zp.x; - int b = lastZP.y - zp.y; - c += Math.sqrt(a * a + b * b); lastZP = zp; } - - // int a = lastPoint.x - (set.offsetX + token.getX()); - // int b = lastPoint.y - (set.offsetY + token.getY()); - // - // c += Math.sqrt(a*a + b*b)/zone.getUnitsPerCell(); + assert lastZP != null : "Our non-empty iterable was empty!"; c /= renderer.getZone().getGrid().getSize(); c *= renderer.getZone().getUnitsPerCell(); - String distance = NumberFormat.getInstance().format(c); - ScreenPoint sp = ScreenPoint.fromZonePoint(renderer, lastZP.x, lastZP.y); - GraphicsUtil.drawBoxedString(g, distance, (int) sp.x, (int) sp.y - 20); - - SwingUtil.restoreAntiAliasing(g, oldAA); + Object oldAA = SwingUtil.useAntiAliasing(g); + try { + g.setColor(Color.black); + g.draw(path2D); + + String distance = NumberFormat.getInstance().format(c); + ScreenPoint sp = ScreenPoint.fromZonePoint(renderer, lastZP.x, lastZP.y); + GraphicsUtil.drawBoxedString(g, distance, (int) sp.x, (int) sp.y - 20); + } finally { + SwingUtil.restoreAntiAliasing(g, oldAA); + } } } @@ -155,9 +150,6 @@ protected void installKeystrokes(Map actionMap) { KeyStroke.getKeyStroke(KeyEvent.VK_SPACE, 0, false), new AbstractAction() { public void actionPerformed(ActionEvent e) { - if (walker == null && gridlessPath == null) { - return; - } // Waypoint if (walker != null) { CellPoint cp = @@ -166,9 +158,9 @@ public void actionPerformed(ActionEvent e) { .getGrid() .convert(new ScreenPoint(mouseX, mouseY).convertToZone(renderer)); walker.toggleWaypoint(cp); - } else { - gridlessPath.addWayPoint(new ScreenPoint(mouseX, mouseY).convertToZone(renderer)); - gridlessPath.addPathCell(new ScreenPoint(mouseX, mouseY).convertToZone(renderer)); + } else if (gridlessPath != null) { + gridlessPath.addWayPoint(new ZonePoint(currentGridlessPoint)); + gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); } } }); @@ -186,11 +178,9 @@ public void mousePressed(java.awt.event.MouseEvent e) { walker = renderer.getZone().getGrid().createZoneWalker(); walker.addWaypoints(cellPoint, cellPoint); } else { - gridlessPath = new Path(); - gridlessPath.addPathCell(new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer)); - - // Add a second one that will be replaced as the mouse moves around the screen - gridlessPath.addPathCell(new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer)); + currentGridlessPoint = new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer); + gridlessPath = new Path<>(); + gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); } renderer.repaint(); return; @@ -205,6 +195,7 @@ public void mouseReleased(MouseEvent e) { if (SwingUtilities.isLeftMouseButton(e)) { walker = null; gridlessPath = null; + currentGridlessPoint = null; renderer.repaint(); return; } @@ -224,7 +215,7 @@ public void mouseDragged(MouseEvent e) { CellPoint cellPoint = renderer.getCellAt(new ScreenPoint(e.getX(), e.getY())); walker.replaceLastWaypoint(cellPoint); } else if (gridlessPath != null) { - gridlessPath.replaceLastPoint(new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer)); + currentGridlessPoint = new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer); } renderer.repaint(); } From cabe6462beb72dbe092a6bc563dc8867e524bfde Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 08:39:55 -0700 Subject: [PATCH 04/10] Make path copyable via Path.copy() Also change DTO translation to work with the field rather than the public API. --- .../java/net/rptools/maptool/model/Path.java | 42 ++++++++++++++----- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/main/java/net/rptools/maptool/model/Path.java b/src/main/java/net/rptools/maptool/model/Path.java index 8fcb47cfd6..ba1b998248 100644 --- a/src/main/java/net/rptools/maptool/model/Path.java +++ b/src/main/java/net/rptools/maptool/model/Path.java @@ -39,6 +39,22 @@ protected Object readResolve() { return this; } + public Path copy() { + var result = new Path(); + for (var cell : cellList) { + result.cellList.add(copyPoint(cell)); + } + for (var waypoint : waypointList) { + result.waypointList.add(copyPoint(waypoint)); + } + return result; + } + + @SuppressWarnings("unchecked") + private static U copyPoint(U point) { + return (U) point.clone(); + } + public void addPathCell(T point) { cellList.add(point); } @@ -273,31 +289,35 @@ else if (buildVal.y > endPoint.y) return path; } - public static Path fromDto(PathDto dto) { + public static Path fromDto(PathDto dto) { if (dto.getPointType() == PathDto.PointType.CELL_POINT) { final var path = new Path(); - dto.getCellsList().forEach(p -> path.addPathCell(new CellPoint(p.getX(), p.getY()))); - dto.getWaypointsList().forEach(p -> path.addWayPoint(new CellPoint(p.getX(), p.getY()))); + dto.getCellsList().forEach(p -> path.cellList.add(new CellPoint(p.getX(), p.getY()))); + dto.getWaypointsList().forEach(p -> path.waypointList.add(new CellPoint(p.getX(), p.getY()))); return path; } else { final var path = new Path(); - dto.getCellsList().forEach(p -> path.addPathCell(new ZonePoint(p.getX(), p.getY()))); - dto.getWaypointsList().forEach(p -> path.addWayPoint(new ZonePoint(p.getX(), p.getY()))); + dto.getCellsList().forEach(p -> path.cellList.add(new ZonePoint(p.getX(), p.getY()))); + dto.getWaypointsList().forEach(p -> path.waypointList.add(new ZonePoint(p.getX(), p.getY()))); return path; } } public PathDto toDto() { - var cellPath = getCellPath(); - if (cellPath.size() == 0) return null; + if (cellList.isEmpty()) { + return null; + } var dto = PathDto.newBuilder(); - if (cellPath.get(0) instanceof CellPoint) dto.setPointType(PathDto.PointType.CELL_POINT); - else dto.setPointType(PathDto.PointType.ZONE_POINT); + if (cellList.getFirst() instanceof CellPoint) { + dto.setPointType(PathDto.PointType.CELL_POINT); + } else { + dto.setPointType(PathDto.PointType.ZONE_POINT); + } - cellPath.forEach(p -> dto.addCells(IntPointDto.newBuilder().setX(p.x).setY(p.y))); - getWayPointList().forEach(p -> dto.addWaypoints(IntPointDto.newBuilder().setX(p.x).setY(p.y))); + cellList.forEach(p -> dto.addCells(IntPointDto.newBuilder().setX(p.x).setY(p.y))); + waypointList.forEach(p -> dto.addWaypoints(IntPointDto.newBuilder().setX(p.x).setY(p.y))); return dto.build(); } From a6a902f1c752cc8430f6bf42b178eec044d90f46 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 16:04:31 -0700 Subject: [PATCH 05/10] SelectionSet no longer modifies its path Just like `MeasureTool`, `SelectionSet` keeps its own current point that it modifies and only adds to the path when set as a waypoint. If the final path is requested, the path is copied and the current point is added as the final waypoint. A subtle aspect of this change is that all points in the `Path` are waypoints and there are no "dangling" cell points. --- .../client/ui/zone/renderer/SelectionSet.java | 25 +++++++++++-------- .../client/ui/zone/renderer/ZoneRenderer.java | 8 +++--- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java index 40f295defd..ccf7630f23 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java @@ -38,7 +38,8 @@ public class SelectionSet { private ZoneWalker walker; private final Token token; - Path gridlessPath; + private Path gridlessPath; + private ZonePoint currentGridlessPoint; /** Pixel distance (x) from keyToken's origin. */ int offsetX; @@ -73,8 +74,11 @@ public SelectionSet( walker.setWaypoints(tokenPoint, tokenPoint); } } else { - gridlessPath = new Path(); - gridlessPath.addPathCell(new ZonePoint(token.getX(), token.getY())); + gridlessPath = new Path<>(); + + currentGridlessPoint = new ZonePoint(token.getX(), token.getY()); + gridlessPath.addWayPoint(new ZonePoint(currentGridlessPoint)); + gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); } } @@ -82,7 +86,10 @@ public SelectionSet( * @return path computation. */ public @Nonnull Path getGridlessPath() { - return gridlessPath; + var result = gridlessPath.copy(); + result.addWayPoint(new ZonePoint(currentGridlessPoint)); + result.addPathCell(new ZonePoint(currentGridlessPoint)); + return result; } public ZoneWalker getWalker() { @@ -152,11 +159,8 @@ public void setOffset(int x, int y) { renderer); renderPathThreadPool.execute(renderPathTask); } else { - if (gridlessPath.getCellPath().size() > 1) { - gridlessPath.replaceLastPoint(zp); - } else { - gridlessPath.addPathCell(zp); - } + currentGridlessPoint.x = zp.x; + currentGridlessPoint.y = zp.y; } } @@ -194,7 +198,8 @@ public ZonePoint getLastWaypoint() { zp = renderer.getZone().getGrid().convert(cp); } else { - zp = gridlessPath.getLastJunctionPoint(); + // Gridless path will never be empty if set. + zp = gridlessPath.getWayPointList().getLast(); } return zp; } diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java index e4040b318c..96bd75b233 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java @@ -432,7 +432,7 @@ public void commitMoveSelectionSet(GUID keyTokenId) { } Path path = - set.getWalker() != null ? set.getWalker().getPath() : set.gridlessPath; + set.getWalker() != null ? set.getWalker().getPath() : set.getGridlessPath(); // Jamz: add final path render here? List filteredTokens = new ArrayList(); @@ -1450,7 +1450,7 @@ protected void showBlockedMoves(Graphics2D g, PlayerView view, Set if (token == keyToken && token.getLayer().supportsWalker()) { renderPath( g, - walker != null ? walker.getPath() : set.gridlessPath, + walker != null ? walker.getPath() : set.getGridlessPath(), token.getFootprint(zone.getGrid())); } @@ -1594,7 +1594,7 @@ private boolean shouldShowMovementLabels(Token token, SelectionSet set, Area cle final var grid = zone.getGrid(); tokenRectangle = token.getFootprint(grid).getBounds(grid, lastPoint); } else { - final var path = set.gridlessPath; + final var path = set.getGridlessPath(); if (path.getCellPath().isEmpty()) { return false; } @@ -1617,7 +1617,7 @@ private double calculateTraveledDistance(SelectionSet set) { double distanceTraveled = 0; ZonePoint lastPoint = null; - for (ZonePoint zp : set.gridlessPath.getCellPath()) { + for (ZonePoint zp : set.getGridlessPath().getCellPath()) { if (lastPoint == null) { lastPoint = zp; continue; From 36fe144d6c8e466301a1b913b26ac91208b87af3 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Thu, 30 May 2024 16:26:37 -0700 Subject: [PATCH 06/10] Create a new append-only API for Path Now that all users of `Path` do not modify the `Path` and are guaranteed to not have dangling cell points, we can have a new mutating API for Path: - One method for appending a waypoint (implies adding a cell point too) - Oen method for appending a partial path, the last point of which is treated as a waypoint. This gives the following invariants for `Path`: 1. If there are no waypoints, then there are no cell points. 2. The first cell point in the path is a waypoint. 3. The last cell point in the path is a waypoint. 4. The waypoints are a subsequence of the cell points. Before this change, there we no invariants for `Path` - the cell points and waypoints could vary arbitrarily depending on the caller. The downside we still have is that the representation does not reflect this invariant. E.g., there is no way accurately pull the path back apart into its partial paths. --- .../maptool/client/tool/MeasureTool.java | 5 +- .../client/ui/zone/renderer/SelectionSet.java | 9 +-- .../client/walker/AbstractZoneWalker.java | 16 ++-- .../java/net/rptools/maptool/model/Path.java | 73 +++++++++---------- 4 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java b/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java index 313abda586..ea9dfbb67d 100644 --- a/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java +++ b/src/main/java/net/rptools/maptool/client/tool/MeasureTool.java @@ -159,8 +159,7 @@ public void actionPerformed(ActionEvent e) { .convert(new ScreenPoint(mouseX, mouseY).convertToZone(renderer)); walker.toggleWaypoint(cp); } else if (gridlessPath != null) { - gridlessPath.addWayPoint(new ZonePoint(currentGridlessPoint)); - gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); + gridlessPath.appendWaypoint(currentGridlessPoint); } } }); @@ -180,7 +179,7 @@ public void mousePressed(java.awt.event.MouseEvent e) { } else { currentGridlessPoint = new ScreenPoint(e.getX(), e.getY()).convertToZone(renderer); gridlessPath = new Path<>(); - gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); + gridlessPath.appendWaypoint(currentGridlessPoint); } renderer.repaint(); return; diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java index ccf7630f23..3136684966 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/SelectionSet.java @@ -77,8 +77,7 @@ public SelectionSet( gridlessPath = new Path<>(); currentGridlessPoint = new ZonePoint(token.getX(), token.getY()); - gridlessPath.addWayPoint(new ZonePoint(currentGridlessPoint)); - gridlessPath.addPathCell(new ZonePoint(currentGridlessPoint)); + gridlessPath.appendWaypoint(currentGridlessPoint); } } @@ -87,8 +86,7 @@ public SelectionSet( */ public @Nonnull Path getGridlessPath() { var result = gridlessPath.copy(); - result.addWayPoint(new ZonePoint(currentGridlessPoint)); - result.addPathCell(new ZonePoint(currentGridlessPoint)); + result.appendWaypoint(currentGridlessPoint); return result; } @@ -173,8 +171,7 @@ public void toggleWaypoint(ZonePoint location) { if (walker != null && token.isSnapToGrid() && renderer.getZone().getGrid() != null) { walker.toggleWaypoint(renderer.getZone().getGrid().convert(location)); } else { - gridlessPath.addWayPoint(location); - gridlessPath.addPathCell(location); + gridlessPath.appendWaypoint(location); } } diff --git a/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java b/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java index af39dd09d8..d87f6dbfe9 100644 --- a/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java +++ b/src/main/java/net/rptools/maptool/client/walker/AbstractZoneWalker.java @@ -128,22 +128,17 @@ public Path getPath(RenderPathWorker renderPathWorker) { synchronized (partialPaths) { if (!partialPaths.isEmpty()) { - path.addPathCell(partialPaths.get(0).start); + path.appendWaypoint(partialPaths.getFirst().start); + for (PartialPath partial : partialPaths) { - if (partial.path.size() > 1) { - // Remove duplicated cells (end of a path = start of next path) - path.addAllPathCells(partial.path.subList(1, partial.path.size())); + // First point is already in (end of a path = start of next path) + if (!partial.path.isEmpty()) { + path.appendPartialPath(partial.path.subList(1, partial.path.size())); } } } } - for (CellPoint cp : path.getCellPath()) { - if (isWaypoint(cp)) { - path.addWayPoint(cp); - } - } - return path; } @@ -223,7 +218,6 @@ public String toString() { protected abstract List calculatePath(CellPoint start, CellPoint end); protected static class PartialPath { - final CellPoint start; final CellPoint end; final List path; diff --git a/src/main/java/net/rptools/maptool/model/Path.java b/src/main/java/net/rptools/maptool/model/Path.java index ba1b998248..e963e8fed4 100644 --- a/src/main/java/net/rptools/maptool/model/Path.java +++ b/src/main/java/net/rptools/maptool/model/Path.java @@ -23,8 +23,12 @@ import net.rptools.maptool.client.walker.NaiveWalker; import net.rptools.maptool.server.proto.PathDto; import net.rptools.maptool.server.proto.drawing.IntPointDto; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; public class Path { + private static final Logger log = LogManager.getLogger(Path.class); + private final List cellList = new LinkedList(); private final List waypointList = new LinkedList(); @@ -55,53 +59,44 @@ private static U copyPoint(U point) { return (U) point.clone(); } - public void addPathCell(T point) { - cellList.add(point); + public void appendWaypoint(T waypoint) { + cellList.add(copyPoint(waypoint)); + waypointList.add(copyPoint(waypoint)); } - public void addAllPathCells(List cells) { - cellList.addAll(cells); - } + public void appendPartialPath(List cells) { + if (cells.isEmpty()) { + return; + } - public List getCellPath() { - return Collections.unmodifiableList(cellList); - } + // If we have no waypoints yet, we must treat the first of `cells` as a waypoint. + if (waypointList.isEmpty()) { + log.warn("Attempt to add a partial path to a path, but no starting waypoint has been set."); + // Note that we always add the last point as a waypoint, so don't redundantly do so here. + if (cells.size() >= 2) { + waypointList.add(copyPoint(cells.getFirst())); + } + } - public void replaceLastPoint(T point) { - cellList.remove(cellList.size() - 1); - cellList.add(point); + for (var cell : cells) { + cellList.add(copyPoint(cell)); + } + waypointList.add(copyPoint(cells.getLast())); } - public void addWayPoint(T point) { - waypointList.add(point); + public List getCellPath() { + return Collections.unmodifiableList(cellList); } public boolean isWaypoint(T point) { return waypointList.contains(point); } - public T getLastWaypoint() { - if (waypointList.isEmpty()) return null; - return waypointList.get(waypointList.size() - 1); - } - /** - * Lee: I wonder why this convenience method was never put in. Rectifying... - * * @return way point list for path */ public List getWayPointList() { - return waypointList; - } - - /** - * Returns the last waypoint if there is one, or the last T point if there is not. - * - * @return a non-null location - */ - public T getLastJunctionPoint() { - T temp = getLastWaypoint(); - return temp != null ? temp : cellList.get(cellList.size() - 1); + return Collections.unmodifiableList(waypointList); } @SuppressWarnings("unchecked") @@ -130,8 +125,10 @@ public Path derive( Path processPath = new Path(); for (T p : cellList) { ZonePoint tempPoint = (ZonePoint) buildVal.clone(); - processPath.addPathCell(tempPoint); - if (waypointList.contains(p)) processPath.addWayPoint(tempPoint); + processPath.cellList.add(tempPoint); + if (waypointList.contains(p)) { + processPath.waypointList.add(tempPoint); + } if (buildVal.x < endPoint.x) buildVal.x += 100; else if (buildVal.x > endPoint.x) buildVal.x -= 100; @@ -172,11 +169,11 @@ public Path derive( for (T p : waypointCheck) { if (p instanceof ZonePoint) convPoint = grid.convert((ZonePoint) p); else convPoint = (CellPoint) p; - processPath.addAllPathCells(nw.calculatePath(prevPoint, convPoint)); + processPath.cellList.addAll(nw.calculatePath(prevPoint, convPoint)); prevPoint = convPoint; } } - processPath.addAllPathCells(nw.calculatePath(prevPoint, terminalPoint)); + processPath.cellList.addAll(nw.calculatePath(prevPoint, terminalPoint)); path = (Path) processPath; @@ -187,7 +184,7 @@ public Path derive( T next = (T) convPoint.clone(); next.x -= cellOffsetX; next.y -= cellOffsetY; - path.addWayPoint(next); + path.waypointList.add(next); } } else { @@ -195,14 +192,14 @@ public Path derive( T np = (T) cp.clone(); np.x -= cellOffsetX; np.y -= cellOffsetY; - path.addPathCell(np); + path.cellList.add(np); } for (T cp : waypointList) { T np = (T) cp.clone(); np.x -= cellOffsetX; np.y -= cellOffsetY; - path.addWayPoint(np); + path.waypointList.add(np); } /* From d748d387a28e152794f16236f90c47e97c83e2ab Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 31 May 2024 09:46:16 -0700 Subject: [PATCH 07/10] Refactor how we apply moves to follower tokens First of all, we copy the leader token before making any changes to the movement set. Otherwise we run the risk of modifying the leader partway and getting inconsistent results depending on iteration order. Next we pull the responsibility for derive the path out of `Token`. Previously, `Token.applyMove()` had to be aware of a bunch of information that is really specific to the renderer state, only to pass it along to `Path.derive()`. We now instead just make the renderer derive the path, then set it on the token. This also removes the need for `Token.applyMove()`. Finally, we push the calculation of the "cell offset" into `Path.derive()`. There is casework in the renderer and in `Path.derive()` that has to line up or else things don't work. So it is simpler to just have `Path.derive()` do the calculation in those cases where it is needed. --- .../client/ui/zone/renderer/ZoneRenderer.java | 36 +++++-------------- .../java/net/rptools/maptool/model/Path.java | 20 ++++++++--- .../java/net/rptools/maptool/model/Token.java | 28 --------------- 3 files changed, 25 insertions(+), 59 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java index 96bd75b233..483fb9d183 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java @@ -395,7 +395,7 @@ public void commitMoveSelectionSet(GUID keyTokenId) { removeMoveSelectionSet(keyTokenId); MapTool.serverCommand().stopTokenMove(getZone().getId(), keyTokenId); - Token keyToken = zone.getToken(keyTokenId); + Token keyToken = new Token(zone.getToken(keyTokenId), true); /* * Lee: if the lead token is snapped-to-grid and has not moved, every follower should return to where they were. Flag set at PointerTool and StampTool's stopTokenDrag() Handling the rest here. @@ -422,15 +422,6 @@ public void commitMoveSelectionSet(GUID keyTokenId) { boolean topologyTokenMoved = false; // If any token has topology we need to reset FoW - // Lee: the 1st of evils. changing it to handle proper computation - // for a key token's snapped state - AbstractPoint originPoint, tokenCell; - if (keyToken.isSnapToGrid()) { - originPoint = zone.getGrid().convert(new ZonePoint(keyToken.getX(), keyToken.getY())); - } else { - originPoint = new ZonePoint(keyToken.getX(), keyToken.getY()); - } - Path path = set.getWalker() != null ? set.getWalker().getPath() : set.getGridlessPath(); // Jamz: add final path render here? @@ -449,21 +440,6 @@ public void commitMoveSelectionSet(GUID keyTokenId) { continue; } - // Lee: get offsets based on key token's snapped state - if (token.isSnapToGrid()) { - tokenCell = zone.getGrid().convert(new ZonePoint(token.getX(), token.getY())); - } else { - tokenCell = new ZonePoint(token.getX(), token.getY()); - } - - int cellOffX, cellOffY; - if (token.isSnapToGrid() == keyToken.isSnapToGrid()) { - cellOffX = originPoint.x - tokenCell.x; - cellOffY = originPoint.y - tokenCell.y; - } else { - cellOffX = cellOffY = 0; // not used unless both are of same SnapToGrid - } - if (token.isSnapToGrid() && (!AppPreferences.getTokensSnapWhileDragging() || !keyToken.isSnapToGrid())) { // convert to Cellpoint and back to ensure token ends up at correct X and Y @@ -484,8 +460,14 @@ public void commitMoveSelectionSet(GUID keyTokenId) { * Lee: the problem now is to keep the precise coordinate computations for unsnapped tokens following a snapped key token. The derived path in the following section contains rounded * down values because the integer cell values were passed. If these were double in nature, the precision would be kept, but that would be too difficult to change at this stage... */ - - token.applyMove(set, path, offsetX, offsetY, keyToken, cellOffX, cellOffY); + var endPoint = new ZonePoint(token.getX() + offsetX, token.getY() + offsetY); + var tokenPath = + path.derive( + set, keyToken, token, token.getOriginPoint(), new ZonePoint(endPoint)); + + token.setX(endPoint.x); + token.setY(endPoint.y); + token.setLastPath(tokenPath); // Lee: setting originPoint to landing point token.setOriginPoint(new ZonePoint(token.getX(), token.getY())); diff --git a/src/main/java/net/rptools/maptool/model/Path.java b/src/main/java/net/rptools/maptool/model/Path.java index e963e8fed4..d715865a8a 100644 --- a/src/main/java/net/rptools/maptool/model/Path.java +++ b/src/main/java/net/rptools/maptool/model/Path.java @@ -104,8 +104,6 @@ public Path derive( SelectionSet set, Token keyToken, Token followerToken, - int cellOffsetX, - int cellOffsetY, ZonePoint startPoint, ZonePoint endPoint) { @@ -182,12 +180,26 @@ public Path derive( else convPoint = (CellPoint) p; T next = (T) convPoint.clone(); - next.x -= cellOffsetX; - next.y -= cellOffsetY; path.waypointList.add(next); } } else { + AbstractPoint originPoint, tokenCell; + if (keyToken.isSnapToGrid()) { + originPoint = zone.getGrid().convert(new ZonePoint(keyToken.getX(), keyToken.getY())); + } else { + originPoint = new ZonePoint(keyToken.getX(), keyToken.getY()); + } + if (followerToken.isSnapToGrid()) { + tokenCell = + zone.getGrid().convert(new ZonePoint(followerToken.getX(), followerToken.getY())); + } else { + tokenCell = new ZonePoint(followerToken.getX(), followerToken.getY()); + } + + var cellOffsetX = originPoint.x - tokenCell.x; + var cellOffsetY = originPoint.y - tokenCell.y; + for (T cp : cellList) { T np = (T) cp.clone(); np.x -= cellOffsetX; diff --git a/src/main/java/net/rptools/maptool/model/Token.java b/src/main/java/net/rptools/maptool/model/Token.java index 515b34dbb1..d5359059b3 100644 --- a/src/main/java/net/rptools/maptool/model/Token.java +++ b/src/main/java/net/rptools/maptool/model/Token.java @@ -51,7 +51,6 @@ import net.rptools.maptool.client.MapToolVariableResolver; import net.rptools.maptool.client.functions.json.JSONMacroFunctions; import net.rptools.maptool.client.swing.SwingUtil; -import net.rptools.maptool.client.ui.zone.renderer.SelectionSet; import net.rptools.maptool.client.ui.zone.renderer.ZoneRenderer; import net.rptools.maptool.language.I18N; import net.rptools.maptool.model.sheet.stats.StatSheetProperties; @@ -1280,33 +1279,6 @@ public ZonePoint getOriginPoint() { return tokenOrigin; } - /* - * Lee: changing this to apply new X and Y values (as end point) for the token BEFORE its path is - * computed. Path to be saved will be computed here instead of in ZoneRenderer - */ - public void applyMove( - SelectionSet set, - Path followerPath, - int xOffset, - int yOffset, - Token keyToken, - int cellOffX, - int cellOffY) { - setX(x + xOffset); - setY(y + yOffset); - lastPath = - followerPath != null - ? followerPath.derive( - set, - keyToken, - this, - cellOffX, - cellOffY, - getOriginPoint(), - new ZonePoint(getX(), getY())) - : null; - } - public void setLastPath(Path path) { lastPath = path; } From abc9f60318e7774ba9a7c0a38c64aed393b30e26 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 31 May 2024 10:15:15 -0700 Subject: [PATCH 08/10] Rework path derivation The calculation for how to derive a follower path is quite different now (and simpler). When both tokens are STG or both are not STG, the calculation is the same: simply apply the offset to all path points. This lets the follower's path have the exact same shape as the leader. When a non-STG token is following a STG leader, we reflect the waypoints of the leader in the follower path (offset, of course). This does not keep the full pathfinding shape when pathfinding around terrain, but at least the path is not arbitrarily chosen and does not suffer issues with zig-zagging. When a STG token is following a STG leader, we again reflect the waypoints. But in this case we have to snap each resulting point to the grid, and need to fill in partial paths between each waypoint. As before, this pathfinding is simple, i.e., it is not an A*-based algorithm. --- .../client/ui/zone/renderer/ZoneRenderer.java | 12 +- .../java/net/rptools/maptool/model/Path.java | 283 ++++++------------ .../java/net/rptools/maptool/model/Token.java | 15 - 3 files changed, 89 insertions(+), 221 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java index 483fb9d183..4af61b4cf7 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java @@ -460,18 +460,12 @@ public void commitMoveSelectionSet(GUID keyTokenId) { * Lee: the problem now is to keep the precise coordinate computations for unsnapped tokens following a snapped key token. The derived path in the following section contains rounded * down values because the integer cell values were passed. If these were double in nature, the precision would be kept, but that would be too difficult to change at this stage... */ - var endPoint = new ZonePoint(token.getX() + offsetX, token.getY() + offsetY); - var tokenPath = - path.derive( - set, keyToken, token, token.getOriginPoint(), new ZonePoint(endPoint)); + var tokenPath = path.derive(zone.getGrid(), keyToken, token); - token.setX(endPoint.x); - token.setY(endPoint.y); + token.setX(token.getX() + offsetX); + token.setY(token.getY() + offsetY); token.setLastPath(tokenPath); - // Lee: setting originPoint to landing point - token.setOriginPoint(new ZonePoint(token.getX(), token.getY())); - flush(token); MapTool.serverCommand().putToken(zone.getId(), token); diff --git a/src/main/java/net/rptools/maptool/model/Path.java b/src/main/java/net/rptools/maptool/model/Path.java index d715865a8a..75997f7c04 100644 --- a/src/main/java/net/rptools/maptool/model/Path.java +++ b/src/main/java/net/rptools/maptool/model/Path.java @@ -17,9 +17,6 @@ import java.util.Collections; import java.util.LinkedList; import java.util.List; -import net.rptools.maptool.client.MapTool; -import net.rptools.maptool.client.ui.zone.renderer.SelectionSet; -import net.rptools.maptool.client.ui.zone.renderer.ZoneRenderer; import net.rptools.maptool.client.walker.NaiveWalker; import net.rptools.maptool.server.proto.PathDto; import net.rptools.maptool.server.proto.drawing.IntPointDto; @@ -99,203 +96,97 @@ public List getWayPointList() { return Collections.unmodifiableList(waypointList); } - @SuppressWarnings("unchecked") - public Path derive( - SelectionSet set, - Token keyToken, - Token followerToken, - ZonePoint startPoint, - ZonePoint endPoint) { - - /* - * Lee: aiming to fix the following here (snapped = snapped to grid): a. fixing snapped tokens full path when following an unsnapped key token b. fixing zone point precision for unsnapped - * tokens following a snapped key token - */ - - Path path = new Path(); - // Lee: caching - ZoneRenderer zr = MapTool.getFrame().getCurrentZoneRenderer(); - Zone zone = zr.getZone(); - Grid grid = zone.getGrid(); - - if (keyToken.isSnapToGrid() && !followerToken.isSnapToGrid()) { - ZonePoint buildVal = startPoint; - Path processPath = new Path(); - for (T p : cellList) { - ZonePoint tempPoint = (ZonePoint) buildVal.clone(); - processPath.cellList.add(tempPoint); - if (waypointList.contains(p)) { - processPath.waypointList.add(tempPoint); - } - - if (buildVal.x < endPoint.x) buildVal.x += 100; - else if (buildVal.x > endPoint.x) buildVal.x -= 100; - if (buildVal.y < endPoint.y) buildVal.y += 100; - else if (buildVal.y > endPoint.y) buildVal.y -= 100; - } - - path = (Path) processPath; - - } else if (!keyToken.isSnapToGrid() && followerToken.isSnapToGrid()) { - NaiveWalker nw = new NaiveWalker(); - Path processPath = new Path(); - - CellPoint prevPoint = grid.convert(new ZonePoint(startPoint.x, startPoint.y)); - CellPoint terminalPoint = grid.convert(endPoint); - CellPoint convPoint; - - Path wpl = set.getGridlessPath(); - List waypointCheck = new LinkedList(); - List cp = wpl.getCellPath(); - ZonePoint keyStart = cp.get(0); - ZonePoint diffFromKey = new ZonePoint(keyStart.x - startPoint.x, keyStart.y - startPoint.y); - - // Lee: list is unmodifiable, working around it - int indexCheck = 0; - for (ZonePoint zp : cp) { - - if (indexCheck != 0 && indexCheck != cp.size() - 1 && !waypointCheck.contains(zp)) { - zp.x = zp.x + diffFromKey.x; - zp.y = zp.y + diffFromKey.y; - waypointCheck.add((T) zp); - } - - indexCheck++; - } - - if (!waypointCheck.isEmpty()) { - for (T p : waypointCheck) { - if (p instanceof ZonePoint) convPoint = grid.convert((ZonePoint) p); - else convPoint = (CellPoint) p; - processPath.cellList.addAll(nw.calculatePath(prevPoint, convPoint)); - prevPoint = convPoint; - } - } - processPath.cellList.addAll(nw.calculatePath(prevPoint, terminalPoint)); - - path = (Path) processPath; - - for (T p : waypointCheck) { - if (p instanceof ZonePoint) convPoint = grid.convert((ZonePoint) p); - else convPoint = (CellPoint) p; + /** Create a related path that can be applied to a follower token. */ + public Path derive(Grid grid, Token keyToken, Token followerToken) { + if (keyToken.isSnapToGrid() && followerToken.isSnapToGrid()) { + // Assume T = CellPoint. + var originCell = grid.convert(new ZonePoint(keyToken.getX(), keyToken.getY())); + var tokenCell = grid.convert(new ZonePoint(followerToken.getX(), followerToken.getY())); + return deriveSameSnapToGrid(this, originCell.x - tokenCell.x, originCell.y - tokenCell.y); + } else if (!keyToken.isSnapToGrid() && !followerToken.isSnapToGrid()) { + // Assume T = ZonePoint. + var originPoint = new ZonePoint(keyToken.getX(), keyToken.getY()); + var tokenPoint = new ZonePoint(followerToken.getX(), followerToken.getY()); + return deriveSameSnapToGrid(this, originPoint.x - tokenPoint.x, originPoint.y - tokenPoint.y); + } else if (keyToken.isSnapToGrid()) { + // Assume T = CellPoint. + return deriveFromSnapToGrid( + (Path) this, + grid, + keyToken.getX() - followerToken.getX(), + keyToken.getY() - followerToken.getY()); + } else /* (!keyToken.isSnapToGrid) */ { + // Assume T = ZonePoint. + return deriveFromNotSnapToGrid( + (Path) this, + grid, + keyToken.getX() - followerToken.getX(), + keyToken.getY() - followerToken.getY()); + } + } - T next = (T) convPoint.clone(); - path.waypointList.add(next); - } + private static Path deriveSameSnapToGrid( + Path path, int offsetX, int offsetY) { + var result = new Path(); + // Not much to do here except copy the list, offsetting the follower. + for (T point : path.cellList) { + var newPoint = copyPoint(point); + newPoint.x -= offsetX; + newPoint.y -= offsetY; + result.cellList.add(newPoint); + } - } else { - AbstractPoint originPoint, tokenCell; - if (keyToken.isSnapToGrid()) { - originPoint = zone.getGrid().convert(new ZonePoint(keyToken.getX(), keyToken.getY())); - } else { - originPoint = new ZonePoint(keyToken.getX(), keyToken.getY()); - } - if (followerToken.isSnapToGrid()) { - tokenCell = - zone.getGrid().convert(new ZonePoint(followerToken.getX(), followerToken.getY())); - } else { - tokenCell = new ZonePoint(followerToken.getX(), followerToken.getY()); - } + for (T point : path.waypointList) { + var newPoint = copyPoint(point); + newPoint.x -= offsetX; + newPoint.y -= offsetY; + result.waypointList.add(newPoint); + } + return result; + } - var cellOffsetX = originPoint.x - tokenCell.x; - var cellOffsetY = originPoint.y - tokenCell.y; + private static Path deriveFromSnapToGrid( + Path path, Grid grid, int zoneOffsetX, int zoneOffsetY) { + var result = new Path(); + // Only use the waypoint list, otherwise we get a path full of nothing but waypoints. + for (CellPoint point : path.waypointList) { + var newPoint = grid.convert(point); + newPoint.x -= zoneOffsetX; + newPoint.y -= zoneOffsetY; + result.appendWaypoint(newPoint); + } - for (T cp : cellList) { - T np = (T) cp.clone(); - np.x -= cellOffsetX; - np.y -= cellOffsetY; - path.cellList.add(np); - } + return result; + } - for (T cp : waypointList) { - T np = (T) cp.clone(); - np.x -= cellOffsetX; - np.y -= cellOffsetY; - path.waypointList.add(np); + private static Path deriveFromNotSnapToGrid( + Path path, Grid grid, int zoneOffsetX, int zoneOffsetY) { + var result = new Path(); + // The waypoints are easy: just map them to the best grid cell. But we need to fill in all the + // intervening points, so use a naive walker for that. + // The cell points of path should just be the waypoints, so just ignore them. + + CellPoint previous = null; + for (ZonePoint point : path.waypointList) { + var newPoint = new ZonePoint(point); + newPoint.x -= zoneOffsetX; + newPoint.y -= zoneOffsetY; + var current = grid.convert(newPoint); + + if (previous == null) { + result.appendWaypoint(current); + previous = current; + continue; } - /* - * Not exactly sure what Lee was trying to do here? - * I believe he was trying to return all the "cells" a non-STG token moved though? - * I'll leave the code below in case someone wants to clean it up later. - * For now, I've restored partial logic back to 1.4.0.5 above. - */ - - /* - // Lee: solo movement - if (keyToken.isSnapToGrid()) { - for (T cp : cellList) { - T np = (T) cp.clone(); - np.x -= cellOffsetX; - np.y -= cellOffsetY; - path.addPathCell(np); - } - - for (T cp : waypointList) { - T np = (T) cp.clone(); - np.x -= cellOffsetX; - np.y -= cellOffsetY; - path.addWayPoint(np); - } - } else { - Path reflectedPath = new Path(); - NaiveWalker nw = new NaiveWalker(zone); - Path wpl = set.getGridlessPath(); - - if (cellList.size() > 2) { - - CellPoint prevPoint = grid.convert(new ZonePoint(startPoint.x, startPoint.y)); - CellPoint terminalPoint = grid.convert(endPoint); - CellPoint convPoint; - - // Lee: since we already have the start point - ((List) cellList).remove(0); - - for (T p : cellList) { - convPoint = grid.convert((ZonePoint) p); - reflectedPath.addAllPathCells(nw.calculatePath(prevPoint, convPoint)); - prevPoint = convPoint; - } - - } else { - reflectedPath.addAllPathCells( - nw.calculatePath(grid.convert(startPoint), grid.convert(endPoint))); - } - - ZonePoint buildVal = startPoint; - Path processPath = new Path(); - - for (CellPoint p : reflectedPath.getCellPath()) { - ZonePoint tempPoint = (ZonePoint) buildVal.clone(); - processPath.addPathCell(tempPoint); - - if (buildVal.x < endPoint.x) - buildVal.x += 100; - else if (buildVal.x > endPoint.x) - buildVal.x -= 100; - if (buildVal.y < endPoint.y) - buildVal.y += 100; - else if (buildVal.y > endPoint.y) - buildVal.y -= 100; - } - - // processPath.addWayPoint(startPoint); - for (T cp : waypointList) { - ZonePoint np = (ZonePoint) cp; - if (np != startPoint && np != endPoint) - processPath.addWayPoint(np); - } - - processPath.addWayPoint(endPoint); - - // Lee: replacing the last point in derived path for the more - // accurate landing point - processPath.replaceLastPoint(endPoint); - path = (Path) processPath; - } - */ + var walker = new NaiveWalker(); + var walkerPath = walker.calculatePath(previous, current); + // Path will be a list: [previous, ..., current]. We already have previous, so chop that off. + result.appendPartialPath(walkerPath.subList(1, walkerPath.size())); + previous = current; } - return path; + + return result; } public static Path fromDto(PathDto dto) { @@ -313,13 +204,11 @@ public static Path fromDto(PathDto dto) { } public PathDto toDto() { - if (cellList.isEmpty()) { - return null; - } - var dto = PathDto.newBuilder(); - if (cellList.getFirst() instanceof CellPoint) { + // An empty path cannot tell what kind of points it is supposed to contain. Arbitrarily assign + // it as cell points. + if (cellList.isEmpty() || cellList.getFirst() instanceof CellPoint) { dto.setPointType(PathDto.PointType.CELL_POINT); } else { dto.setPointType(PathDto.PointType.ZONE_POINT); diff --git a/src/main/java/net/rptools/maptool/model/Token.java b/src/main/java/net/rptools/maptool/model/Token.java index d5359059b3..59d5a3baca 100644 --- a/src/main/java/net/rptools/maptool/model/Token.java +++ b/src/main/java/net/rptools/maptool/model/Token.java @@ -242,8 +242,6 @@ public enum Update { private int lastY; private Path lastPath; - // Lee: for use in added path calculations - private transient ZonePoint tokenOrigin = null; private boolean snapToScale = true; // Whether the scaleX and scaleY represent snap-to-grid // measurements @@ -1266,19 +1264,6 @@ public void setY(int y) { this.y = y; } - // Lee: added functions necessary for path computations - public void setOriginPoint(ZonePoint p) { - tokenOrigin = p; - } - - public ZonePoint getOriginPoint() { - if (tokenOrigin == null) { - tokenOrigin = new ZonePoint(getX(), getY()); - } - - return tokenOrigin; - } - public void setLastPath(Path path) { lastPath = path; } From dd21669842f1fef5dfe08247edbffa705ac25b7c Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 31 May 2024 11:15:25 -0700 Subject: [PATCH 09/10] Set follower token position based on derived path This is not visibly different from before, though it ensures that STG followers have their position snapped to the grid, just as they would be had they been the leader. --- .../client/ui/zone/renderer/ZoneRenderer.java | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java index 4af61b4cf7..99c7a02dce 100644 --- a/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java +++ b/src/main/java/net/rptools/maptool/client/ui/zone/renderer/ZoneRenderer.java @@ -429,8 +429,6 @@ public void commitMoveSelectionSet(GUID keyTokenId) { List filteredTokens = new ArrayList(); moveTimer.stop("setup"); - int offsetX, offsetY; - moveTimer.start("eachtoken"); for (GUID tokenGUID : selectionSet) { Token token = zone.getToken(tokenGUID); @@ -440,39 +438,21 @@ public void commitMoveSelectionSet(GUID keyTokenId) { continue; } - if (token.isSnapToGrid() - && (!AppPreferences.getTokensSnapWhileDragging() || !keyToken.isSnapToGrid())) { - // convert to Cellpoint and back to ensure token ends up at correct X and Y - CellPoint cellEnd = - zone.getGrid() - .convert( - new ZonePoint( - token.getX() + set.getOffsetX(), token.getY() + set.getOffsetY())); - ZonePoint pointEnd = cellEnd.convertToZonePoint(zone.getGrid()); - offsetX = pointEnd.x - token.getX(); - offsetY = pointEnd.y - token.getY(); - } else { - offsetX = set.getOffsetX(); - offsetY = set.getOffsetY(); - } - - /* - * Lee: the problem now is to keep the precise coordinate computations for unsnapped tokens following a snapped key token. The derived path in the following section contains rounded - * down values because the integer cell values were passed. If these were double in nature, the precision would be kept, but that would be too difficult to change at this stage... - */ var tokenPath = path.derive(zone.getGrid(), keyToken, token); - - token.setX(token.getX() + offsetX); - token.setY(token.getY() + offsetY); token.setLastPath(tokenPath); + var lastPoint = tokenPath.getWayPointList().getLast(); + var endPoint = + switch (lastPoint) { + case CellPoint cp -> zone.getGrid().convert(cp); + case ZonePoint zp -> zp; + }; + token.setX(endPoint.x); + token.setY(endPoint.y); + flush(token); MapTool.serverCommand().putToken(zone.getId(), token); - // No longer need this version - // Lee: redundant flush() already did this above - // replacementImageMap.remove(token); - // Only add certain tokens to the list to process in the move // Macro function(s). if (token.getLayer().supportsWalker() && token.isVisible()) { From 9be8dce70c9b222a2e504d79a13a016bdb47e839 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 31 May 2024 13:20:26 -0700 Subject: [PATCH 10/10] Add unit tests for Path Includes tests for all four derived path cases, and a test for waypoints in a self-crossing snap-to-grid path. The latter case used to include extraneous waypoints. --- .../net/rptools/maptool/model/PathTest.java | 681 ++++++++++++++++++ 1 file changed, 681 insertions(+) create mode 100644 src/test/java/net/rptools/maptool/model/PathTest.java diff --git a/src/test/java/net/rptools/maptool/model/PathTest.java b/src/test/java/net/rptools/maptool/model/PathTest.java new file mode 100644 index 0000000000..457825e33a --- /dev/null +++ b/src/test/java/net/rptools/maptool/model/PathTest.java @@ -0,0 +1,681 @@ +/* + * This software Copyright by the RPTools.net development team, and + * licensed under the Affero GPL Version 3 or, at your option, any later + * version. + * + * MapTool Source Code is distributed in the hope that it will be + * useful, but WITHOUT ANY WARRANTY; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * + * You should have received a copy of the GNU Affero General Public + * License * along with this source Code. If not, please visit + * and specifically the Affero license + * text at . + */ +package net.rptools.maptool.model; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.List; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +public class PathTest { + @Test + @DisplayName("Test that a new path is empty") + public void testNewPath() { + var path = new Path<>(); + + assertTrue(path.getCellPath().isEmpty()); + assertTrue(path.getWayPointList().isEmpty()); + } + + @Test + @DisplayName( + "Test that appending a waypoint to an empty path results in a path with one cell and one waypoint") + public void testSingletonPath() { + var path = new Path(); + var point = new ZonePoint(1, 3); + + path.appendWaypoint(point); + + assertIterableEquals(List.of(point), path.getCellPath()); + assertIterableEquals(List.of(point), path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending a partial path to an empty path sets the first and last point as waypoints") + public void testAddingPartialPathToEmptyPath() { + var path = new Path(); + var partialPath = + List.of(new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0)); + + path.appendPartialPath(partialPath); + + assertIterableEquals(partialPath, path.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(1, 3), new ZonePoint(4, 0)); + assertIterableEquals(expectedWaypoints, path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending an singleton partial path to an empty path adds the single point as the only cell and waypoint in the result") + public void testAddingSingletonPartialPathToEmptyPath() { + var path = new Path(); + var partialPath = List.of(new ZonePoint(1, 3)); + + path.appendPartialPath(partialPath); + + assertIterableEquals(partialPath, path.getCellPath()); + assertIterableEquals(partialPath, path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending a partial path to a singleton path extends the path and adds only the last point as a waypoint") + public void testAddingPartialPathToSingletonPath() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0)); + assertIterableEquals(expectedCellPoints, path.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0)); + assertIterableEquals(expectedWaypoints, path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending a partial path to a non-empty path extends the path and adds only the last point as a waypoint") + public void testAddingPartialPathToPath() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + + path.appendPartialPath( + List.of(new ZonePoint(5, -1), new ZonePoint(6, -2), new ZonePoint(7, -3))); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0), + new ZonePoint(5, -1), + new ZonePoint(6, -2), + new ZonePoint(7, -3)); + assertIterableEquals(expectedCellPoints, path.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0), new ZonePoint(7, -3)); + assertIterableEquals(expectedWaypoints, path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending an empty partial path to a non-empty path does not modify the path") + public void testAddingEmptyPartialPathToPath() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + + path.appendPartialPath(List.of()); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0)); + assertIterableEquals(expectedCellPoints, path.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0)); + assertIterableEquals(expectedWaypoints, path.getWayPointList()); + } + + @Test + @DisplayName( + "Test that appending an singleton partial path to a non-empty path adds the single point as a cell and as a waypoint") + public void testAddingSingletonPartialPathToPath() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + + path.appendPartialPath(List.of(new ZonePoint(5, -1))); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0), + new ZonePoint(5, -1)); + assertIterableEquals(expectedCellPoints, path.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0), new ZonePoint(5, -1)); + assertIterableEquals(expectedWaypoints, path.getWayPointList()); + } + + @Test + @DisplayName("Test that copying a path returns an equivalent path with unique point objects") + public void testCopyPath() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + + var copy = path.copy(); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0)); + assertIterableEquals(expectedCellPoints, copy.getCellPath()); + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0)); + assertIterableEquals(expectedWaypoints, copy.getWayPointList()); + + for (int i = 0; i < path.getCellPath().size(); ++i) { + var originalPoint = path.getCellPath().get(i); + var copyPoint = copy.getCellPath().get(i); + assertNotSame(originalPoint, copyPoint); + } + for (int i = 0; i < path.getWayPointList().size(); ++i) { + var originalPoint = path.getWayPointList().get(i); + var copyPoint = copy.getWayPointList().get(i); + assertNotSame(originalPoint, copyPoint); + } + } + + @Test + @DisplayName("Test that isWaypoint() agrees with getWaypointList()") + public void testIsWaypoint() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + path.appendPartialPath( + List.of(new ZonePoint(5, -1), new ZonePoint(6, -2), new ZonePoint(7, -3))); + + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0), new ZonePoint(7, -3)); + for (var waypoint : expectedWaypoints) { + assertTrue( + path.isWaypoint(waypoint), + () -> String.format("Point %s should be a waypoint", waypoint)); + } + var expectedNotWaypoints = + List.of( + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(5, -1), + new ZonePoint(6, -2)); + for (var notWaypoint : expectedNotWaypoints) { + assertFalse( + path.isWaypoint(notWaypoint), + () -> String.format("Point %s should not be a waypoint", notWaypoint)); + } + } + + @Test + @DisplayName("Test that an empty path can be converted to a DTO and back") + public void testEmptyDto() { + var path = new Path(); + + var dto = path.toDto(); + var newPath = Path.fromDto(dto); + + assertTrue(newPath.getCellPath().isEmpty()); + assertTrue(newPath.getWayPointList().isEmpty()); + } + + @Test + @DisplayName("Test that a path of ZonePoint can be converted to and from a DTO") + public void testDtoZonePoint() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(0, 4)); + path.appendPartialPath( + List.of( + new ZonePoint(1, 3), new ZonePoint(2, 2), new ZonePoint(3, 1), new ZonePoint(4, 0))); + path.appendPartialPath( + List.of(new ZonePoint(5, -1), new ZonePoint(6, -2), new ZonePoint(7, -3))); + + var dto = path.toDto(); + var newPath = Path.fromDto(dto); + + var expectedCellPoints = + List.of( + new ZonePoint(0, 4), + new ZonePoint(1, 3), + new ZonePoint(2, 2), + new ZonePoint(3, 1), + new ZonePoint(4, 0), + new ZonePoint(5, -1), + new ZonePoint(6, -2), + new ZonePoint(7, -3)); + assertIterableEquals(expectedCellPoints, newPath.getCellPath()); + + var expectedWaypoints = List.of(new ZonePoint(0, 4), new ZonePoint(4, 0), new ZonePoint(7, -3)); + assertIterableEquals(expectedWaypoints, newPath.getWayPointList()); + + // Make sure we didn't confuse the point type. + for (var cell : newPath.getCellPath()) { + assertInstanceOf(ZonePoint.class, cell); + } + for (var waypoint : newPath.getWayPointList()) { + assertInstanceOf(ZonePoint.class, waypoint); + } + } + + @Test + @DisplayName("Test that a path of CellPoint can be converted to and from a DTO") + public void testDtoCellPoint() { + var path = new Path(); + path.appendWaypoint(new CellPoint(0, 4)); + path.appendPartialPath( + List.of( + new CellPoint(1, 3), new CellPoint(2, 2), new CellPoint(3, 1), new CellPoint(4, 0))); + path.appendPartialPath( + List.of(new CellPoint(5, -1), new CellPoint(6, -2), new CellPoint(7, -3))); + + var dto = path.toDto(); + var newPath = Path.fromDto(dto); + + var expectedCellPoints = + List.of( + new CellPoint(0, 4), + new CellPoint(1, 3), + new CellPoint(2, 2), + new CellPoint(3, 1), + new CellPoint(4, 0), + new CellPoint(5, -1), + new CellPoint(6, -2), + new CellPoint(7, -3)); + assertIterableEquals(expectedCellPoints, newPath.getCellPath()); + + var expectedWaypoints = List.of(new CellPoint(0, 4), new CellPoint(4, 0), new CellPoint(7, -3)); + assertIterableEquals(expectedWaypoints, newPath.getWayPointList()); + + // Make sure we didn't confuse the point type. + for (var cell : newPath.getCellPath()) { + assertInstanceOf(CellPoint.class, cell); + } + for (var waypoint : newPath.getWayPointList()) { + assertInstanceOf(CellPoint.class, waypoint); + } + } + + // TODO @Nested for derived tests. + + @Nested + class DerivedPathTests { + @Test + @DisplayName( + "Test that a non-snap-to-grid follower path is properly derived from a non-snap-to-grid leader path") + public void testDeriveNonStgFollowingNonStg() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(52, 427)); + path.appendPartialPath( + List.of( + new ZonePoint(116, 364), + new ZonePoint(231, 290), + new ZonePoint(342, 172), + new ZonePoint(429, 65))); + path.appendPartialPath( + List.of(new ZonePoint(502, -91), new ZonePoint(628, -171), new ZonePoint(756, -272))); + var grid = mock(Grid.class); + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(false); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(52); + when(leaderToken.getY()).thenReturn(427); + var followerToken = mock(Token.class); + when(followerToken.isSnapToGrid()).thenReturn(false); + // Offset a bit from the leader. + when(followerToken.getX()).thenReturn(1012); + when(followerToken.getY()).thenReturn(1184); + + var derived = path.derive(grid, leaderToken, followerToken); + + // All points are used, even though in practice non-STG tokens only have waypoints. + var expectedCellPoints = + List.of( + new ZonePoint(1012, 1184), + new ZonePoint(1076, 1121), + new ZonePoint(1191, 1047), + new ZonePoint(1302, 929), + new ZonePoint(1389, 822), + new ZonePoint(1462, 666), + new ZonePoint(1588, 586), + new ZonePoint(1716, 485)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = + List.of(new ZonePoint(1012, 1184), new ZonePoint(1389, 822), new ZonePoint(1716, 485)); + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(ZonePoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(ZonePoint.class, waypoint); + } + } + + @Test + @DisplayName( + "Test that a snap-to-grid follower path is properly derived from a snap-to-grid leader path") + public void testDeriveStgFollowingStg() { + var path = new Path(); + path.appendWaypoint(new CellPoint(0, 4)); + path.appendPartialPath( + List.of( + new CellPoint(1, 3), new CellPoint(2, 2), new CellPoint(3, 1), new CellPoint(4, 0))); + path.appendPartialPath( + List.of(new CellPoint(5, -1), new CellPoint(6, -2), new CellPoint(7, -3))); + var grid = mock(Grid.class); + when(grid.convert(new ZonePoint(52, 427))).thenReturn(new CellPoint(0, 4)); + when(grid.convert(new ZonePoint(1012, 1184))).thenReturn(new CellPoint(10, 11)); + + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(true); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(52); + when(leaderToken.getY()).thenReturn(427); + var followerToken = mock(Token.class); + when(followerToken.isSnapToGrid()).thenReturn(true); + // Offset a bit from the leader. + when(followerToken.getX()).thenReturn(1012); + when(followerToken.getY()).thenReturn(1184); + + var derived = path.derive(grid, leaderToken, followerToken); + + // All points are used, even though in practice non-STG tokens only have waypoints. + var expectedCellPoints = + List.of( + new CellPoint(10, 11), + new CellPoint(11, 10), + new CellPoint(12, 9), + new CellPoint(13, 8), + new CellPoint(14, 7), + new CellPoint(15, 6), + new CellPoint(16, 5), + new CellPoint(17, 4)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = + List.of(new CellPoint(10, 11), new CellPoint(14, 7), new CellPoint(17, 4)); + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(CellPoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(CellPoint.class, waypoint); + } + } + + @Test + @DisplayName( + "Test that a non-snap-to-grid follower path is properly derived from a snap-to-grid leader path") + public void testDeriveNonStgFollowingStg() { + var path = new Path(); + path.appendWaypoint(new CellPoint(0, 4)); + path.appendPartialPath( + List.of( + new CellPoint(1, 3), new CellPoint(2, 2), new CellPoint(3, 1), new CellPoint(4, 0))); + path.appendPartialPath( + List.of(new CellPoint(5, -1), new CellPoint(6, -2), new CellPoint(7, -3))); + var grid = mock(Grid.class); + when(grid.convert(new CellPoint(0, 4))).thenReturn(new ZonePoint(0, 400)); + when(grid.convert(new CellPoint(1, 3))).thenReturn(new ZonePoint(100, 300)); + when(grid.convert(new CellPoint(2, 2))).thenReturn(new ZonePoint(200, 200)); + when(grid.convert(new CellPoint(3, 1))).thenReturn(new ZonePoint(300, 100)); + when(grid.convert(new CellPoint(4, 0))).thenReturn(new ZonePoint(400, 0)); + when(grid.convert(new CellPoint(5, -1))).thenReturn(new ZonePoint(500, -100)); + when(grid.convert(new CellPoint(6, -2))).thenReturn(new ZonePoint(600, -200)); + when(grid.convert(new CellPoint(7, -3))).thenReturn(new ZonePoint(700, -300)); + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(true); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(52); + when(leaderToken.getY()).thenReturn(427); + var followerToken = mock(Token.class); + when(followerToken.isSnapToGrid()).thenReturn(false); + // Offset a bit from the leader. + when(followerToken.getX()).thenReturn(1012); + when(followerToken.getY()).thenReturn(1184); + + var derived = path.derive(grid, leaderToken, followerToken); + + // Only the waypoints are used. + var expectedCellPoints = + List.of(new ZonePoint(960, 1157), new ZonePoint(1360, 757), new ZonePoint(1660, 457)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = expectedCellPoints; + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(ZonePoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(ZonePoint.class, waypoint); + } + } + + @Test + @DisplayName( + "Test that a snap-to-grid follower path is properly derived from a non-snap-to-grid leader path") + public void testDeriveStgFollowingNonStg() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(52, 427)); + path.appendPartialPath( + List.of( + new ZonePoint(116, 364), + new ZonePoint(231, 290), + new ZonePoint(342, 172), + new ZonePoint(429, 65))); + path.appendPartialPath( + List.of(new ZonePoint(502, -91), new ZonePoint(628, -171), new ZonePoint(756, -272))); + var grid = mock(Grid.class); + when(grid.convert(new ZonePoint(1012, 1184))).thenReturn(new CellPoint(10, 11)); + when(grid.convert(new ZonePoint(1389, 822))).thenReturn(new CellPoint(13, 8)); + when(grid.convert(new ZonePoint(1716, 485))).thenReturn(new CellPoint(17, 4)); + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(false); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(52); + when(leaderToken.getY()).thenReturn(427); + var followerToken = mock(Token.class); + when(followerToken.isSnapToGrid()).thenReturn(true); + // Offset a bit from the leader. + when(followerToken.getX()).thenReturn(1012); + when(followerToken.getY()).thenReturn(1184); + + var derived = path.derive(grid, leaderToken, followerToken); + + // Only waypoints are used directly. The rest are interpolated. + var expectedCellPoints = + List.of( + new CellPoint(10, 11), + new CellPoint(11, 10), + new CellPoint(12, 9), + new CellPoint(13, 8), + new CellPoint(14, 7), + new CellPoint(15, 6), + new CellPoint(16, 5), + new CellPoint(17, 4)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = + List.of(new CellPoint(10, 11), new CellPoint(13, 8), new CellPoint(17, 4)); + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(CellPoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(CellPoint.class, waypoint); + } + } + + @Test + @DisplayName( + "Test that a snap-to-grid follower path is properly derived from a non-snap-to-grid leader path using the naive walk") + public void testDeriveStgFollowingNonStgWithDifferentSlope() { + var path = new Path(); + path.appendWaypoint(new ZonePoint(52, 427)); + path.appendWaypoint(new ZonePoint(889, 838)); + path.appendWaypoint(new ZonePoint(445, -238)); + var grid = mock(Grid.class); + when(grid.convert(new ZonePoint(1012, 1184))).thenReturn(new CellPoint(10, 11)); + when(grid.convert(new ZonePoint(1849, 1595))).thenReturn(new CellPoint(18, 15)); + when(grid.convert(new ZonePoint(1405, 519))).thenReturn(new CellPoint(14, 5)); + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(false); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(52); + when(leaderToken.getY()).thenReturn(427); + var followerToken = mock(Token.class); + when(followerToken.isSnapToGrid()).thenReturn(true); + // Offset a bit from the leader. + when(followerToken.getX()).thenReturn(1012); + when(followerToken.getY()).thenReturn(1184); + + var derived = path.derive(grid, leaderToken, followerToken); + + // Only waypoints are used directly. The rest are interpolated. + var expectedCellPoints = + List.of( + new CellPoint(10, 11), + new CellPoint(11, 12), + new CellPoint(12, 13), + new CellPoint(13, 14), + new CellPoint(14, 15), + new CellPoint(15, 15), + new CellPoint(16, 15), + new CellPoint(17, 15), + new CellPoint(18, 15), + new CellPoint(17, 14), + new CellPoint(16, 13), + new CellPoint(15, 12), + new CellPoint(14, 11), + new CellPoint(14, 10), + new CellPoint(14, 9), + new CellPoint(14, 8), + new CellPoint(14, 7), + new CellPoint(14, 6), + new CellPoint(14, 5)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = + List.of(new CellPoint(10, 11), new CellPoint(18, 15), new CellPoint(14, 5)); + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(CellPoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(CellPoint.class, waypoint); + } + } + + @Test + @DisplayName( + "Test that a snap-to-grid leader's derived path does not include extra waypoints when the path crosses the start or endpoint") + public void testStgPathCrossingItself() { + // Previous implementations of derive() would add waypoints anywhere along a path if the + // position was a waypoint. So a path that crosses itself could be given many more waypoints + // than were actually set. We don't do that anymore, and this test ensures it. + + var path = new Path(); + path.appendWaypoint(new CellPoint(0, 0)); + path.appendPartialPath( + List.of( + new CellPoint(1, 0), new CellPoint(2, 0), new CellPoint(3, 0), new CellPoint(4, 0))); + path.appendPartialPath( + List.of( + new CellPoint(3, 1), new CellPoint(2, 2), new CellPoint(1, 3), new CellPoint(0, 4))); + path.appendPartialPath( + List.of( + new CellPoint(0, 3), + new CellPoint(0, 2), + new CellPoint(0, 1), + // Note: this point is also the first point in the path. + new CellPoint(0, 0), + new CellPoint(0, -1), + new CellPoint(0, -2))); + path.appendPartialPath( + List.of( + new CellPoint(1, -1), + // Note: this last point crosses through the first partial path. + new CellPoint(2, 0))); + var grid = mock(Grid.class); + when(grid.convert(new ZonePoint(50, 50))).thenReturn(new CellPoint(0, 0)); + var leaderToken = mock(Token.class); + when(leaderToken.isSnapToGrid()).thenReturn(true); + // Position agrees with start of path. + when(leaderToken.getX()).thenReturn(50); + when(leaderToken.getY()).thenReturn(50); + + var derived = path.derive(grid, leaderToken, leaderToken); + + // All points are used, even though in practice non-STG tokens only have waypoints. + var expectedCellPoints = + List.of( + new CellPoint(0, 0), + new CellPoint(1, 0), + new CellPoint(2, 0), + new CellPoint(3, 0), + new CellPoint(4, 0), + new CellPoint(3, 1), + new CellPoint(2, 2), + new CellPoint(1, 3), + new CellPoint(0, 4), + new CellPoint(0, 3), + new CellPoint(0, 2), + new CellPoint(0, 1), + new CellPoint(0, 0), + new CellPoint(0, -1), + new CellPoint(0, -2), + new CellPoint(1, -1), + new CellPoint(2, 0)); + assertIterableEquals(expectedCellPoints, derived.getCellPath()); + var expectedWaypoints = + List.of( + // Important that these are exactly the waypoints set, in the orer they were set. + new CellPoint(0, 0), + // Note: No (2, 0) waypoint here. + new CellPoint(4, 0), + new CellPoint(0, 4), + // Note: New (0, 0) waypoint here. + new CellPoint(0, -2), + new CellPoint(2, 0)); + assertIterableEquals(expectedWaypoints, derived.getWayPointList()); + + for (var cell : derived.getCellPath()) { + assertInstanceOf(CellPoint.class, cell); + } + for (var waypoint : derived.getWayPointList()) { + assertInstanceOf(CellPoint.class, waypoint); + } + } + } +}