From 9fcc0bba531cd91842769f293b155c99cc5c5937 Mon Sep 17 00:00:00 2001 From: Marko Luukkainen Date: Tue, 13 Aug 2019 16:52:55 +0300 Subject: [PATCH] Clear removed objects from mapping cache * Using undo/redo caused mapping cache to return already removed objects, which caused bookkeeping issues. * PipeControlPoints structure is now more resilient to random order of graph side changes. gitlab #24 Change-Id: Ia3e00f116bb86be3ef4472a646058842d1c28c27 --- .../g3d/vtk/common/AbstractVTKNodeMap.java | 167 +++++++++++++++--- .../plant3d/scenegraph/PipelineComponent.java | 117 ++++++++++-- .../controlpoint/PipeControlPoint.java | 44 +++-- .../scenegraph/controlpoint/PipingRules.java | 4 +- 4 files changed, 271 insertions(+), 61 deletions(-) diff --git a/org.simantics.g3d.vtk/src/org/simantics/g3d/vtk/common/AbstractVTKNodeMap.java b/org.simantics.g3d.vtk/src/org/simantics/g3d/vtk/common/AbstractVTKNodeMap.java index dab2040e..6194ec82 100644 --- a/org.simantics.g3d.vtk/src/org/simantics/g3d/vtk/common/AbstractVTKNodeMap.java +++ b/org.simantics.g3d.vtk/src/org/simantics/g3d/vtk/common/AbstractVTKNodeMap.java @@ -13,6 +13,7 @@ package org.simantics.g3d.vtk.common; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -27,11 +28,13 @@ import org.simantics.db.common.request.ReadRequest; import org.simantics.db.common.request.WriteRequest; import org.simantics.db.exception.DatabaseException; import org.simantics.db.layer0.util.Layer0Utils; +import org.simantics.db.service.UndoRedoSupport; import org.simantics.g3d.ontology.G3D; import org.simantics.g3d.scenegraph.RenderListener; import org.simantics.g3d.scenegraph.base.INode; import org.simantics.g3d.scenegraph.base.NodeListener; import org.simantics.g3d.scenegraph.base.ParentNode; +import org.simantics.objmap.exceptions.MappingException; import org.simantics.objmap.graph.IMapping; import org.simantics.objmap.graph.IMappingListener; import org.simantics.utils.datastructures.Callback; @@ -42,7 +45,7 @@ import org.simantics.utils.ui.ExceptionUtils; import vtk.vtkProp; -public abstract class AbstractVTKNodeMap implements VTKNodeMap, IMappingListener, RenderListener, NodeListener{ +public abstract class AbstractVTKNodeMap implements VTKNodeMap, IMappingListener, RenderListener, NodeListener, UndoRedoSupport.ChangeListener{ private static final boolean DEBUG = false; @@ -55,6 +58,9 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< protected ParentNode rootNode; + protected UndoRedoSupport undoRedoSupport; + protected int undoOpCount = 0; + protected boolean runUndo = false; public AbstractVTKNodeMap(Session session, IMapping mapping, VtkView view, ParentNode rootNode) { this.session = session; this.mapping = mapping; @@ -63,9 +69,18 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< view.addListener(this); mapping.addMappingListener(this); rootNode.addListener(this); + + undoRedoSupport = session.getService(UndoRedoSupport.class); + undoRedoSupport.subscribe(this); + try { + undoOpCount = undoRedoSupport.getUndoContext(session).getAll().size(); + } catch(DatabaseException e) { + e.printStackTrace(); + } } + protected abstract void addActor(E node); protected abstract void removeActor(E node); protected abstract void updateActor(E node,Set ids); @@ -122,6 +137,25 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< private boolean rangeModified = false; + @Override + public void onChanged() { + try { + int count = undoRedoSupport.getUndoContext(session).getAll().size(); + if (count < undoOpCount) { + runUndo = true; + } else { + runUndo = false; + } + undoOpCount = count; + if (DEBUG) System.out.println("Undo " + runUndo); + } catch (DatabaseException e) { + // TODO Auto-generated catch block + e.printStackTrace(); + } + + + } + @SuppressWarnings("unchecked") @Override public void updateRenderObjectsFor(INode node) { @@ -153,7 +187,7 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< @SuppressWarnings("unchecked") private void receiveAdd(E node, String id, boolean db) { - if (DEBUG) System.out.println("receiveAdd " + node + " " + id + " " + db); + if (DEBUG) System.out.println("receiveAdd " + debugString(node) + " " + id + " " + db); synchronized (syncMutex) { for (Pair n : added) { if (n.first.equals(node)) @@ -170,7 +204,7 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< @SuppressWarnings("unchecked") private void receiveRemove(E node, String id, boolean db) { - if (DEBUG) System.out.println("receiveRemove " + node + " " + id + " " + db); + if (DEBUG) System.out.println("receiveRemove " + debugString(node) + " " + id + " " + db); synchronized (syncMutex) { for (Pair n : removed) { if (n.first.equals(node)) @@ -186,7 +220,7 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< @SuppressWarnings("unchecked") private void receiveUpdate(E node, String id, boolean db) { - if (DEBUG) System.out.println("receiveUpdate " + node + " " + id + " " + db); + if (DEBUG) System.out.println("receiveUpdate " + debugString(node) + " " + id + " " + db); synchronized (syncMutex) { // for (Pair n : updated) { // if (n.first.equals(node)) @@ -218,12 +252,13 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< @Override public void perform(WriteGraph graph) throws DatabaseException { - commit(graph); + if (DEBUG) System.out.println("Commit " + commitMessage); if (commitMessage != null) { Layer0Utils.addCommentMetadata(graph, commitMessage); + graph.markUndoPoint(); commitMessage = null; } - graph.markUndoPoint(); + commit(graph); } }, new Callback() { @@ -242,9 +277,13 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< graphUpdates = true; mapping.updateDomain(graph); graphUpdates = false; + clearDeletes(); + if (DEBUG) System.out.println("Commit done"); } } + + @Override public void domainModified() { if (graphUpdates) @@ -261,21 +300,45 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< } + protected void reset(ReadGraph graph) throws MappingException { + if (DEBUG) System.out.println("Reset"); + synchronized (syncMutex) { + graphUpdates = true; + mapping.getRangeModified().clear(); + for (Object o : mapping.getDomain()) + mapping.domainModified(o); + mapping.updateRange(graph); + graphModified.clear(); + graphUpdates = false; + } + } + + private boolean useFullSyncWithUndo = false; + protected void update(ReadGraph graph) throws DatabaseException { - synchronized (syncMutex) { - graphUpdates = true; - for (Object domainObject : mapping.getDomainModified()) { - E rangeObject = mapping.get(domainObject); - if (rangeObject != null) - graphModified.add(rangeObject); - } - mapping.updateRange(graph); - graphModified.clear(); - graphUpdates = false; - } + if (DEBUG) System.out.println("Graph update start"); + if (runUndo && useFullSyncWithUndo) { + reset(graph); + } else { + synchronized (syncMutex) { + graphUpdates = true; + for (Object domainObject : mapping.getDomainModified()) { + E rangeObject = mapping.get(domainObject); + if (rangeObject != null) + graphModified.add(rangeObject); + } + mapping.updateRange(graph); + graphModified.clear(); + syncDeletes(); + clearDeletes(); + graphUpdates = false; + } + } - if (mapping.isRangeModified()) - commit("Graph sync"); + //if (mapping.isRangeModified() && !runUndo) // FIXME : redo? + if (mapping.isRangeModified()) + commit((String)null); + if (DEBUG) System.out.println("Graph update done"); } @Override @@ -295,18 +358,62 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< } } - List> rem = new ArrayList>(); - List> add = new ArrayList>(); - MapSet mod = new MapSet.Hash(); - Set propagation = new HashSet(); - Stack stack = new Stack(); - + // Reusable containers for data synchronisation + List> rem = new ArrayList>(); // Removed objects + List> add = new ArrayList>(); // Added objects + MapSet mod = new MapSet.Hash(); // Modified objects + Set propagation = new HashSet(); // Objects with propagated changes + Stack stack = new Stack(); // Stack for handling propagation + Set delete = Collections.synchronizedSet(new HashSet()); // Objects to be completely deleted + Set deleteUC = new HashSet(); @Override public synchronized void preRender() { updateCycle(); } + + /** + * When objects are removed (either from Java or Graph), after remove processing the Java objects remain in mapping cache. + * This causes problems with Undo and Redo, whcih the end up re-using the removed objects from mapping cache. + * + * This code here synchronizes removed and added objects to collect deletable objects. (a deletable object is one which is removed but not added). + * + */ + protected void syncDeletes() { + deleteUC.clear(); + for (Pair n : removed) { + deleteUC.add(n.first); + } + for (Pair n : added) { + deleteUC.remove(n.first); + } + if (DEBUG && deleteUC.size() > 0) { + System.out.println("Delete sync"); + for (E n : delete) { + System.out.println(debugString(n)); + } + } + delete.addAll(deleteUC); + deleteUC.clear(); + } + + /** + * Clears deletable objects from mapping cache. + */ + protected void clearDeletes() { + if (DEBUG && delete.size() > 0) System.out.println("Delete"); + for (E n : delete) { + if (DEBUG) System.out.println(debugString(n)); + mapping.getRange().remove(n); + } + delete.clear(); + } + + protected String debugString(E n) { + return n + "@" + Integer.toHexString(n.hashCode()); + } + @SuppressWarnings("unchecked") protected void updateCycle() { rem.clear(); @@ -314,6 +421,7 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< mod.clear(); propagation.clear(); + synchronized (syncMutex) { rem.addAll(removed); add.addAll(added); @@ -322,7 +430,7 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< mod.add(e, s); } } - + syncDeletes(); removed.clear(); added.clear(); updated.clear(); @@ -331,7 +439,6 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< for (Pair n : rem) { stopListening(n.first); removeActor(n.first); - } for (Pair n : add) { @@ -408,8 +515,9 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< for (String s : mod.getValues(e)) l.propertyChanged(e, s); } + synchronized (syncMutex) { - if (added.isEmpty() && removed.isEmpty() && updated.getKeys().size() == 0) + if (added.isEmpty() && removed.isEmpty() && updated.getKeys().size() == 0) rangeModified = false; } } @@ -467,6 +575,9 @@ public abstract class AbstractVTKNodeMap implements VTKNodeMap< @Override public void delete() { + if (undoRedoSupport != null) + undoRedoSupport.cancel(this); + changeTracking = false; view.removeListener(this); mapping.removeMappingListener(this); diff --git a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/PipelineComponent.java b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/PipelineComponent.java index 26a32397..36c6dde1 100644 --- a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/PipelineComponent.java +++ b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/PipelineComponent.java @@ -88,7 +88,10 @@ public abstract class PipelineComponent extends GeometryNode { public void setNext(PipelineComponent comp) { if (next == comp) return; + if (comp == null) + this.next._removeRef(this); this.next = comp; + this.syncnext = false; syncNext(); firePropertyChanged(Plant3D.URIs.HasNext); if (comp != null) @@ -106,8 +109,11 @@ public abstract class PipelineComponent extends GeometryNode { public void setPrevious(PipelineComponent comp) { if (previous == comp) return; + if (comp == null) + this.previous._removeRef(this); this.previous = comp; - + this.syncprev = false; + syncPrevious(); firePropertyChanged(Plant3D.URIs.HasPrevious); if (comp != null) comp.sync(); @@ -124,7 +130,10 @@ public abstract class PipelineComponent extends GeometryNode { public void setBranch0(PipelineComponent comp) { if (branch0 == comp) return; + if (comp == null) + this.branch0._removeRef(this); this.branch0 = comp; + this.syncbr0 = false; syncBranch0(); firePropertyChanged(Plant3D.URIs.HasBranch0); if (comp != null) @@ -132,8 +141,29 @@ public abstract class PipelineComponent extends GeometryNode { // System.out.println(this + " next " + comp); } + @GetPropertyValue(name="Previous",tabId="Debug",value=Plant3D.URIs.HasPrevious) + public String getPreviousDebug() { + if (previous == null) + return null; + return previous.getName(); + } + + @GetPropertyValue(name="Next",tabId="Debug",value=Plant3D.URIs.HasNext) + public String getNextDebug() { + if (next == null) + return null; + return next.getName(); + } + + @GetPropertyValue(name="Branch0",tabId="Debug",value=Plant3D.URIs.HasBranch0) + public String getBR0Debug() { + if (branch0 == null) + return null; + return branch0.getName(); + } + private PipeControlPoint getBranchPoint() { PipeControlPoint branchPoint; if (getControlPoint().getSubPoint().size() > 0) { @@ -182,28 +212,56 @@ public abstract class PipelineComponent extends GeometryNode { return true; } + // When link to a component is removed, also link to the other direction must be removed at the same time, or + // Control point structure is left into illegal state. + private void _removeRef(PipelineComponent comp) { + if (next == comp) { + next = null; + syncnext = false; + syncNext(); + } else if (previous == comp) { + previous = null; + syncprev = false; + syncPrevious(); + } else if (branch0 == comp) { + branch0 = null; + syncbr0 = false; + syncBranch0(); + } + } + + boolean syncnext = false; + private void syncNext() { + if (syncnext) + return; + syncnext = _syncNext(); + } - private boolean syncNext() { - - if (getControlPoint() != null) { + private boolean _syncNext() { + PipeControlPoint pcp = getControlPoint(); + if (pcp != null) { + if (next != null ) { - if (next.getControlPoint() != null && next.getPipeRun() != null) { + if (next.getControlPoint() != null) { // TODO, relying that the other direction is connected. boolean nxt = next.getPrevious() == this; boolean br0 = next.getBranch0() == this; if (nxt){ - return _connectNext(getControlPoint(), next.getControlPoint()); + return _connectNext(pcp, next.getControlPoint()); } else if (br0) { - return _connectNext(getControlPoint(), next.getBranchPoint()); + return _connectNext(pcp, next.getBranchPoint()); + } else { + return false; } } else { return false; } - } else if (getControlPoint().getPrevious() != null) { - getControlPoint().setNext(null); + } else if (pcp.getNext() != null) { + pcp.setNext(null); + return true; } } else { return false; @@ -211,26 +269,36 @@ public abstract class PipelineComponent extends GeometryNode { return true; } - private boolean syncPrevious() { - - if (getControlPoint() != null) { + boolean syncprev = false; + private void syncPrevious() { + if (syncprev) + return; + syncprev = _syncPrevious(); + } + + private boolean _syncPrevious() { + PipeControlPoint pcp = getControlPoint(); + if (pcp != null) { if (previous != null ) { - if (previous.getControlPoint() != null && previous.getPipeRun() != null) { + if (previous.getControlPoint() != null) { // TODO, relying that the other direction is connected. boolean prev = previous.getNext() == this; boolean br0 = previous.getBranch0() == this; if (prev){ - return _connectPrev(getControlPoint(), previous.getControlPoint()); + return _connectPrev(pcp, previous.getControlPoint()); } else if (br0) { - return _connectPrev(getControlPoint(), previous.getBranchPoint()); + return _connectPrev(pcp, previous.getBranchPoint()); + } else { + return false; } } else { return false; } - } else if (getControlPoint().getPrevious() != null) { - getControlPoint().setPrevious(null); + } else if (pcp.getPrevious() != null) { + pcp.setPrevious(null); + return true; } } else { return false; @@ -238,14 +306,21 @@ public abstract class PipelineComponent extends GeometryNode { return true; } - private boolean syncBranch0() { + boolean syncbr0 = false; + private void syncBranch0() { + if (syncbr0) + return; + syncbr0 = _syncBranch0(); + } + + private boolean _syncBranch0() { if (getControlPoint() != null) { if (getControlPoint().isDualInline()) { branch0 = null; return false; } if (branch0 != null) { - if (branch0.getControlPoint() != null && branch0.getPipeRun() != null) { + if (branch0.getControlPoint() != null) { PipeControlPoint branchPoint = getBranchPoint(); PipeControlPoint pcp = branch0.getControlPoint(); // TODO, relying that the other direction is connected. @@ -255,7 +330,10 @@ public abstract class PipelineComponent extends GeometryNode { _connectNext(branchPoint, pcp); } else if (prev){ _connectPrev(branchPoint, pcp); + } else { + return false; } + } else { return false; } @@ -263,6 +341,7 @@ public abstract class PipelineComponent extends GeometryNode { } else if (getControlPoint().getSubPoint().size() > 0) { // TODO : this may cause problems? (Removes branch point, before branch has been set?) getControlPoint().getSubPoint().get(0).remove(); getControlPoint().children.clear(); + return true; } } else { return false; diff --git a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipeControlPoint.java b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipeControlPoint.java index 22c58ddb..fc78f1b9 100644 --- a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipeControlPoint.java +++ b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipeControlPoint.java @@ -22,6 +22,8 @@ import vtk.vtkRenderer; public class PipeControlPoint extends G3DNode implements IP3DNode { + + private static boolean DEBUG = false; public enum Type{INLINE,TURN,END}; public enum Direction{NEXT,PREVIOUS}; @@ -186,8 +188,9 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { public void setNext(PipeControlPoint next) { if (isEnd() && previous != null && next != null) throw new RuntimeException("End control points are allowed to have only one connection"); -// if (next != null && getPipeRun() == null) -// throw new RuntimeException("Cannot connect control point befor piperun has been set"); + if (this.next == next) + return; + if (DEBUG) System.out.println(this + " next " + next); this.next = next; if (component != null) { if (parent == null || sub) @@ -202,8 +205,9 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { public void setPrevious(PipeControlPoint previous) { if (isEnd() && next != null && previous != null) throw new RuntimeException("End control points are allowed to have only one connection"); -// if (previous != null && getPipeRun() == null) -// throw new RuntimeException("Cannot connect control point befor piperun has been set"); + if (this.previous == previous) + return; + if (DEBUG) System.out.println(this + " previous " + previous); this.previous = previous; if (component != null) { if (parent == null || sub) @@ -225,14 +229,8 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { public PipeControlPoint getParentPoint() { return parent; } - - - - - - private double length; private Double turnAngle; private Vector3d turnAxis; @@ -300,7 +298,9 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { } public void setTurnAxis(Vector3d turnAxis) { - this.turnAxis = turnAxis; + if (this.turnAxis != null && MathTools.equals(turnAxis, this.turnAxis)) + return; + this.turnAxis = turnAxis; firePropertyChanged("turnAxis"); } @@ -483,6 +483,8 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { dir.sub(pcp.getWorldPosition(),previous.getWorldPosition()); if (dir.lengthSquared() > MathTools.NEAR_ZERO) dir.normalize(); + else + return null; Quat4d q = getControlPointOrientationQuat(dir, pcp.getRotationAngle() != null ? pcp.getRotationAngle() : 0.0); AxisAngle4d aa = new AxisAngle4d(MathTools.Y_AXIS,pcp.getTurnAngle() == null ? 0.0 : pcp.getTurnAngle()); Quat4d q2 = MathTools.getQuat(aa); @@ -499,6 +501,8 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { dir.sub(next.getWorldPosition(),pcp.getWorldPosition()); if (dir.lengthSquared() > MathTools.NEAR_ZERO) dir.normalize(); + else + return null; Quat4d q = getControlPointOrientationQuat(dir, pcp.getRotationAngle() != null ? pcp.getRotationAngle() : 0.0); AxisAngle4d aa = new AxisAngle4d(MathTools.Y_AXIS,pcp.getTurnAngle() == null ? 0.0 : pcp.getTurnAngle()); Quat4d q2 = MathTools.getQuat(aa); @@ -1149,18 +1153,34 @@ public class PipeControlPoint extends G3DNode implements IP3DNode { if (component == null) return; PipelineComponent next = component.getNext(); - PipelineComponent prev = component.getNext(); + PipelineComponent prev = component.getPrevious(); + PipelineComponent br0 = component.getBranch0(); + component.setNext(null); + component.setPrevious(null); + component.setBranch0(null); if (next != null) { if (next.getNext() == component) next.setNext(null); else if (next.getPrevious() == component) next.setPrevious(null); + else if (next.getBranch0() == component) + next.setBranch0(null); } if (prev != null) { if (prev.getNext() == component) prev.setNext(null); else if (prev.getPrevious() == component) prev.setPrevious(null); + else if (prev.getBranch0() == component) + prev.setBranch0(null); + } + if (br0 != null) { + if (br0.getNext() == component) + prev.setNext(null); + else if (br0.getPrevious() == component) + prev.setPrevious(null); + else if (br0.getBranch0() == component) + br0.setBranch0(null); } PipelineComponent comp = component; component = null; diff --git a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipingRules.java b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipingRules.java index be66cf14..67c5f35b 100644 --- a/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipingRules.java +++ b/org.simantics.plant3d/src/org/simantics/plant3d/scenegraph/controlpoint/PipingRules.java @@ -1612,7 +1612,7 @@ public class PipingRules { turnAngle = 0.0; tcp.setTurnAngle(0.0); tcp.setLength(0.0); - tcp.setTurnAxis(MathTools.Y_AXIS); + tcp.setTurnAxis(new Vector3d(MathTools.Y_AXIS)); } updateControlPointOrientation(tcp); if (DEBUG) @@ -1731,7 +1731,7 @@ public class PipingRules { } List runPcps = getControlPoints(pipeRun); if (runPcps.size() != count) { - System.out.println("Run is not connected"); + System.out.println("Run " + pipeRun.getName() + " contains unconnected control points"); } for (PipeControlPoint pcp : pcps) { if (!pcp.isDirected() && pcp.getNext() == null && pcp.getPrevious() == null) -- 2.47.1