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.mciner...@ieee.org> 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 <j...@craftyjon.com> 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 <re...@his.com> 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 <j...@craftyjon.com> 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 <re...@his.com> 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 <re...@his.com> >> > >>>> 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 <re...@his.com> 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@lists.launchpad.net >> > >>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >> > >>>>>>> More help : https://help.launchpad.net/ListHelp >> > >>>>> _______________________________________________ >> > >>>>> Mailing list: https://launchpad.net/~kicad-developers >> > >>>>> Post to : kicad-developers@lists.launchpad.net >> > >>>>> Unsubscribe : https://launchpad.net/~kicad-developers >> > >>>>> More help : https://help.launchpad.net/ListHelp >> > >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : kicad-developers@lists.launchpad.net >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
