From 478df24f3654d19487f4523ff475c706cdab54b8 Mon Sep 17 00:00:00 2001 From: John Beard Date: Sat, 29 Jul 2023 14:15:03 +0100 Subject: [PATCH] Fix fillets and chamfers when the original lines become zero-length This needs the ITEM_MODIFICATION_ROUTINE to learn to delete items. Condense the item change handlers into a single injected object (ITEM_MODIFICATION_ROUTINE::CHANGE_HANDLER) and provide the basic implementation that just takes some callables. This simplifies the construction of the routines and also would make a CHANGE_HANDLER object possible that can be reused between different tools. --- pcbnew/tools/edit_tool.cpp | 35 +++-- pcbnew/tools/item_modification_routine.cpp | 50 ++++-- pcbnew/tools/item_modification_routine.h | 167 +++++++++++++++------ 3 files changed, 178 insertions(+), 74 deletions(-) diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp index da21dd5e12..88869a3c37 100644 --- a/pcbnew/tools/edit_tool.cpp +++ b/pcbnew/tools/edit_tool.cpp @@ -1201,6 +1201,9 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) // (doing it as we go will invalidate the iterator) std::vector items_to_select_on_success; + // And same for items to deselect + std::vector items_to_deselect_on_success; + // Handle modifications to existing items by the routine // How to deal with this depends on whether we're in the footprint editor or not // and whether the item was conjured up by decomposing a polygon or rectangle @@ -1214,7 +1217,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) } }; - bool any_items_created = false; + bool any_items_created = !lines_to_add.empty(); const auto item_creation_handler = [&]( std::unique_ptr aItem ) { any_items_created = true; @@ -1222,6 +1225,18 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) commit.Add( aItem.release() ); }; + bool any_items_removed = !items_to_remove.empty(); + const auto item_removal_handler = [&]( PCB_SHAPE& aItem ) + { + any_items_removed = true; + items_to_deselect_on_success.push_back( &aItem ); + commit.Remove( &aItem ); + }; + + // Combine these callbacks into a CHANGE_HANDLER to inject in the ROUTINE + ITEM_MODIFICATION_ROUTINE::CALLABLE_BASED_HANDLER change_handler( + item_creation_handler, item_modification_handler, item_removal_handler ); + // Construct an appropriate tool std::unique_ptr pairwise_line_routine; wxString error_message; @@ -1233,8 +1248,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) if( filletRadiusIU.has_value() ) { pairwise_line_routine = std::make_unique( - frame()->GetModel(), item_creation_handler, item_modification_handler, - *filletRadiusIU ); + frame()->GetModel(), change_handler, *filletRadiusIU ); } } else if( aEvent.IsAction( &PCB_ACTIONS::chamferLines ) ) @@ -1245,8 +1259,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) if( chamfer_params.has_value() ) { pairwise_line_routine = std::make_unique( - frame()->GetModel(), item_creation_handler, item_modification_handler, - *chamfer_params ); + frame()->GetModel(), change_handler, *chamfer_params ); } } else if( aEvent.IsAction( &PCB_ACTIONS::extendLines ) ) @@ -1257,8 +1270,8 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) } else { - pairwise_line_routine = std::make_unique( - frame()->GetModel(), item_creation_handler, item_modification_handler ); + pairwise_line_routine = + std::make_unique( frame()->GetModel(), change_handler ); } } @@ -1301,10 +1314,14 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent ) for( PCB_SHAPE* item : items_to_select_on_success ) m_selectionTool->AddItemToSel( item, true ); - if( !items_to_remove.empty() ) + // Deselect removed items + for( PCB_SHAPE* item : items_to_deselect_on_success ) + m_selectionTool->RemoveItemFromSel( item, true ); + + if( any_items_removed ) m_toolMgr->ProcessEvent( EVENTS::UnselectedEvent ); - if( !any_items_created ) + if( any_items_created ) m_toolMgr->ProcessEvent( EVENTS::SelectedEvent ); // Notify other tools of the changes diff --git a/pcbnew/tools/item_modification_routine.cpp b/pcbnew/tools/item_modification_routine.cpp index 40e99e76b2..cb217ff477 100644 --- a/pcbnew/tools/item_modification_routine.cpp +++ b/pcbnew/tools/item_modification_routine.cpp @@ -36,6 +36,28 @@ bool SegmentsShareEndpoint( const SEG& aSegA, const SEG& aSegB ) } // namespace + +bool ITEM_MODIFICATION_ROUTINE::ModifyLineOrDeleteIfZeroLength( PCB_SHAPE& aLine, const SEG& aSeg ) +{ + wxASSERT_MSG( aLine.GetShape() == SHAPE_T::SEGMENT, "Can only modify segments" ); + + const bool removed = aSeg.Length() == 0; + if( !removed ) + { + // Mark modified, then change it + GetHandler().MarkItemModified( aLine ); + aLine.SetStart( aSeg.A ); + aLine.SetEnd( aSeg.B ); + } + else + { + // The line has become zero length - delete it + GetHandler().DeleteItem( aLine ); + } + return removed; +} + + wxString LINE_FILLET_ROUTINE::GetCommitDescription() const { return _( "Fillet Lines" ); @@ -128,17 +150,15 @@ void LINE_FILLET_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLineB tArc->SetLayer( aLineA.GetLayer() ); tArc->SetLocked( aLineA.IsLocked() ); - MarkItemModified( aLineA ); - MarkItemModified( aLineB ); + CHANGE_HANDLER& handler = GetHandler(); - AddNewItem( std::move( tArc ) ); + handler.AddNewItem( std::move( tArc ) ); *a_pt = t1newPoint; *b_pt = t2newPoint; - aLineA.SetStart( seg_a.A ); - aLineA.SetEnd( seg_a.B ); - aLineB.SetStart( seg_b.A ); - aLineB.SetEnd( seg_b.B ); + + ModifyLineOrDeleteIfZeroLength( aLineA, seg_a ); + ModifyLineOrDeleteIfZeroLength( aLineB, seg_b ); AddSuccess(); } @@ -194,16 +214,12 @@ void LINE_CHAMFER_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLineB tSegment->SetLayer( aLineA.GetLayer() ); tSegment->SetLocked( aLineA.IsLocked() ); - AddNewItem( std::move( tSegment ) ); + CHANGE_HANDLER& handler = GetHandler(); - MarkItemModified( aLineA ); - MarkItemModified( aLineB ); + handler.AddNewItem( std::move( tSegment ) ); - // Shorten the original lines - aLineA.SetStart( chamfer_result->m_updated_seg_a->A ); - aLineA.SetEnd( chamfer_result->m_updated_seg_a->B ); - aLineB.SetStart( chamfer_result->m_updated_seg_b->A ); - aLineB.SetEnd( chamfer_result->m_updated_seg_b->B ); + ModifyLineOrDeleteIfZeroLength( aLineA, *chamfer_result->m_updated_seg_a ); + ModifyLineOrDeleteIfZeroLength( aLineB, *chamfer_result->m_updated_seg_b ); AddSuccess(); } @@ -247,6 +263,8 @@ void LINE_EXTENSION_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLin return; } + CHANGE_HANDLER& handler = GetHandler(); + const auto line_extender = [&]( const SEG& aSeg, PCB_SHAPE& aLine ) { // If the intersection point is not already n the line, we'll extend to it @@ -257,7 +275,7 @@ void LINE_EXTENSION_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLin const VECTOR2I& furthest_pt = ( dist_start < dist_end ) ? aSeg.B : aSeg.A; - MarkItemModified( aLine ); + handler.MarkItemModified( aLine ); aLine.SetStart( furthest_pt ); aLine.SetEnd( *intersection ); } diff --git a/pcbnew/tools/item_modification_routine.h b/pcbnew/tools/item_modification_routine.h index 1587e26862..22d929f951 100644 --- a/pcbnew/tools/item_modification_routine.h +++ b/pcbnew/tools/item_modification_routine.h @@ -47,37 +47,112 @@ class ITEM_MODIFICATION_ROUTINE { public: /* - * Handlers for receiving changes from the tool - * - * These are used to allow the tool's caller to make changes to - * affected board items using extra information that the tool - * does not have access to (e.g. is this an FP editor, was - * the line created from a rectangle and needs to be added, not - * modified, etc). - * - * We can't store them up until the end, because modifications - * need the old state to be known. - */ + * Handlers for receiving changes from the tool + * + * These are used to allow the tool's caller to make changes to + * affected board items using extra information that the tool + * does not have access to (e.g. is this an FP editor, was + * the line created from a rectangle and needs to be added, not + * modified, etc). + * + * We can't store them up until the end, because modifications + * need the old state to be known, so this allows the caller to + * inject the dependencies for how to handle the changes. + */ + class CHANGE_HANDLER + { + public: + virtual ~CHANGE_HANDLER() = default; + + /** + * @brief Report that the tools wants to add a new item to the board + * + * @param aItem the new item + */ + virtual void AddNewItem( std::unique_ptr aItem ) = 0; + + /** + * @brief Report that the tool has modified an item on the board + * + * @param aItem the modified item + */ + virtual void MarkItemModified( PCB_SHAPE& aItem ) = 0; + + /** + * @brief Report that the tool has deleted an item on the board + * + * @param aItem the deleted item + */ + virtual void DeleteItem( PCB_SHAPE& aItem ) = 0; + }; /** - * Handler for creating a new item on the board - * - * @param PCB_SHAPE& the shape to add + * @brief A handler that is based on a set of callbacks provided + * by the user of the ITEM_MODIFICATION_ROUTINE */ - using CREATION_HANDLER = std::function )>; + class CALLABLE_BASED_HANDLER : public CHANGE_HANDLER + { + public: + /** + * Handler for creating a new item on the board + * + * @param PCB_SHAPE& the shape to add + */ + using CREATION_HANDLER = std::function )>; - /** - * Handler for modifying an existing item on the board - * - * @param PCB_SHAPE& the shape to modify - */ - using MODIFICATION_HANDLER = std::function; + /** + * Handler for modifying or deleting an existing item on the board + * + * @param PCB_SHAPE& the shape to modify + */ + using MODIFICATION_HANDLER = std::function; - ITEM_MODIFICATION_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler, - MODIFICATION_HANDLER aModificationHandler ) : - m_board( aBoard ), - m_creationHandler( aCreationHandler ), m_modificationHandler( aModificationHandler ), - m_numSuccesses( 0 ), m_numFailures( 0 ) + /** + * Handler for modifying or deleting an existing item on the board + * + * @param PCB_SHAPE& the shape to delete + */ + using DELETION_HANDLER = std::function; + + CALLABLE_BASED_HANDLER( CREATION_HANDLER aCreationHandler, + MODIFICATION_HANDLER aModificationHandler, + DELETION_HANDLER aDeletionHandler ) : + m_creationHandler( aCreationHandler ), + m_modificationHandler( aModificationHandler ), m_deletionHandler( aDeletionHandler ) + { + } + + /** + * @brief Report that the tools wants to add a new item to the board + * + * @param aItem the new item + */ + void AddNewItem( std::unique_ptr aItem ) override + { + m_creationHandler( std::move( aItem ) ); + } + + /** + * @brief Report that the tool has modified an item on the board + * + * @param aItem the modified item + */ + void MarkItemModified( PCB_SHAPE& aItem ) override { m_modificationHandler( aItem ); } + + /** + * @brief Report that the tool has deleted an item on the board + * + * @param aItem the deleted item + */ + void DeleteItem( PCB_SHAPE& aItem ) override { m_deletionHandler( aItem ); } + + CREATION_HANDLER m_creationHandler; + MODIFICATION_HANDLER m_modificationHandler; + DELETION_HANDLER m_deletionHandler; + }; + + ITEM_MODIFICATION_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) : + m_board( aBoard ), m_handler( aHandler ), m_numSuccesses( 0 ), m_numFailures( 0 ) { } @@ -109,23 +184,22 @@ protected: void AddFailure() { ++m_numFailures; } /** - * @brief Report that the tools wants to add a new item to the board + * @brief Helper function useful for multiple tools: modify a line or delete + * it if it has zero length * - * @param aItem the new item + * @param aItem the line to modify + * @param aSeg the new line geometry */ - void AddNewItem( std::unique_ptr aItem ) { m_creationHandler( std::move( aItem ) ); } + bool ModifyLineOrDeleteIfZeroLength( PCB_SHAPE& aItem, const SEG& aSeg ); /** - * @brief Report that the tool has modified an item on the board - * - * @param aItem the modified item + * @brief Access the handler for making changes to the board */ - void MarkItemModified( PCB_SHAPE& aItem ) { m_modificationHandler( aItem ); } + CHANGE_HANDLER& GetHandler() { return m_handler; } private: BOARD_ITEM* m_board; - CREATION_HANDLER m_creationHandler; - MODIFICATION_HANDLER m_modificationHandler; + CHANGE_HANDLER& m_handler; unsigned m_numSuccesses; unsigned m_numFailures; @@ -137,9 +211,8 @@ private: class PAIRWISE_LINE_ROUTINE : public ITEM_MODIFICATION_ROUTINE { public: - PAIRWISE_LINE_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler, - MODIFICATION_HANDLER aModificationHandler ) : - ITEM_MODIFICATION_ROUTINE( aBoard, aCreationHandler, aModificationHandler ) + PAIRWISE_LINE_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) : + ITEM_MODIFICATION_ROUTINE( aBoard, aHandler ) { } @@ -167,10 +240,8 @@ public: class LINE_FILLET_ROUTINE : public PAIRWISE_LINE_ROUTINE { public: - LINE_FILLET_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler, - MODIFICATION_HANDLER aModificationHandler, int filletRadiusIU ) : - PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler ), - m_filletRadiusIU( filletRadiusIU ) + LINE_FILLET_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler, int filletRadiusIU ) : + PAIRWISE_LINE_ROUTINE( aBoard, aHandler ), m_filletRadiusIU( filletRadiusIU ) { } @@ -190,10 +261,9 @@ private: class LINE_CHAMFER_ROUTINE : public PAIRWISE_LINE_ROUTINE { public: - LINE_CHAMFER_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler, - MODIFICATION_HANDLER aModificationHandler, - CHAMFER_PARAMS aChamferParams ) : - PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler ), + LINE_CHAMFER_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler, + CHAMFER_PARAMS aChamferParams ) : + PAIRWISE_LINE_ROUTINE( aBoard, aHandler ), m_chamferParams( std::move( aChamferParams ) ) { } @@ -214,9 +284,8 @@ private: class LINE_EXTENSION_ROUTINE : public PAIRWISE_LINE_ROUTINE { public: - LINE_EXTENSION_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler, - MODIFICATION_HANDLER aModificationHandler ) : - PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler ) + LINE_EXTENSION_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) : + PAIRWISE_LINE_ROUTINE( aBoard, aHandler ) { }