← Back to team overview

kicad-developers team mailing list archive

Re: [PATCH] Fix for 3D model offset

 

Hold on a second before you prepare a patch.  This would introduce a new
semantic to the coordinate entries in the file format which could be
open to abuse in the future.  I don't want to open up the possibility of
using different units within the file format itself and adding units
token will open the door for that possibility.  I will also be confusing
to readability because all other units are in mm but do not have the
'mm' token except for 3D model offsets.  I would prefer that we do a one
time conversion to mm and bump the file format version rather than add a
new token to the file format.  The parser change would be fairly
straight forward:

if( file_version < VERSION )
    convert_units_from_decimils_to_mm();

The files will still be readable in previous version of kicad.
Obviously users will have to reset the offsets if they are not zero but
this typically isn't a big problem.

On 11/11/2017 03:57 AM, Oliver Walters wrote:
> JP,
> 
> Yes I will update with a patch. 
> 
> On 11 Nov 2017 19:45, "jp charras" <jp.charras@xxxxxxxxxx
> <mailto:jp.charras@xxxxxxxxxx>> wrote:
> 
>     Le 09/11/2017 à 21:48, Oliver Walters a écrit :
>     > JP,
>     >
>     > I think that 3) is the better option, because it preservers PCB
>     file compatibility (as long as the
>     > offset is zero). Any time the PCB is saved, the (at (xyz 0 0 0))
>     gets written.
>     >
>     > You are also correct that none of the official library footprints
>     have a defined offset. This is why
>     > i have never encountered this before.
> 
>     So I also think 3 is the better option:
> 
>     (at (xyz nn mm ll)) is read as position in inches (old files)
>     (offset(xyz nn mm ll)) is read as position in mm (new files)
> 
>     And the position is written in file only if not 0 (that is the case
>     of our footprint files) to avoid
>     breaking the compatibility with older Pcbnew version when not
>     mandatory, at least for now.
> 
>     Could you prepare a patch?
>     Thanks.
> 
>     >
>     > On Fri, Nov 10, 2017 at 7:09 AM, Oliver Walters
>     <oliver.henry.walters@xxxxxxxxx <mailto:oliver.henry.walters@xxxxxxxxx>
>     > <mailto:oliver.henry.walters@xxxxxxxxx
>     <mailto:oliver.henry.walters@xxxxxxxxx>>> wrote:
>     >
>     >     I like 2) or 3) - I think that a major release is a good time
>     to fix such a bug. 
>     >
>     >     Would you like me to add a patch implementing one of these
>     options JP?
>     >
>     >     On 9 Nov 2017 23:33, "Kristoffer Ödmark"
>     <kristofferodmark90@xxxxxxxxx <mailto:kristofferodmark90@xxxxxxxxx>
>     >     <mailto:kristofferodmark90@xxxxxxxxx
>     <mailto:kristofferodmark90@xxxxxxxxx>>> wrote:
>     >
>     >         Currently the footprints arent compatible anyway i guess,
>     they support more than 4.07
>     >         footprints do. So option 2 is my preferred solution.
>     >
>     >         On 11/09/2017 12:51 PM, jp charras wrote:
>     >
>     >             Le 09/11/2017 à 11:12, Kristoffer Ödmark a écrit :
>     >
>     >                 My 2 cents is that the headaches of storing values
>     in mixed units, without
>     >                 indication of which unit
>     >                 they are stored as is a huge drawback for
>     readability of the saved files and for
>     >                 maintainability.
>     >
>     >                 Also I think the proposed new tag offset is a good
>     idea, since the libraries can be
>     >                 gradually
>     >                 updated then. That the files cannot be opened by
>     previous versions is a minor
>     >                 problem, since the
>     >                 files cannot be opened by kicad 4.07 anyway already.
>     >
>     >
>     >             In fact, footprint files can be opened by 4.07
>     version, as long as they contain no round
>     >             rect or
>     >             custom pads (should be most of files)
>     >
>     >
>     >                 On 11/09/2017 12:55 AM, Wayne Stambaugh wrote:
>     >
>     >                     This requires a file version bump and code
>     that tests for prior versions
>     >                     before converting the units on read.  At that
>     point, the file will no
>     >                     longer be compatible with prior version of
>     KiCad.  I'm not opposed to
>     >                     this but I'm not sure it's worth the headaches
>     it will cause.
>     >
>     >                     On 11/08/2017 03:33 PM, Oliver Walters wrote:
>     >
>     >                         What about a controversial idea:
>     >
>     >                         Read "at" dimensions as inches, but new
>     files write "offset" in mm.
>     >
>     >                         This preserves read compatibility but
>     fixes the units issue going forward.
>     >
>     >             <...>
>     >
>     >             There are 3 different things related to the anchor
>     coordinate 3D shapes in Oliver's patch:
>     >
>     >             1 - coordinate units inside Kicad: they should be in
>     internal units
>     >             (I do not remember if it is currently the case).
>     >             no problem.
>     >
>     >             2 - Display unit name in dialog: good enhancement.
>     >
>     >             3 - how to store this anchor coordinate in .kicad_pcb
>     files.
>     >             There are 2 options:
>     >             opt 1 - use keyword "at" and store it in inches
>     (current case). Certainly annoying
>     >             because all other
>     >             coordinates use mm, but this is not a major issue.
>     >             opt 2 - be able to read "at" (inches) and "offset" (or
>     perhaps "anchor") (in mm) and use
>     >             offset as
>     >             new keyword: but it breaks compatibility with old
>     (namely the recent 4.07) kicad version.
>     >             opt 3 - same as 2, but stores "offset" (or "anchor")
>     *only* if it is not the default
>     >             value (0 0 0).
>     >             AFAIK, the default value is the case of most (perhaps
>     all) footprint files in our
>     >             official repo.
>     >
>     >             in case of option 2, the parser should be (obviously)
>     able to understand the keyword 
>     >             "offset" (or
>     >             "anchor") to prepare the future.
>     >
>     >
>     >         _______________________________________________
>     >         Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     >         <https://launchpad.net/%7Ekicad-developers
>     <https://launchpad.net/%7Ekicad-developers>>
>     >         Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
>     >         Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     >         <https://launchpad.net/%7Ekicad-developers
>     <https://launchpad.net/%7Ekicad-developers>>
>     >         More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
>     <https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>>
>     >
>     >
>     >
>     >
>     > _______________________________________________
>     > Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     > Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     > More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
>     >
> 
> 
>     --
>     Jean-Pierre CHARRAS
> 
>     _______________________________________________
>     Mailing list: https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>     Unsubscribe : https://launchpad.net/~kicad-developers
>     <https://launchpad.net/~kicad-developers>
>     More help   : https://help.launchpad.net/ListHelp
>     <https://help.launchpad.net/ListHelp>
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 


Follow ups

References