Added conversion of legacy fp_arc to modern fp_arc

[zardini123]

This PR address another half of the issue mentioned by issue #60. KiUtils currently parses (module files without question. Though, (module files can have the legacy syntax for fp_arc, which instead has tokens start, end, and angle. KiUtils does not fail when parsing fp_arcs with this old syntax as is in an incomplete state. Instead of having KiUtils fail, I wrote these changes based off of KiCad source code (sources included in code) to upgrade that old syntax to modern fp_arc. This PR then works towards fully supporting (module related syntax.

KiCad's file format page does not mention the legacy formatting of fp_arc (docs). Instead, I found the description in a 3rd party documentation from 2015 when footprint libraries started with (module (legacy footprint documentation).

As mentioned in the changes, the current approach assumes both legacy and modern fp_arcs are possible at the same time. KiCad switches parsing based on version number as mentioned in #60. Though, the only objects able to access version numbers are the container footprint/symbols. Therefore until the KiUtils API supports children objects having a version number, the hack there exists.

Especially for the older .kicad_mod files I am working with, this change helps tremendously in preserving that information.

[mvnmgrx]

I remember running in such an issue too when migrating from KiCad 5 to KiCad 6 and using kiutils. See issue #1.

I guess supporting both versions determined by their tokens makes the most sense. The easiest fix would be to convert to the new format, as your PR suggests. No version token would be needed then. But this implicitly converts a KiCad 5 footprint to a KiCad 6+ footprint.

Another approach would be to have an additional class member of some sort (e.g. legacyFormat: bool = False) that is set during parsing and generates the respective code in to_sexpr().

But as i think that it only complicates things when we are fully supporting KiCad 5 style of footprints, i would rather see the converting approach. But i am happy to hear your thoughts about this.

the current approach assumes both legacy and modern fp_arcs are possible at the same time

I dont think that there are footprints that have legacy fp_arc and new fp_arc s-expression in it.

As in your issue #60:

In the source of KiCad I found that they consider both (modules and (footprint as .kicad_mod files as modern due to first versions of .kicad_mod being (modules

Aren't footprints > LEGACY_ARC_FORMATTING called (footprint ....) and older versions (module ...) in s-expression? If so, this would suffice in differentiating between the two formats.

As of right now, i think this is the only thing that really holds back parsing of KiCad 5 style footprints with kiutils.

We'd need some unittests for this as well. Do you want to provide some? E.g. A minimal example of an older style (module ...) footprint that explicitly tests this fp_arc behavior.

[zardini123]

Another approach would be to have an additional class member of some sort (e.g. legacyFormat: bool = False) that is set during parsing and generates the respective code in to_sexpr().

I think in terms of API there still is more pressing issues about how they decided to name start and end tokens in the legacy format. start is the center of the circle and end is the starting point on the arc. It's really confusing and preserving that confusion I think is a problem. I believe adding legacyFormat would assign two meanings to the same variables.

If there would be an API change, I think having a KiUtil-only class, possibly called RadialArc and ThreePointArc that are related via inheritance. Therefore the arc definition could be swapped out, having meaningful variable names between the two classes. Both classes could have their own to_expr, but probably fp_arc would be the one instantiating it from an from_expr. Of course this breaks existing API but this won't propagate the confusion KiCad developers imposed upon their decision in switch.

I dont think that there are footprints that have legacy fp_arc and new fp_arc s-expression in it.

This is true, but in terms of code that is how it would function. If a file had both fp_arc definitions in it, it would parse fine under this PR. Instead, it should probably complain to the user. But, I think complaining could simply be done via version number instead of tracking what was parsed.

Aren't footprints > LEGACY_ARC_FORMATTING called (footprint ....) and older versions (module ...) in s-expression? If so, this would suffice in differentiating between the two formats.

From what I understand, not necessarily. In KiCad source, if either (footprint or (module is found in the beginning, it is parsed under the modern s-expr format (file type check). Even when in the parsing code, module is synonymous for footprint (see here and here). Therefore only differentiator is the version.

I think KiUtils may benefit from a Context Object which stores version and generator and propagated to the children classes. This could help keep file formats consistent to a version, allow for version based checks, and also help users with converting between versions by just changing the context object. Curious to hear your thoughts on this.

[zardini123]

We'd need some unittests for this as well. Do you want to provide some? E.g. A minimal example of an older style (module ...) footprint that explicitly tests this fp_arc behavior.

As of currently I am low on time and energy to build a thorough test case, hence my FIXME in the code. I am actively using this branch in my project if that means anything 😁

[mvnmgrx]

Going the inheritance route may be our the best bet here. I think of adding some base class (maybe FpArcBase or somewhat) that has a classmethod for creating the correct fp_arc (as you suggested of type RadialArc or ThreePointArc) depending on the tokens found in the given S-Expression. I guess this would break the API the least bit. Or we keep the name of the v6+ -style FpArc as is and rename the old version to FpArcLegacy or something like that.

As of currently I am low on time and energy to build a thorough test case

Absolutely no problem, i'm going to add this to the to-do list for now.

I think KiUtils may benefit from a Context Object which stores version and generator and propagated to the children classes.

I'm currently reviewing the changes KiCad 7 brings and they added some breaking changes here and there, some of which i am not sure how to implement yet without breaking v6 support. So this may be an approach we have to implement to correctly support both new and older versions of KiCad without breaking kiutils every new release for older versions. But i am not sure yet ..

[zardini123]

Or we keep the name of the v6+ -style FpArc as is and rename the old version to FpArcLegacy or something like that.

Ah, my original idea was having the three parameters defining the geometry of the arc (start mid and end for modern and start end and angle for legacy) be encapsulated into separate classes derived from a single base class. Therefore FpArc could have instantiated as a member for either geometry versions that doesn't require multiple classes of FpArc.

In context of the entire KiUtils project, I do wonder the best approach to structuring classes for version-dependent classes. For example if FpArc changed again in v8+ (which totally sounds like something that could happen) we'd have three separate FpArc classes for each version. That's where I think the API-breaking change of having variant classes for members that change between versions can not bloat the API too hard, but also ensuring users understand via the API that a FpArc is still a "arc" between all the versions, but how it is described (RadialArc vs ThreePointArc) is changeable. Maybe more research into version-dependent recursive descent parsers would be required to figure out a best practice for this?

So this may be an approach we have to implement to correctly support both new and older versions of KiCad without breaking kiutils every new release for older versions.

That is what I believe too. When looking through KiCad's source for their parser, it is amazing how many conditionals there are based on version. I think breaking the API once to ensure cross-version support like KiCad would be best for the future to come.

[mvnmgrx]

I do wonder the best approach to structuring classes for version-dependent classes.

I guess we need some kind of base class that every kiutils class is derived from or some module that keeps track of what context is being parsed. But i don't have concept for this yet, only some ideas i still have to test. But your idea with the context object from above may come in handy for the next versions of KiCad.

But for the sake of now supporting both KiCad 5 and 6+7 it would be sufficient to implement your approach with the base class FpArc and both specializations RadialArc and ThreePointArc. The classmethod FpArc.from_sexpr() then return either of both objects, depending on the information in the given S-Expression, as you stated above.