MitjaNemec / ReplicateLayout

GNU General Public License v2.0
90 stars 12 forks source link

Duplicate tracks, vias, zones, drawings in v7 #54

Closed atait closed 1 year ago

atait commented 1 year ago

My group has used replicate layout for years, so it was a serious problem when we upgraded to KiCad v7 and lost replication, which is way more valuable to us than anything introduced in the v6 -> v7 upgrade.

Turns out PCB_ITEM.Duplicate is returning a parent class, even if called by a via or track object. But now there are also pcbnew.Cast_to_<insert class> functions. It just needs a cast. I implemented with a try/catch that doesn't break v6.

Addresses #53 #52 #51 #49

Version: 7.0.2-6a45011f42~172~ubuntu22.04.1, release build
Platform: Ubuntu 22.04.2 LTS, 64 bit, Little endian, wxGTK, ubuntu, x11
MitjaNemec commented 1 year ago

First of all thanks for your contribution. It is a PITA that python API changed during 7.0.x release cycle.

As for your PR I'll reject it mainly because I've had bad experiences monkeypatching pcbnew python API. It works if you only have one plugin installed. But if some other plugins also used the same methods that you've monkeypatched then you can get a lot of issue reports from users (and plugin authors) that your monkeypatching breaks. Additionally I think that the solution can be simplified by using pcbnew's Cast function which handles the proper casting for each class type.

I'll try to put some time aside to properly handle this and push a solution soon.

atait commented 1 year ago

Ok, thanks for the quick reply. Very good points. You could go through and change wxPoint to VECTOR2I, but that loses v6 compatibility (which I think is worth keeping indefinitely).

Another approach I've used for kicad-python without monkey patching is to make a class or dict called SWIGtypes where SWIGtypes.Point gives you different pcbnew classes depending on the version it detects. This way, scope is limited.

if SWIG_api >= 7:
    class SWIGtypes:
        Point = pcbnew.VECTOR2I
        Box = ...
else:
    class SWIGtypes:
        Point = pcbnew.wxPoint
        Box = ...

You should decide your preferred approach. LMK if I can help draft that other approach or if you would rather implement yourself.

Thanks for the excellent plugin!

MitjaNemec commented 1 year ago

As it turns out this is due to a bug in KiCad. It got introduce just before the 7.0.2 release with 8e579ec8 and got fixed just after the 7.0.2 was tagged in 7843e6a7.

And the solution for this is to just invoke an additional cast. In 7.0.1 or earlier and 7.0.3 and later the additional Cast() does nothing (as it is already called when invoking Duplicate()). For 7.0.2 it does fix the issue.

Anyhow, the fix for this is in 2.0.12 release, which is available for installation from file and should be available in PCM in a couple of days.

Regarding the V6 compatibility, I've already set up my plugins so that I have separate release for V6 and V7. While the code of each release is cleaner, it is a bit of the PITA to maintain this. But this is the system I have in place and I am going to keep it. If V8 does not implement this functionality natively then I'll think about how to proceed.

MitjaNemec commented 1 year ago

Oh and thanks for the SWIGtypes idea. It should be a nice tool when attacking the V7/V8 compatibility issues