kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

I view the package of information need as the arguments to the function. If
information isn't needed, then it shouldn't be passed in. Creating a new
wrapper type that just holds the EDA_UNITS and the ORIGIN_TRANSFORMS object
seems like an extra level of abstraction that is not needed. The point of
my original question about the IU is that if the transform operates on the
IU, then these two items are probably not needed together anywhere else.
Adding a struct solely to hold these two datatypes will add
code/understanding overhead to the functions and its callsite, specifically:
* Having to create the actual struct and populate it's fields before
calling the function
* In the function the struct has to be unpacked/continually referenced (so
the variable names become longer)
* It hides the fact that we are passing these two items (one of which is
already an opaque type) inside another opaque type when someone just views
the prototype for the function

To me, it seems like there is not enough here to justify the creation of a
new layer of abstraction to pass these two items into this function. If
these two datatypes are needed in other places at the same time, then I
could see the justification.

-Ian

On Fri, Jul 10, 2020 at 9:30 PM Jon Evans <jon@xxxxxxxxxxxxx> wrote:

> The way I see it, there is one package of information that we should
> be delivering: how to turn internal units into real text for display.
> That information includes both a units selection and whatever
> transform to apply in case there are coordinates involved.
>
> I see no benefit into splitting that into two arguments.
>
> On Fri, Jul 10, 2020 at 4:27 PM Ian McInerney <Ian.S.McInerney@xxxxxxxx>
> wrote:
> >
> > Doesn't the origin transform operate on the IU and not use the units
> directly (so the units are only needed by the actual menu text generation
> function)? If they are separate, then we should pass them as two individual
> arguments to the GetSelectMenuText function.
> >
> > -Ian
> >
> > On Fri, Jul 10, 2020 at 8:40 PM Jon Evans <jon@xxxxxxxxxxxxx> wrote:
> >>
> >> It would be preferable to make it possible to call GetSelectMenuText
> >> (or really, any facility that needs access to the current units and
> >> coordinate transform) without a frame.
> >>
> >> We have recently been putting a lot of effort into separating the
> >> logic code from the UI / frame code in KiCad, and it would be a shame
> >> to introduce something that ties them together.
> >>
> >> What about bundling together the units selection and the origin
> >> transforms into a single object that can be passed into
> >> GetSelectMenuText instead of the EDA_DRAW_FRAME?
> >>
> >> That would make it easier to write testcases and other standalone
> >> chunks of code that can pass in arbitrary units and transforms without
> >> having to construct a frame.
> >>
> >> -Jon
> >>
> >> On Fri, Jul 10, 2020 at 3:33 PM Reece R. Pollack <reece@xxxxxxx> wrote:
> >> >
> >> > The units selection and the origin transforms are both available
> through
> >> > the EDA_DRAW_FRAME. I haven't looked at every call in detail (there
> are
> >> > a combined 118 calls and function declarations), but it appears that
> >> > when the caller has access to an EDA_DRAW_FRAME then the call is some
> >> > variant of:
> >> >
> >> >       GetSelectMenuText( m_editFrame->GetUserUnits() );
> >> >
> >> > Where there isn't access to an EDA_DRAW_FRAME, it's coded as:
> >> >
> >> >       GetSelectMenuText( EDA_UNITS::MILLIMETRES );
> >> >
> >> > It seems to me that if we're going to change the parameters to this
> >> > function, we should pass a pointer to an EDA_DRAW_FRAME as a single
> >> > parameter. If this parameter is nullptr, then we assume
> >> > EDA_UNITS::MILLIMETRES and null transforms, otherwise we use the
> getter
> >> > functions in EDA_DRAW_FRAME to get the correct object or data.
> >> >
> >> > I'd need to see more than a few nodding heads before I made either of
> >> > these changes.
> >> >
> >> > -Reece
> >> >
> >> > On 7/10/20 3:00 PM, Jeff Young wrote:
> >> > > I agree on both points: units and transform should be saved in the
> project file, and they should be passed to GetSelectMenuText as parameters.
> >> > >
> >> > > Cheers,
> >> > > Jeff.
> >> > >
> >> > >
> >> > >> On 10 Jul 2020, at 19:35, Jon Evans <jon@xxxxxxxxxxxxx> wrote:
> >> > >>
> >> > >> OK, I see.
> >> > >>
> >> > >> I have mostly stayed out of this conversation before so I will
> >> > >> probably step back, but I would suggest that I think that display
> >> > >> units and origin choice should be a project-level setting, not
> >> > >> system-level.
> >> > >>
> >> > >> Probably it makes sense to change GetSelectMenuText so that it has
> >> > >> this context available somehow (whether by passing in an
> >> > >> EDA_DRAW_FRAME or by just passing in the ORIGIN_TRANSFORMS or
> >> > >> whatever)
> >> > >>
> >> > >> -Jon
> >> > >>
> >> > >> On Fri, Jul 10, 2020 at 2:29 PM Reece R. Pollack <reece@xxxxxxx>
> wrote:
> >> > >>> Jon,
> >> > >>>
> >> > >>> The alternate origins themselves (Place & Drill / Aux origin, and
> Grid
> >> > >>> origin) in Pcbnew are stored in the BOARD_DESIGN_SETTINGS class
> and
> >> > >>> saved in the board file. Of course, the Page origin location
> doesn't
> >> > >>> need to be stored; it's always (0,0).
> >> > >>>
> >> > >>> My initial thought last year was to store the user's choice of
> display
> >> > >>> origin in the board file, but after some discussion we reached
> consensus
> >> > >>> that the choice of display origin was more like
> millimeters/inches and
> >> > >>> thus should be a user option rather than a board property. In the
> >> > >>> proposed implementation, the user's preference for which origin
> is used
> >> > >>> for display is stored in the PCB_DISPLAY_OPTIONS class and saved
> in the
> >> > >>> pcbnew.json file. I think a case could be made that this should
> be saved
> >> > >>> per-project, but no one has made a good argument for that.
> >> > >>>
> >> > >>>
> >> > >>> The primary user of the display origin transforms is the
> UNIT_BINDER
> >> > >>> class. This class has a pointer to the EDA_DRAW_FRAME object.
> Since
> >> > >>> UNIT_BINDER is common, I added a virtual function
> >> > >>> "GetOriginTransforms()" to the EDA_DRAW_FRAME class. This function
> >> > >>> returns a reference to an ORIGIN_TRANSFORMS class. The base
> >> > >>> implementation of the ORIGIN_TRANSFORMS class contains functions
> that
> >> > >>> return their coordinate arguments unchanged. This way none of the
> KiCad
> >> > >>> applications see a change in behavior unless they override the
> >> > >>> GetOriginTransforms() function.
> >> > >>>
> >> > >>> In the case of Pcbnew, the EDA_DRAW_FRAME parameter is actually a
> >> > >>> pointer to a PCB_BASE_FRAME object. In this class, the
> >> > >>> GetOriginTransforms() function is overridden and returns a
> reference to
> >> > >>> a PCB_ORIGIN_TRANSFORMS object. This object's functions know how
> to
> >> > >>> access the BOARD_DESIGN_SETTINGS and PCB_DISPLAY_OPTIONS objects
> through
> >> > >>> the PCB_BASE_FRAME object and perform the needed transformations.
> >> > >>>
> >> > >>> This works for everything that can find the EDA_DRAW_FRAME
> object, like
> >> > >>> UNIT_BINDER. The GetMsgPanelInfo functions take an EDA_DRAW_FRAME
> >> > >>> pointer as a parameter, so this isn't a problem. However, the
> >> > >>> GetSelectMenuText() functions take only an EDA_UNITS parameter.
> Thus my
> >> > >>> problem.
> >> > >>>
> >> > >>> -Reece
> >> > >>>
> >> > >>> On 7/10/20 1:25 PM, Jon Evans wrote:
> >> > >>>> Shouldn't the origin be stored in the design data (BOARD /
> SCHEMATIC)
> >> > >>>> rather than the base frame?
> >> > >>>>
> >> > >>>> It seems like all data about objects, including their
> coordinates in
> >> > >>>> the coordinate space defined by the user, should be available
> just by
> >> > >>>> parsing a board or schematic file (which should be independent
> of the
> >> > >>>> EDA_BASE_FRAME)
> >> > >>>>
> >> > >>>> -Jon
> >> > >>>>
> >> > >>>> On Fri, Jul 10, 2020 at 1:18 PM Reece R. Pollack <reece@xxxxxxx>
> wrote:
> >> > >>>>> Jeff,
> >> > >>>>>
> >> > >>>>> Thanks for the follow-up.
> >> > >>>>>
> >> > >>>>> I'm finding some of the GetSelectMenuText() implementations
> format
> >> > >>>>> coordinate addresses for display. That means they need to use
> display
> >> > >>>>> origin transforms so the displayed coordinate matches what the
> user sees
> >> > >>>>> on the status line and elsewhere. However, there does not
> appear to be a
> >> > >>>>> path from these functions to the EDA_BASE_FRAME class where
> these
> >> > >>>>> transforms are currently available.
> >> > >>>>>
> >> > >>>>> Might someone more familiar with the data structures and calling
> >> > >>>>> sequences could suggest how this can best be accomplished?
> >> > >>>>>
> >> > >>>>> Seth, I'd appreciate it if you could bring your knowledge and
> expertise
> >> > >>>>> to bear.
> >> > >>>>>
> >> > >>>>> -Reece
> >> > >>>>>
> >> > >>>>> On 7/10/20 6:35 AM, Jeff Young wrote:
> >> > >>>>>> No, the DRC re-write won’t affect GetSelectMenuText().  It
> originated for describing items in the Clarify Selection menu, but it’s now
> used whenever we want to describe an item to the user.
> >> > >>>>>>
> >> > >>>>>>> On 10 Jul 2020, at 00:51, Reece R. Pollack <reece@xxxxxxx>
> wrote:
> >> > >>>>>>>
> >> > >>>>>>> On 7/9/20 7:09 PM, Tomasz Wlostowski wrote:
> >> > >>>>>>>> On 10/07/2020 00:58, Reece R. Pollack wrote:
> >> > >>>>>>>>> I'm looking at display origin transformations for DRC
> reports.
> >> > >>>>>>>>>
> >> > >>>>>>>>> With the 5.1.x branch Pcbnew, the DRC report dialog box
> message texts
> >> > >>>>>>>>> contained the Cartesian coordinates of each flagged item.
> It appears
> >> > >>>>>>>>> that the 5.99 branch no longer displays the coordinates of
> DRC items.
> >> > >>>>>>>>> However, the Cartesian coordinates are still listed in the
> report file.
> >> > >>>>>>>>> Unlike the display in a dialog box, this report is
> persistent and could
> >> > >>>>>>>>> be passed to someone using different display origin
> settings.
> >> > >>>>>>>>>
> >> > >>>>>>>>>    1. Should these coordinates be reported relative to the
> page origin, or
> >> > >>>>>>>>>       transformed per the user-selected origin and axis
> directions?
> >> > >>>>>>>>>    2. If the coordinates are transformed, should the report
> include the
> >> > >>>>>>>>>       user settings?
> >> > >>>>>>>>>
> >> > >>>>>>>>> -Reece
> >> > >>>>>>>>>
> >> > >>>>>>>> Reece,
> >> > >>>>>>>>
> >> > >>>>>>>> I wouldn't introduce any changes to the current DRC code,
> we're
> >> > >>>>>>>> designing a new DRC engine for KiCad V6 and many things in
> DRC interface
> >> > >>>>>>>> will likely change.
> >> > >>>>>>>>
> >> > >>>>>>>> IMHO the DRC coordinate transform belongs to the UI, not the
> DRC itself:
> >> > >>>>>>>> - the DRC engine generates an internal report with
> coordinates in board
> >> > >>>>>>>> coordinate space
> >> > >>>>>>>> - whatever displays the report to the user (i.e. the DRC
> dialog)
> >> > >>>>>>>> converts the board coordinates to UI coordinates.
> >> > >>>>>>>>
> >> > >>>>>>>> - my 5 cents
> >> > >>>>>>>> Tom
> >> > >>>>>>>>
> >> > >>>>>>>>
> >> > >>>>>>> Tom,
> >> > >>>>>>>
> >> > >>>>>>> I'm not hard to convince, especially when it means doing less
> work.
> >> > >>>>>>>
> >> > >>>>>>> This sounds like the RC_ITEM::ShowCoord function will be
> removed or replaced. It's one of the two troublesome function groups.
> >> > >>>>>>>
> >> > >>>>>>> The other troublesome function group is the GetSelectMenuText
> function. Last year I knew what they did but I've forgotten in the interim.
> Will this DRC rewrite replace these?
> >> > >>>>>>>
> >> > >>>>>>> -Reece
> >> > >>>>>>>
> >> > >>>>>>> _______________________________________________
> >> > >>>>>>> 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
> >> > >>>>> _______________________________________________
> >> > >>>>> 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
> >> >
> >>
> >> _______________________________________________
> >> 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
>