Discussion: PCB_GROUP items in 5.99 and 6.0

[EeliK]

I'll detach discussion about PCB_GROUP items here because this isn't a bug or feature request for specific feature, but I want to give some details which may make decisions and feature plans easier. I just introduce pcbnew groups and possible use cases for especially replicate layout plugin.

The class itself is pretty simple, it has a set of items and ways to handle them as one whole (select, copy, rotate etc.). It's here for reference.

/**
 * A set of BOARD_ITEMs (i.e., without duplicates).
 *
 * The group parent is always board, not logical parent group. The group is transparent
 * container - e.g., its position is derived from the position of its members.  A selection
 * containing a group implicitly contains its members. However other operations on sets of
 * items, like committing, updating the view, etc the set is explicit.
 */
class PCB_GROUP : public BOARD_ITEM
{
public:
    PCB_GROUP( BOARD_ITEM* aParent );

    static inline bool ClassOf( const EDA_ITEM* aItem )
    {
        return aItem && PCB_GROUP_T == aItem->Type();
    }

    wxString GetClass() const override
    {
        return wxT( "PCB_GROUP" );
    }

    wxString GetName() const { return m_name; }
    void SetName( wxString aName ) { m_name = aName; }

    std::unordered_set<BOARD_ITEM*>& GetItems()
    {
        return m_items;
    }

    const std::unordered_set<BOARD_ITEM*>& GetItems() const
    {
        return m_items;
    }

    /**
     * Add item to group. Does not take ownership of item.
     *
     * @return true if item was added (false if item belongs to a different group).
     */
    bool AddItem( BOARD_ITEM* aItem );

    /**
     * Remove item from group.
     *
     * @return true if item was removed (false if item was not in the group).
     */
    bool RemoveItem( BOARD_ITEM* aItem );

    void RemoveAll();

    /*
     * Search for highest level group containing item.
     *
     * @param aScope restricts the search to groups within the group scope.
     * @param aFootprintEditor true if we should stop promoting at the footprint level
     * @return group containing item, if it exists, otherwise, NULL
     */
    static PCB_GROUP* TopLevelGroup( BOARD_ITEM* aItem, PCB_GROUP* aScope, bool aFootprintEditor );

    static bool WithinScope( BOARD_ITEM* item, PCB_GROUP* scope );

#if defined( DEBUG )
    void Show( int nestLevel, std::ostream& os ) const override
    {
        ShowDummy( os );
    }
#endif

    ///< @copydoc EDA_ITEM::GetPosition
    wxPoint GetPosition() const override;

    ///< @copydoc EDA_ITEM::SetPosition
    void SetPosition( const wxPoint& aNewpos ) override;

    ///< @copydoc BOARD_ITEM::GetLayerSet
    LSET GetLayerSet() const override;

    ///< @copydoc BOARD_ITEM::SetLayer
    void SetLayer( PCB_LAYER_ID aLayer ) override
    {
        wxFAIL_MSG( "groups don't support layer SetLayer" );
    }

    ///< @copydoc EDA_ITEM::Clone
    EDA_ITEM* Clone() const override;

    /*
     * Clone() this and all descendants
     */
    PCB_GROUP* DeepClone() const;

    /*
     * Duplicate() this and all descendants
     */
    PCB_GROUP* DeepDuplicate() const;

    ///< @copydoc BOARD_ITEM::SwapData
    void SwapData( BOARD_ITEM* aImage ) override;

    ///< @copydoc BOARD_ITEM::IsOnLayer
    bool IsOnLayer( PCB_LAYER_ID aLayer ) const override;

    ///< @copydoc EDA_ITEM::HitTest
    bool HitTest( const wxPoint& aPosition, int aAccuracy = 0 ) const override;

    ///< @copydoc EDA_ITEM::HitTest
    bool HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy = 0 ) const override;

    ///< @copydoc EDA_ITEM::GetBoundingBox
    const EDA_RECT GetBoundingBox() const override;

    ///< @copydoc EDA_ITEM::Visit
    SEARCH_RESULT Visit( INSPECTOR aInspector, void* aTestData,
                         const KICAD_T aScanTypes[] ) override;

    ///< @copydoc VIEW_ITEM::ViewGetLayers
    void ViewGetLayers( int aLayers[], int& aCount ) const override;

    ///< @copydoc VIEW_ITEM::ViewGetLOD
    double ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const override;

    ///< @copydoc BOARD_ITEM::Move
    void Move( const wxPoint& aMoveVector ) override;

    ///< @copydoc BOARD_ITEM::Rotate
    void Rotate( const wxPoint& aRotCentre, double aAngle ) override;

    ///< @copydoc BOARD_ITEM::Flip
    void Flip( const wxPoint& aCentre, bool aFlipLeftRight ) override;

    ///< @copydoc EDA_ITEM::GetSelectMenuText
    wxString GetSelectMenuText( EDA_UNITS aUnits ) const override;

    ///< @copydoc EDA_ITEM::GetMenuImage
    BITMAP_DEF GetMenuImage() const override;

    ///< @copydoc EDA_ITEM::GetMsgPanelInfo
    void GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList ) override;

    /**
     * Invoke a function on all members of the group.
     *
     * @note This function should not add or remove items to the group.
     *
     * @param aFunction is the function to be invoked.
     */
    void RunOnChildren( const std::function<void ( BOARD_ITEM* )>& aFunction ) const;

    /**
     * Invoke a function on all descendants of the group.
     *
     * @note This function should not add or remove items to the group or descendant groups.
     * @param aFunction is the function to be invoked.
     */
    void RunOnDescendants( const std::function<void( BOARD_ITEM* )>& aFunction ) const;

private:
    std::unordered_set<BOARD_ITEM*> m_items;     // Members of the group
    wxString                        m_name;      // Optional group name
};

---

Groups can be nested, but otherwise one item can belong only to one group. Groups can be named -- this is important because it can be utilized by plugins.

---

https://user-images.githubusercontent.com/23579950/110312147-64dbf900-800d-11eb-95dd-ceb9108fc577.mp4

---

It would be relatively easy to decide how to utilize groups for certain plugin, but the user may want to use groups for other purposes for these items, so it's not that simple. For example several non-related groups can intersect a "channel" layout area when it's replicated. The user must know how to use groups for the plugin and the plugin must know how the user uses them. For example, here are some possible situations.

---

There are several possible strategies:

• Ignore groups altogether (the current behavior).
• Use groups to select footprints to be affected, i.e. some footprints of a hierarchical sheet can be left out.
• Use groups to select extra items to be replicated.
• Use groups to select all items to be replicated, i.e. footprints and extra items.

There are several details to notice:

• Replicating both groups and their items should be avoided because the items would be duplicated.
• If groups are ignored in implementation groups aren't replicated but items are (the current behavior). So the original can have a group but the corresponding items aren't grouped in the replicated targets.
• The items in a user created group can be spread far away.
• There can be several groups in the bounding box of the footprints. Only some of them can be meant for replication.
• It's possible to ignore the bounding box altogether if group is used for extra items.
• A named group can be used: for example, add possibility in the UI to filter replicated groups by regular expression. The name could be for example "replicate_U1" where U1 would be the anchor fooptrint. The group would then be replicated as "replicate_U2" etc. That way each hierarchical sheet would have its own named group which then could be deleted instead of trying to find and delete previously replicated extra items.

The most straightforward strategy would be probably to use one group for the original layout, and all the items which are to be replicated should belong to that group. Then:

• Positions of all footprints belonging to the group would be replicated, but not other footprints. (Footprints not belonging to the hierarchical sheet should be ignored or the attempt should be rejected.)
• Extra items belonging to that group would be replicated.
• In the targets the replicated items should be grouped again.
• If the plugin is used several times, there are already groups in the targets. Then the extra items from the groups should be first deleted from the targets (but not the footprints).
• What to do if the user has used groups for other purposes? For example, in one target one footrprint is already group for some other purpose. Groups are exclusive. In that case the old grouping must be removed or grouping can't be used.
• What to do if the user changes the grouping, for example chooses different subset of the sheet's footprint to be replicated? Must the old groupings be deleted first?
• There may be other difficult situations, or at least things to be considered, for example the user ungroups items, adds some of them into new subgroups, then groups the items again so that the original grouping is still there but some items are in the group only indirectly... Or the user changes the grouping in one of the targets... Or selects another sheet/"channel" as the original...

As can be seen, grouping can make things easier but also more difficult. Even if groups aren't used for the plugin they must be dealt with if the user uses them. And they can be used in some implicit simple ways, or several options can be added to the UI.

[MitjaNemec]

Thanks for the notice and your view on this.

Currently I am thinking that initial port of the plugin for V6 will completely disregard the groups. Later I might add them as a selection mechanism for disregarding certain footprints from the hierarchical sheet. And only then I might also create corresponding groups in destination sheets

I don't see how using groups as a base for replication would work though. Basing the replication upon the schematic sheets the KiCad backend offers a way to identify which footprints from one(source) sheet corespond to shemtatically "same" footprints from other(destination) sheet. I don't see how plugin could identify which footprint in destination group corresponds to specific footprint in source group. It might be possible, but then the plugin would need to do a lot of heavy lifting (matching partial netlists) and this is not really what I am looking forward doing.

[EeliK]

As far as I can see hierarchical sheets must still be used as the base mechanism. Limiting the used footprints (leaving some footprints of the hierarchical sheet out from replication) could work using groups if the reference designators are in the same order, which I assume to be true if the sheets have been automatically annotated. For example in the orignal sheet there are R1, R2, R3. R1 and R3 would be grouped, R2 left out. In the target sheet there are R17...R19. It can be deducted that R17 and R19 belong together and R18 is left out.

Maybe I have time and can play with groups to try out some possibilities.

[MitjaNemec]

The order of reference designators does not matter. The plugin does not derive any information from the reference designators. It uses KiCad's footprint/symbol UUID (used to be timestamp) and sheet UUID(timestamp) path.

[EeliK]

Yes, I remembered after posting that I have already solved the problem. :) The compoent's own ID is the same in all sheets (uniqueness comes from the combination of the sheet hierarchy IDs plust the component ID which together form the "path"), so there's no need to compare refdes.

Anyway, I think you can now understand what I mean: the user can group together some footprints inside one hierarchical sheet, and some are left out from the group. The positions of those in the group can be replicated and the rest can be left intact.

[MitjaNemec]

Yup, I've got what you've meant. I thought I conveyed this clearly when describing adding the "selection mechanism". But obviously I didn't. Man is language (especially technical) difficult thing.

[sslupsky]

To add some additional thoughts, I think grouping in pcbnew needs to be fixed which would trigger a number of other follow on changes. First, a component should be allowed to have membership in multiple groups. Second, grouping should include the schematic and carry over to the pcb and go both ways. Third, replicating a layout should be able to operate on groups or hierarchical sheets (I think when groups are fixed, replicating based on groups will be the more productive choice). Fourth, editing component properties needs to be fixed so you can select multiple components and add, edit or delete a field. Fifth, the symbol fields editor in the schematic editor should allow to group components by group or hierarchical sheet.

[EeliK]

This is not a place to discuss such far reaching feature wishes, especially because they are about KiCad, not the plugin. We are discussing about possibilities the grouping feature as implemented by KiCad could give for the plugin.

Even in the KiCad issue database you shouldn't group several wishes in one issue. It's impossible to handle an issue, for example categorize or close it properly, and discussion gets messy.

[sslupsky]

@EeliK My comment was made with the idea to stimulate a conversation on what is really required in KiCad to facilitate a better implementation of the action plugins. I find the plugins difficult to use and I think some of these issues are a result of the limitations that KiCad imposes. A coordinated discussion of possible changes to KiCad that would assist this effort could lead to submitting an issue that gets some attention on the KiCad gitlab repo.

[MitjaNemec]

@sslupsky I don't mind the discussion, but this kind of discussion is better to have somewhere else. Even if it looks like, this is not a common internet forum. But I'll bit it this time. From the perspective of one who develops the plugin and tries to support the users as much as I can, expanding the plugin to support replication based upon groups is a no go for me. There are a number of reasons why I feel this way:

1. Architecturally this basically means significantly rewriting the plugin with not as much reuse of the existing code as one would hope to
2. I'd need to have a support for this from the KiCad API and depending the better solution we'd want the more support we'd need as you've already alluded to.
3. Hierarchy based replication solves one issue really really well. It guarantees that each channel is the same footprint wise and connectivity wise. With group based replication it would be up to users to group items properly. And I am afraid that this would cause a lot of frustration when the user would forget something and the plugin would crash. This would cause frustration to me, as I'd need to add code to gracefully handle user errors.
4. I'd have to seriously reconsider the UI. Such a change would enable different workflow, and UI should follow this. Even now, I am pretty certain that a lot could be done to the UI.

@EeliK, as for the groups and V6 I've managed to port the plugin and first release is available at https://github.com/MitjaNemec/ReplicateLayout. I've added support for replication on the flip side, and I've been quite shy when adding support for groups. If source anchor footprint belongs to group, the plugin will replicate only footprints belonging to this group and connecting copper. Currently I've still got to solve a MacOs issue, before it is accepted in PCM, I am sure that I can expand on this, but I'll tread carefully. I'd appreciate the ideas though. But please report them to new repo, as I'll archive this one.

[EeliK]

I'll take some time to test this.

f source anchor footprint belongs to group, the plugin will replicate only footprints belonging to this group and connecting copper.

How about something like this...

The plugin could take everything from the original group and create identical groups for all replicates. There would be at least three benefits:

1. Some footprints from the hierarchical sheet can be left out. (If I understood correctly, this is already implemented.)
2. Other items than footprints could always be recognized, deleted and replicated easily. Replication process could just always delete the copper and graphic items from the replicates and recreate them. There would be no need to guess what belongs to the replicate area and what not,.
3. Each replicate would stay untouched to avoid accidental changes and it would be easy to move the replicates around even after replication. The user can enter the group to make changes afterwards if needed in some replicate.

The KiCad developers (especially Seth) let us understand somewhere that this new grouping feature would be a prerequisite for a native "room" (i.e. layout replication) feature. Maybe they had something like this in mind.

EDIT: sorry, I forgot this should have gone to the new repo.... :)

[MitjaNemec]

Yeah, replicating only items in the group (tracks, zones, ...) seems more logical way. But I am hesitant regarding creating a group in the destination. How should I name the groups? While I see the sense of doing this, especially if one want to rerun the plugin so that only previously replicated items are deleted. But as it stands now, an item can be a member of only one group. So what should the plugin do, if an item is already in a group even before the plugin was run? Currently the only was to handle this gracefully is to check before replicating if any of the destination items are already in the group and then prevent the plugin to run.

[EeliK]

Certainly there are difficulties and corner cases.

I'm not sure if being in another group is a big problem. Yes, it has to be checked and refused (but maybe only after choosing which sheets to replicate, or disable those sheets which have grouped items?) which means more work for the one who implements this. But for the end user it probably isn't a problem because I believe that when the hierarchical sheet items + extra items are wanted to be replicated, the user doesn't want to and doesn't need to add them to other groups. At least in v6 grouping is useful mostly for tying item positions together, and it would hardly be logical for the user to bind to-be-replicated items to some other items.

BTW, this grouping problem may partly exist already: if the user has tied some items which are meant to be moved by replication to some group, what to do? If the user grouped the items for keeping their relative positions, the plugin should move all grouped items, too, or the group looses its meaning. I think it's better to prevent using sheets which have items in some other groups. The user then has to decide what they want and act accordingly.

Is naming really a problem? The teardrop plugin (by niluje) at least wanted to use zone names and some developer explicitly said that zone names were added partly for that kind of purpose. Even the native teardrop implementation in 6.99 uses that at the moment; the generated names are visible in the teardrop zones.

About the benefit number 3 above: the replicates could be easily moved around and placed after replication. Now they need to be positioned carefully using anchor footprints beforehand, or all items of a replicate must be selected for moving, or the anchor footprint must be moved and replication process repeated.

I see great benefits for the end user. Whether it's easy to implement is another thing. But I might be interested enough to play with the code.

[MitjaNemec]

First of all, I really appreciate this discussions. It helps me designing the plugin.

Yeah I can see that groups will be an useful add-on to the plugin, and I'll be able to get rid of the 'delete items' option, which is quite brittle. But as you've alluded this will require additional error checking. Based on my experience, this is usually a pain point to implement as it breaks the flow of the code and makes it harder to reason about the code, which in turn results in more bugs.

As for the naming it is not a problem per-se, but I'd rather see that items have a properties hidden from the user, that only plugin can manipulate. Groups are a KiCad feature that can be used in this case, but they can also be used for some other cases. For example, using groups it would be quite awkward for the user to replicate nested hierarchical design (first replicate nested channels, and then replicate top channels - you can look at the design in the repo to get a better idea). And this can quickly come into issues with other plugins which might also use groups (Save/restore layout is one such plugin which might also use groups). So at the moment KiCad's group feature is not really tempting for me to depend too heavily on it. Additional properties which could be set for each item would really help a lot when developing additional features. Do you recall is something like this already requested?

So at the moment the possible paths forward look like:

1. I can expand the grouping feature in the current plugin, to tracks, zones and other items, but not grouping the destination layout. This is most likely what I'll implement.
2. Group also destination layout after the plugin has run. This would make it easier to re-run the plugin, and I could get rid (or at least simplify) connectivity part of the plugin, and removal of previously replicated items. But the plugin will not be able to handle nested hierarchy as easily as it does now

Additionally from my perspective, I am developing different plugins, but I'd like to keep common code base as much as I can. And this might also be a reason why I don't delve to deep with the groups feature

[EeliK]

(first replicate nested channels, and then replicate top channels

I'll have to look at some examples. If the layouts are nested, I don't know if there's any problem - groups can be nested. For example [A, [B, C]] is possible but not [A, B] [B, C].

Additional properties which could be set for each item would really help a lot when developing additional features. Do you recall is something like this already requested?

It's in the wishlist, and actually the milestone is set to 7 at the moment.

[sslupsky]

I would hate to see you put a lot of effort into the groups at this stage of their development. They really are only half baked in v6 at the moment. I tried to use them for a project last year and ended up deleting all the groups.

[MitjaNemec]

@sslupsky, yeah the UI regarding groups has room for improvement. Bud incomplete group UI is not really an issue for a plugin, which mostly deals with groups programmatically.

@EeliK , thanks for the info on nesting groups. I did not know this.

[EeliK]

@sslupsky,

Assuming that the plugin would be changed (or a new plugin based on this created or whatever) and it would use groups, what would actually be the downsides for you? Please describe the problems in the workflow, how would it prevent you doing something you want to do? What you would want to do which you couldn't do?

The workflow would be like this:

1. Click a symbol of a hierarchical sheet.
2. Go to PCB and see the footprint highlighted.
3. Right click the footprint, choose Select -> Items in Same Hierarchical Sheet.
4. Move the footprints to free space.
5. Position the footprints.
6. Draw tracks, zones and extra items (graphics, text).
7. Select the items to be replicated (footprints, copper, extra items). Some footprints may be left out.
8. Right click -> Grouping -> Group; select the new group.
9. Open the Replicate plugin.
10. Choose sheets to be replicated.
11. No need to choose anything else; grouping takes care of footprints and other items.
12. Run with OK.

This was the first run. The result is that you have one group for each replicated sheet. Now you can move the groups, enter a group and make changes inside that group, ungroup, group again.

For the second run you can make changes to positioning and items. Again, grouping takes care of deleting, moving etc.

Possible: pitfall: What happens when the user ungroups some groups and recreates different groups differently, so that they aren't identical? Usually the user modifies only one existing group and uses that as the reference group, but it's not guaranteed