kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

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
>