From fe3f20a6a3c1f760188bff483b19394d89346027 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 6 Dec 2024 11:44:56 -0800 Subject: [PATCH 1/4] Avoid implicit collections in WallTopologyConverter They are nulled out when reading XML with no elements for them, rather than being empty collections. Using plain `List<>` with aliases gives the same XML but will produce an empty list when reading it back. This also avoids the need for the `WallsRepresentation` and `VerticesRespresentation` wrapper types. --- .../converters/WallTopologyConverter.java | 37 +++++-------------- 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java b/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java index 2398b3405e..600a0ec677 100644 --- a/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java +++ b/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java @@ -15,14 +15,13 @@ package net.rptools.maptool.model.converters; import com.thoughtworks.xstream.XStream; +import com.thoughtworks.xstream.annotations.XStreamAliasType; import com.thoughtworks.xstream.annotations.XStreamAsAttribute; -import com.thoughtworks.xstream.annotations.XStreamImplicit; import com.thoughtworks.xstream.converters.MarshallingContext; import com.thoughtworks.xstream.converters.UnmarshallingContext; import com.thoughtworks.xstream.converters.collections.AbstractCollectionConverter; import com.thoughtworks.xstream.io.HierarchicalStreamReader; import com.thoughtworks.xstream.io.HierarchicalStreamWriter; -import java.awt.geom.Point2D; import java.util.ArrayList; import java.util.Comparator; import java.util.List; @@ -81,12 +80,12 @@ public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingC vertex -> new VertexRepresentation( vertex.id(), vertex.getPosition().getX(), vertex.getPosition().getY())) - .forEach(representation.vertices.list::add); + .forEach(representation.vertices::add); walls .getWalls() .sorted(Comparator.comparingInt(WallTopology.Wall::getZIndex)) .map(wall -> new WallRepresentation(wall.from().id(), wall.to().id())) - .forEach(representation.walls.list::add); + .forEach(representation.walls::add); context.convertAnother(representation); } @@ -96,7 +95,7 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co var walls = new WallTopology(); var representation = (GraphRepresentation) context.convertAnother(walls, GraphRepresentation.class); - for (var vertexRepresentation : representation.vertices.list) { + for (var vertexRepresentation : representation.vertices) { try { var vertex = walls.createVertex(new GUID(vertexRepresentation.id)); vertex.setPosition(vertexRepresentation.x, vertexRepresentation.y); @@ -107,7 +106,7 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co e); } } - for (var wallRepresentation : representation.walls.list) { + for (var wallRepresentation : representation.walls) { try { walls.createWall(new GUID(wallRepresentation.from), new GUID(wallRepresentation.to)); } catch (WallTopology.GraphException e) { @@ -124,20 +123,11 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co // region Intermediate representation that contains only the essence of the wall graph. private static final class GraphRepresentation { - public final VerticesRepresentation vertices = new VerticesRepresentation(); - public final WallsRepresentation walls = new WallsRepresentation(); - } - - private static final class VerticesRepresentation { - @XStreamImplicit(itemFieldName = "vertex") - public final List list = new ArrayList<>(); - } - - private static final class WallsRepresentation { - @XStreamImplicit(itemFieldName = "wall") - public final List list = new ArrayList<>(); + public final List vertices = new ArrayList<>(); + public final List walls = new ArrayList<>(); } + @XStreamAliasType("vertex") private static final class VertexRepresentation { @XStreamAsAttribute public final String id; @XStreamAsAttribute public final double x; @@ -150,6 +140,7 @@ public VertexRepresentation(GUID id, double x, double y) { } } + @XStreamAliasType("wall") private static final class WallRepresentation { @XStreamAsAttribute public final String from; @XStreamAsAttribute public final String to; @@ -160,15 +151,5 @@ public WallRepresentation(GUID from, GUID to) { } } - private static final class PointRepresentation { - @XStreamAsAttribute public final double x; - @XStreamAsAttribute public final double y; - - public PointRepresentation(Point2D position) { - this.x = position.getX(); - this.y = position.getY(); - } - } - // endregion } From 72160b1afac5114e23931d3c74e9426c918542af Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 6 Dec 2024 11:45:24 -0800 Subject: [PATCH 2/4] Have the converter remove dangling vertices after building the graph --- .../maptool/model/converters/WallTopologyConverter.java | 3 +++ .../net/rptools/maptool/model/topology/WallTopology.java | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java b/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java index 600a0ec677..cf846bf8e8 100644 --- a/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java +++ b/src/main/java/net/rptools/maptool/model/converters/WallTopologyConverter.java @@ -117,6 +117,9 @@ public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext co e); } } + + walls.removeDanglingVertices(); + return walls; } diff --git a/src/main/java/net/rptools/maptool/model/topology/WallTopology.java b/src/main/java/net/rptools/maptool/model/topology/WallTopology.java index 7fcb99dd45..0899de9466 100644 --- a/src/main/java/net/rptools/maptool/model/topology/WallTopology.java +++ b/src/main/java/net/rptools/maptool/model/topology/WallTopology.java @@ -568,7 +568,14 @@ public Vertex merge(Vertex first, Vertex second) { // endregion - private void removeDanglingVertices() { + /** + * Ensures that any vertices with no walls are removed from the graph. + * + *

This is usually called automatically when needed, so don't call this method without reason. + * Deserializing is an exception where improper XML could result in vertices being added without + * associated walls. + */ + public void removeDanglingVertices() { var verticesToRemove = new ArrayList(); for (var vertex : graph.vertexSet()) { if (graph.edgesOf(vertex).isEmpty()) { From 7b188f0784afc6d105a229877720ef1991b7a7d6 Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 6 Dec 2024 11:45:46 -0800 Subject: [PATCH 3/4] Add tests cases for WalltopologyConverter Ensures WallTopology survives a write/read cycle, and that specific XML can be read back. --- .../converters/WallTopologyConverterTest.java | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 src/test/java/net/rptools/maptool/model/converters/WallTopologyConverterTest.java diff --git a/src/test/java/net/rptools/maptool/model/converters/WallTopologyConverterTest.java b/src/test/java/net/rptools/maptool/model/converters/WallTopologyConverterTest.java new file mode 100644 index 0000000000..7d211040fe --- /dev/null +++ b/src/test/java/net/rptools/maptool/model/converters/WallTopologyConverterTest.java @@ -0,0 +1,137 @@ +/* + * 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.converters; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Named.named; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import com.thoughtworks.xstream.XStream; +import java.awt.geom.Point2D; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import net.rptools.maptool.model.topology.WallTopology; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Tests that {@link WallTopologyConverter} can be used correctly with {@link XStream} for + * serializing and deserializing. + */ +public class WallTopologyConverterTest { + private XStream xStream; + + @BeforeEach + void setUp() { + xStream = new XStream(); + XStream.setupDefaultSecurity(xStream); + xStream.allowTypesByWildcard(new String[] {"net.rptools.**", "java.awt.**", "sun.awt.**"}); + xStream.registerConverter(new WallTopologyConverter(xStream)); + } + + @ParameterizedTest + @MethodSource + void testSerializeDeserialize(WallTopology topology) { + var xml = xStream.toXML(topology); + var result = xStream.fromXML(xml); + assertInstanceOf(WallTopology.class, result); + var newTopology = (WallTopology) result; + + var originalVertices = topology.getVertices().collect(Collectors.toSet()); + var originalWalls = topology.getWalls().collect(Collectors.toSet()); + var newVertices = newTopology.getVertices().collect(Collectors.toSet()); + var newWalls = newTopology.getWalls().collect(Collectors.toSet()); + + assertEquals(originalVertices, newVertices); + assertEquals(originalWalls, newWalls); + } + + static List testSerializeDeserialize() { + var result = new ArrayList(); + + { + var topology = new WallTopology(); + result.add(arguments(named("empty walls", topology))); + } + { + var topology = new WallTopology(); + topology.brandNewWall(); + result.add(arguments(named("single default wall", topology))); + } + { + var topology = new WallTopology(); + topology.string( + new Point2D.Double(0, 0), + builder -> { + builder.push(new Point2D.Double(100, 0)); + builder.push(new Point2D.Double(100, 100)); + builder.push(new Point2D.Double(50, 0)); + }); + result.add(arguments(named("wall string", topology))); + } + + return result; + } + + @Test + void testDeserializeEmptyVerticesAndWalls() { + var xml = + """ + + + + + """; + + var result = xStream.fromXML(xml); + assertInstanceOf(WallTopology.class, result); + var newTopology = (WallTopology) result; + + assertEquals(Set.of(), newTopology.getVertices().collect(Collectors.toSet())); + assertEquals(Set.of(), newTopology.getWalls().collect(Collectors.toSet())); + } + + @Test + void testDeserializeEmptyWalls() { + // This is not something that is supposed to appear in the XML, but we might as well handle it. + // There are no walls, meaning the vertices are all dangling and shouldn't be there either. + // This also tests that the implicit collection for walls is safe to deserialize when empty. + + var xml = + """ + + + + + + + + """; + + var result = xStream.fromXML(xml); + assertInstanceOf(WallTopology.class, result); + var newTopology = (WallTopology) result; + + // The vertices in the XML are dangling, so are automatically removed. + assertEquals(Set.of(), newTopology.getVertices().collect(Collectors.toSet())); + assertEquals(Set.of(), newTopology.getWalls().collect(Collectors.toSet())); + } +} From a6921c523a22eef8b08f13481bb068e5636334ed Mon Sep 17 00:00:00 2001 From: Kenneth VanderLinde Date: Fri, 6 Dec 2024 12:15:38 -0800 Subject: [PATCH 4/4] Add back tooltip and instructions for wall topology tool --- src/main/resources/net/rptools/maptool/language/i18n.properties | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/resources/net/rptools/maptool/language/i18n.properties b/src/main/resources/net/rptools/maptool/language/i18n.properties index c4f57a1587..5826278b48 100644 --- a/src/main/resources/net/rptools/maptool/language/i18n.properties +++ b/src/main/resources/net/rptools/maptool/language/i18n.properties @@ -2771,6 +2771,8 @@ tool.isorectangletopology.instructions = LClick: set initial/final point, Shif tool.stamp.tooltip = Stamp tool tool.walltemplate.instructions = LClick: set starting cell, move mouse in direction of wall, second LClick to finish wall; Ctrl: move origin point tool.walltemplate.tooltip = Draw a wall template. +tool.walltopology.tooltip = Draw movement and vision blocking walls. +tool.walltopology.instructions = LClick: start new wall or grab wall / vertex, RClick: extend wall, Alt+LClick: extend or split wall / vertex, Alt+Drag: join with wall / vertex, Ctrl: snap-to-grid, Shift: delete wall / vertex tools.ai_selector.tooltip = Tokens will navigate around MBL, VBL (if selected), and account for tokens with terrain modifiers. tools.ignore_vbl_on_move.tooltip = Tokens will navigate around VBL if selected, otherwise they will freely move into VBL. tools.topology_mode_selection.cover_vbl.tooltip = Draw Cover Vision Blocking (Cover VB).