Something's wrong with wiring

[fpw]

I spent most of the weekend finishing my first project in Antares, and I really like the GUI, the new global tunnels and working on my circuit across several sub-circuits!

However, while debugging the circuit, I had to learn the hard way that nearly all of my issues were caused by a wiring issue in Antares. Unfortunately, I don't know how to reproduce this at all, but I had about 10 instances of the following error: Antares will show that a gate is connected to all its wires, e.g. dragging the gate will also move all connections. But one of the wires is somehow not really connected - it just looks like it is, as if it's only connected in the view but not in the model!

The NAND gate in the screenshot shows this: Both inputs are high, but the output doesn't turn low.

I imagine this will be nearly impossible to fix because I don't know how to reproduce it, but I have a different suggestion that would be very useful until the root cause is fixed: Antares knows that something is wrong. If you try to delete the NAND gate, Antares will show an error because some internal assertion fails. My suggestion is: Could Antares do a kind of integrity check when saving and loading a circuit, highlighting gates that have this issue? Because manually finding this was very hard. I basically had to delete gates manually and see where Antares shows an error - but re-opening the circuit after each attempt because a successfully deleted gate could fix the wiring of a broken gate.

Here's a circuit where this issue is still present, and I added three switches to highlight it. Turning all of them on will cause the situation in the screenshot. Don't mind the missing global signals leading to blue wires, this is irrelevant for the issue.

wiring.zip

[flandreas]

@fpw Thank you, Folke! I‘m sorry, I think you ran into the one bug that plagued Antares for years now, and that is the main reason why Antares doesn‘t gain more traction.

My current guess is that it is somehow related with the undo/redo system, and there in particular with the snapshot subsystem.

The consistency check you‘re suggesting is already implemented, but it is currently disabled. The idea was that Antares detects the issue, the User would then have to let Antares generate a dump of the entire user‘s project (already implemented), including the detailed user trail log, and send it to me. Then I could maybe redo all user actions from the last correct circuit file version, and identify the one action that leads to the bug.

I will take care of it this evening.

[flandreas]

Same as #584

[fpw]

Quick question regarding the command concept you explained in #714: Do some commands work based on coordinates? For example, if a command contains a wire placement, is that based on coordinates and the playback expects that the original wire still has snap-connect points at the same coordinates to create a connection in the model? Or does it reference a wire by ID and the coordinates are only for the view?

I'm asking because I'm also trying to find a reproducible case, and I think I found a situation where the wire in a snapshot and the actual circuit are slightly different due to some automagic movement that happens after the snapshot, but not sure whether I should try further from there.

[fpw]

When trying this, I've got this in developer mode, not sure if it would have caused the issue later if I hadn't been in developer mode:

Version: 1.12.0
Move segment 1 of EdgeView 41
Move segment 0 of EdgeView 41
Move EdgeView 33 endpoint open-ended
Delete component with ID 33
Move component 'FF' with ID 32 by key
Move component 'FF' with ID 32 by key
Move component 'FF' with ID 32 by key
Max snapshot size reached. Create new snapshot.
Move component 'FF' with ID 32 by key
Move component 'FF' with ID 32 by key
Move component 'FF' with ID 32 by key
Move component 'FF' with ID 32 by key
Delete component with ID 44
Undo command 'Delete'
Move segment 1 of EdgeView 36
Delete component with ID 5
Undo command 'Delete'
Move component 'FF' with ID 32 by key
Delete component with ID 5
Undo command 'Delete'
java.lang.IndexOutOfBoundsException: Index 2 out of bounds for length 2
	at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
	at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
	at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
	at java.base/java.util.Objects.checkIndex(Objects.java:359)
	at java.base/java.util.ArrayList.get(ArrayList.java:427)
	at ch.scorpion.jabbah.draw.polyline.PolylineShape$Companion.isSegmentHorizontal(PolylineShape.kt:24)
	at ch.scorpion.jabbah.draw.polyline.PolylineShapeImpl.isSegmentHorizontal(PolylineShape.kt:268)
	at ch.scorpion.jabbah.draw.polyline.Polyline$DefaultImpls.isSegmentOrthogonal(Polyline.kt:164)
	at ch.scorpion.jabbah.draw.polyline.PolylineShape$DefaultImpls.isSegmentOrthogonal(PolylineShape.kt:19)
	at ch.scorpion.jabbah.draw.polyline.PolylineShapeImpl.isSegmentOrthogonal(PolylineShape.kt:94)
	at ch.scorpion.jabbah.draw.polyline.PolylineShapeJvm.isSegmentOrthogonal(PolylineShapeJvm.kt)
	at ch.scorpion.jabbah.graph.view.net.edge.LayoutType.getSegmentDirection(LayoutType.kt:156)
	at ch.scorpion.jabbah.graph.view.net.edge.EdgeViewImpl.getSegmentDirection(EdgeViewImpl.kt:589)
	at ch.scorpion.jabbah.graph.view.net.edge.EdgeViewImpl.moveSegment(EdgeViewImpl.kt:493)
	at ch.scorpion.jabbah.graph.view.net.edge.MoveSegmentCommand.execute(MoveSegmentCommand.kt:23)
	at ch.scorpion.jabbah.edit.command.Transaction.execute(Transaction.kt:25)
	at ch.scorpion.jabbah.edit.command.Snapshot.replayFromSnapshot(Snapshot.kt:71)
	at ch.scorpion.jabbah.edit.command.Snapshot.undo(Snapshot.kt:46)
	at ch.scorpion.jabbah.edit.command.SourcingCommandManager.undo(SourcingCommandManager.kt:195)
	at ch.scorpion.jabbah.edit.app.UndoAction.execute(UndoRedo.kt:40)
	at ch.scorpion.jabbah.base.ActionWrapperSwing.actionPerformed(ActionWrapperSwing.kt:73)
	at java.desktop/javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1972)
	at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2313)
	at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:405)
	at java.desktop/javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:262)
	at java.desktop/javax.swing.AbstractButton.doClick(AbstractButton.java:374)
	at java.desktop/javax.swing.AbstractButton.doClick(AbstractButton.java:354)
	at java.desktop/javax.swing.plaf.basic.BasicMenuItemUI$Actions.actionPerformed(BasicMenuItemUI.java:977)
	at java.desktop/javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1809)
	at java.desktop/javax.swing.JComponent.processKeyBinding(JComponent.java:2900)

[flandreas]

Connect commands generally use component and port IDs and not UI coordinates.

However, MoveSegmentCommand (used when moving an individual segment with the mouse) is slightly different. It stores the ID of the wire being moved, the offset (in pixels), and the index of the moved segment in the wire.

Looking at the stack trace in your scenario, you might have found something here. When moving components or creating connections, recalculation of wire layouts are involved (also when replaying from Commands during undo), and this does depend on UI coordinates. And in your scenario, MoveSegment seems to break down because of invalid wire geometry.

Interesting case! I'll deep-dive into this one..

[flandreas]

MoveSegmentCommand is definitively broken. It wants to be "Undoable" and tries to keep and update the segment index the user has moved. The index changes not only in execute() (and therefore in snapshot replaying), but also in undo(). The problem is that wires can change their layout in the meantime, e.g. when components are deleted, making the segment index MoveSegmentCommand is desperately trying to keep up-to-date invalid.

Commands should only be Undoable if they don't depend on circuit structure, e.g. if they only change a property like the color of a component. This is definitively not the case for MoveSegmentCommand.

Solution: MoveSegmentCommand must not be Undoable, i.e. must always be replayed.

---

Undo stack evolution

Move segment 1 of EdgeView 41 MS

Move segment 0 of EdgeView 41 MS MS

Move EdgeView 33 endpoint open-ended MS MS ME

Delete component with ID 33 MS MS ME D

Move component 'FF' with ID 32 by key MS MS ME D M

Move component 'FF' with ID 32 by key MS MS ME D M M

Move component 'FF' with ID 32 by key Max snapshot size reached. Create new snapshot. MS MS ME D M M | M

Move component 'FF' with ID 32 by key MS MS ME D M M | M M

Move component 'FF' with ID 32 by key MS MS ME D M M | M M M

Move component 'FF' with ID 32 by key MS MS ME D M M | M M M M

Move component 'FF' with ID 32 by key MS MS ME D M M | M M M M M

Delete component with ID 44 MS MS ME D M M | M M M M M D.44

Undo command 'Delete'

Replay M M M M M MS MS ME D M M | M M M M M

Move segment 1 of EdgeView 36 MS MS ME D M M | M M M M M MS.36.1

Delete component with ID 5 MS MS ME D M M | M M M M M MS.36.1 D.5 => Can lead to re-layout of EdgeViews. MS.36.1 segment index can become invalid.

Undo command 'Delete' -> Replay M M M M M MS.36.1 MS MS ME D M M | M M M M M MS.36.1 => MS.36.1 is replayed with possibly invalid segment index => Event if not: segment index is updated while replaying

Move component 'FF' with ID 32 by key MS MS ME D M M | M M M M M MS.36.1 M => Can lead to re-layout of EdgeViews. MS.36.1 segment index can become invalid.

Delete component with ID 5 MS MS ME D M M | M M M M M MS.36.1 M D.5

Undo command 'Delete' -> Replay M M M M M MS.36.1 M => Exception in MS.execute()

[fpw]

Sounds good! While I'm not sure that the exception above is the same bug that leads to the wiring issue, maybe the root cause is the same and both are fixed now.

[flandreas]

I have the feeling that the wiring problem is still caused by something else.

We have now a couple of fixes of severe bugs in release-1.11. I would suggest that I make a release 1.11.1 today and activate the GraphViewConsistencyCheck that detects the wire issue and signals it to users, asking them to create a system dump and send it to us.

In the meantime, I will take a closer look at the undo behaviour of DeleteCommand, which is quite complex with its child UnconnectCommands. Maybe write a couple more tests in that area.

[fpw]

The integrity check worked and created the following dump today: https://pdp8.app/Antares-dump.zip

I was deleting a delay gate and a buffer that existed between EAE TP and the output of the previous block (similar as above which I wanted to connect afterwards), then wanted to connect the remaining wires and it happened. No Undo / Redo used, as far as I remember.

Edit: Wow, it's actually reproducible. The entire area seems to be cursed, it took me 7 attempts to find a way to delete the two gates and reconnect without the malfunction. I hope it's not a false positive or the circuit already being broken before.

https://github.com/flandreas/antares/assets/5798899/e4017ea1-5082-446e-bb2d-37ef286cdfa2

[flandreas]

Aah great! I see in the video that you were combining wires. I never do that, didn't even know anymore that this a feature ;-) That's a hot candidate, I'll look into it..

[flandreas]

Yep, looks like you've found it! Nasty bug in the wire connection core logic. Occurs whenever wires are joined, e.g. when dragging wire endpoints onto each other, but also when wire junctions are deleted. Now what's left is to extend the unit test set and implement the correct fix...

[fpw]

Great, thanks for the feedback - glad that it helped!

[flandreas]

Fixed with #726

[fpw]

Just a little feedback: While still working almost daily with Antares, I haven't encountered any more wiring issues since the latest fix! This looks great, previously I had several wiring issues per day.

[flandreas]

Thank you for the feedback, I'm glad to hear that! Encourages me to build the next release soon.