[Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-22 Thread Chris Pavlina 
If you place a wire segment and then backtrack over it, you end up with 
a broken wire in the end:

http://misc.c4757p.com/backtrack.mp4

This is because the two segments are merged together, rather than the 
second *subtracting* from the first, even though the latter case is the 
way it's shown as you're drawing. This patch fixes that by adding a 
function to erase backtracks.

--
Chris
commit d18fec08e34f7b509553fc72ee0616eeda6579a6
Author: Chris Pavlina 
Date:   Mon Jun 22 16:16:50 2015 -0400

Remove backtracking segments when drawing wires

diff --git a/eeschema/bus-wire-junction.cpp b/eeschema/bus-wire-junction.cpp
index a33f7b5..eb86ba5 100644
--- a/eeschema/bus-wire-junction.cpp
+++ b/eeschema/bus-wire-junction.cpp
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 static void AbortCreateNewLine( EDA_DRAW_PANEL* aPanel, wxDC* aDC );
@@ -53,6 +54,45 @@ static DLIST< SCH_ITEM > s_wires;   // when creating a new set of wires,
 
 
 /**
+ * In a contiguous list of wires, remove wires that backtrack over the previous
+ * wire. Example:
+ *
+ * Wire is added:
+ * >
+ *
+ * A second wire backtracks over it:
+ * ---<>
+ *
+ * RemoveBacktracks is called:
+ * --->
+ */
+static void RemoveBacktracks( DLIST& aWires )
+{
+SCH_LINE* last_line = NULL;
+
+EDA_ITEM* first = aWires.GetFirst();
+for( EDA_ITEM* p = first; p; p = p->Next() )
+{
+SCH_LINE *line = dynamic_cast( p );
+
+if( p != first )
+{
+wxASSERT_MSG( last_line->GetEndPoint() == line->GetStartPoint(),
+"RemoveBacktracks() requires contiguous lines" );
+if( IsPointOnSegment( last_line->GetStartPoint(), line->GetStartPoint(),
+line->GetEndPoint() ) )
+{
+last_line->SetEndPoint( line->GetEndPoint() );
+delete s_wires.Remove( line );
+p = line;
+}
+}
+last_line = line;
+}
+}
+
+
+/**
  * Mouse capture callback for drawing line segments.
  */
 static void DrawSegment( EDA_DRAW_PANEL* aPanel, wxDC* aDC, const wxPoint& aPosition,
@@ -261,6 +301,9 @@ void SCH_EDIT_FRAME::EndSegment( wxDC* DC )
 
 SaveCopyInUndoList( oldItems, UR_WIRE_IMAGE );
 
+// Remove segments backtracking over others
+RemoveBacktracks( s_wires );
+
 // Add the new wires
 screen->Append( s_wires );
 
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-22 Thread Nick Østergaard 
I orginally pinged Chris about this bug and he friendly took up the
task. Thumbs up.

I have applied the patch and it fixes the issue.

On a side note:
There was once upon a time that one could draw backwards on a wire to
delete it, but one can't do that anymore.  (But I think that has been
impossible for quite some time now.)   Is this something I am dreaming
or was this really the case?

2015-06-22 22:50 GMT+02:00 Chris Pavlina :
> If you place a wire segment and then backtrack over it, you end up with
> a broken wire in the end:
>
> http://misc.c4757p.com/backtrack.mp4
>
> This is because the two segments are merged together, rather than the
> second *subtracting* from the first, even though the latter case is the
> way it's shown as you're drawing. This patch fixes that by adding a
> function to erase backtracks.
>
> --
> Chris
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-23 Thread jp charras 
Le 22/06/2015 22:50, Chris Pavlina a écrit :
> If you place a wire segment and then backtrack over it, you end up with 
> a broken wire in the end:
> 
> http://misc.c4757p.com/backtrack.mp4
> 
> This is because the two segments are merged together, rather than the 
> second *subtracting* from the first, even though the latter case is the 
> way it's shown as you're drawing. This patch fixes that by adding a 
> function to erase backtracks.
> 
> --
> Chris
> 

Thanks for this patch, it is very usefull.

I noticed you are using a dynamic cast in line:

SCH_LINE *line = dynamic_cast( p );

but the variable line is used without any test for NULL.

Can you have a look at this (use a static cast or a test for NULL) ?

Thanks.


-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-23 Thread Chris Pavlina 
Sure, I can. I usually prefer to use dynamic_cast because it will fail 
if the subclass isn't correct (even if you don't test, since a NULL 
access will result in a segfault) - just more or less as a sanity check. 
There's really no circumstance under which the pointer should not be an 
instance of SCH_LINE, and the behavior when it fails with dynamic_cast 
is a bit more obvious.

I can throw an assert in there, though, that'd definitely be more 
correct. Modified patch attached.

On Tue, Jun 23, 2015 at 09:24:36AM +0200, jp charras wrote:
> Le 22/06/2015 22:50, Chris Pavlina a écrit :
> > If you place a wire segment and then backtrack over it, you end up with 
> > a broken wire in the end:
> > 
> > http://misc.c4757p.com/backtrack.mp4
> > 
> > This is because the two segments are merged together, rather than the 
> > second *subtracting* from the first, even though the latter case is the 
> > way it's shown as you're drawing. This patch fixes that by adding a 
> > function to erase backtracks.
> > 
> > --
> > Chris
> > 
> 
> Thanks for this patch, it is very usefull.
> 
> I noticed you are using a dynamic cast in line:
> 
> SCH_LINE *line = dynamic_cast( p );
> 
> but the variable line is used without any test for NULL.
> 
> Can you have a look at this (use a static cast or a test for NULL) ?
> 
> Thanks.
> 
> 
> -- 
> Jean-Pierre CHARRAS
commit 5b7beeba531124fe9b7786df9f4e977f8ada7ff5
Author: Chris Pavlina 
Date:   Tue Jun 23 09:18:36 2015 -0400

Remove backtracking segments when drawing wires

diff --git a/eeschema/bus-wire-junction.cpp b/eeschema/bus-wire-junction.cpp
index 62141bd..c173f4e 100644
--- a/eeschema/bus-wire-junction.cpp
+++ b/eeschema/bus-wire-junction.cpp
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 static void AbortCreateNewLine( EDA_DRAW_PANEL* aPanel, wxDC* aDC );
@@ -53,6 +54,47 @@ static DLIST< SCH_ITEM > s_wires;   // when creating a new set of wires,
 
 
 /**
+ * In a contiguous list of wires, remove wires that backtrack over the previous
+ * wire. Example:
+ *
+ * Wire is added:
+ * >
+ *
+ * A second wire backtracks over it:
+ * ---<>
+ *
+ * RemoveBacktracks is called:
+ * --->
+ */
+static void RemoveBacktracks( DLIST& aWires )
+{
+SCH_LINE* last_line = NULL;
+
+EDA_ITEM* first = aWires.GetFirst();
+for( EDA_ITEM* p = first; p; p = p->Next() )
+{
+SCH_LINE *line = dynamic_cast( p );
+wxASSERT_MSG( line, "RemoveBacktracks() requires SCH_LINE items" );
+continue;
+
+if( p != first )
+{
+wxASSERT_MSG( last_line->GetEndPoint() == line->GetStartPoint(),
+"RemoveBacktracks() requires contiguous lines" );
+if( IsPointOnSegment( last_line->GetStartPoint(), line->GetStartPoint(),
+line->GetEndPoint() ) )
+{
+last_line->SetEndPoint( line->GetEndPoint() );
+delete s_wires.Remove( line );
+p = line;
+}
+}
+last_line = line;
+}
+}
+
+
+/**
  * Mouse capture callback for drawing line segments.
  */
 static void DrawSegment( EDA_DRAW_PANEL* aPanel, wxDC* aDC, const wxPoint& aPosition,
@@ -261,6 +303,9 @@ void SCH_EDIT_FRAME::EndSegment( wxDC* DC )
 
 SaveCopyInUndoList( oldItems, UR_WIRE_IMAGE );
 
+// Remove segments backtracking over others
+RemoveBacktracks( s_wires );
+
 // Add the new wires
 screen->Append( s_wires );
 
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-23 Thread jp charras 
Le 23/06/2015 15:19, Chris Pavlina a écrit :
> Sure, I can. I usually prefer to use dynamic_cast because it will fail 
> if the subclass isn't correct (even if you don't test, since a NULL 
> access will result in a segfault) - just more or less as a sanity check. 
> There's really no circumstance under which the pointer should not be an 
> instance of SCH_LINE, and the behavior when it fails with dynamic_cast 
> is a bit more obvious.
> 
> I can throw an assert in there, though, that'd definitely be more 
> correct. Modified patch attached.

Thanks.

But there is yet something strange for me:
I am thinking data is used after deleting:
delete s_wires.Remove( line ) delete the data referenced by line.
then after p = line and last_line = line.
but p->Next() and last_line->GetStartPoint() use this deleted data.

I can be wrong.

-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-23 Thread Chris Pavlina 
Wow. I made enough stupid yesterday for all of us, didn't I?

Here you go. I'll try to read over my code more carefully, today...


On Tue, Jun 23, 2015 at 04:04:25PM +0200, jp charras wrote:
> Le 23/06/2015 15:19, Chris Pavlina a écrit :
> > Sure, I can. I usually prefer to use dynamic_cast because it will fail 
> > if the subclass isn't correct (even if you don't test, since a NULL 
> > access will result in a segfault) - just more or less as a sanity check. 
> > There's really no circumstance under which the pointer should not be an 
> > instance of SCH_LINE, and the behavior when it fails with dynamic_cast 
> > is a bit more obvious.
> > 
> > I can throw an assert in there, though, that'd definitely be more 
> > correct. Modified patch attached.
> 
> Thanks.
> 
> But there is yet something strange for me:
> I am thinking data is used after deleting:
> delete s_wires.Remove( line ) delete the data referenced by line.
> then after p = line and last_line = line.
> but p->Next() and last_line->GetStartPoint() use this deleted data.
> 
> I can be wrong.
> 
> -- 
> Jean-Pierre CHARRAS
commit 8b36ca6c62fb2c1a125dc0ba25b82f42d3f7d1c9
Author: Chris Pavlina 
Date:   Tue Jun 23 10:22:00 2015 -0400

Remove backtracking segments when drawing wires

diff --git a/eeschema/bus-wire-junction.cpp b/eeschema/bus-wire-junction.cpp
index 62141bd..d0c6994 100644
--- a/eeschema/bus-wire-junction.cpp
+++ b/eeschema/bus-wire-junction.cpp
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 
 static void AbortCreateNewLine( EDA_DRAW_PANEL* aPanel, wxDC* aDC );
@@ -53,6 +54,53 @@ static DLIST< SCH_ITEM > s_wires;   // when creating a new set of wires,
 
 
 /**
+ * In a contiguous list of wires, remove wires that backtrack over the previous
+ * wire. Example:
+ *
+ * Wire is added:
+ * >
+ *
+ * A second wire backtracks over it:
+ * ---<>
+ *
+ * RemoveBacktracks is called:
+ * --->
+ */
+static void RemoveBacktracks( DLIST& aWires )
+{
+SCH_LINE* last_line = NULL;
+
+EDA_ITEM* first = aWires.GetFirst();
+for( EDA_ITEM* p = first; p; )
+{
+SCH_LINE *line = dynamic_cast( p );
+if( !line )
+{
+wxFAIL_MSG( "RemoveBacktracks() requires SCH_LINE items" );
+break;
+}
+p = line->Next();
+
+if( last_line )
+{
+wxASSERT_MSG( last_line->GetEndPoint() == line->GetStartPoint(),
+"RemoveBacktracks() requires contiguous lines" );
+if( IsPointOnSegment( last_line->GetStartPoint(), line->GetStartPoint(),
+line->GetEndPoint() ) )
+{
+last_line->SetEndPoint( line->GetEndPoint() );
+delete s_wires.Remove( line );
+}
+else
+last_line = line;
+}
+else
+last_line = line;
+}
+}
+
+
+/**
  * Mouse capture callback for drawing line segments.
  */
 static void DrawSegment( EDA_DRAW_PANEL* aPanel, wxDC* aDC, const wxPoint& aPosition,
@@ -261,6 +309,9 @@ void SCH_EDIT_FRAME::EndSegment( wxDC* DC )
 
 SaveCopyInUndoList( oldItems, UR_WIRE_IMAGE );
 
+// Remove segments backtracking over others
+RemoveBacktracks( s_wires );
+
 // Add the new wires
 screen->Append( s_wires );
 
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Kicad-developers] [PATCH] Remove backtracking wires in eeschema

2015-06-23 Thread jp charras 
Le 23/06/2015 16:24, Chris Pavlina a écrit :
> Wow. I made enough stupid yesterday for all of us, didn't I?
> 
> Here you go. I'll try to read over my code more carefully, today...
> 


Committed. Thanks.


-- 
Jean-Pierre CHARRAS

___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp