kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

On 7/3/20 7:31 PM, Reece R. Pollack wrote:
> On 7/3/20 5:42 PM, Jeff Young wrote:
>> Hi Reece,
>>
>>> On 3 Jul 2020, at 21:32, Reece R. Pollack <reece@xxxxxxx
>>> <mailto:reece@xxxxxxx>> wrote:
>>>
>>> Noting that the PCB_BASE_FRAME class is derived from the
>>> EDA_DRAW_FRAME class, is it acceptable to assume that the
>>> EDA_DRAW_FRAME pointer parameters passed to functions in Pcbnew
>>> classes are actually pointers to a PCB_BASE_FRAME?
>>
>> Yes, but it would be considered bad practice.
> 
> I would argue that it could easily be defined that code in the pcbnew
> directory is dealing with a PCB_BASE_FRAME rather than an
> EDA_DRAW_FRAME. Common code should not make that assumption, of course,
> and would not use anything beyond the base class.

Why not just provide a virtual method in the EDA_DRAW_FRAME and override
the method in the derived frame objects to do implement the frame
specific behavior?  This way you don't have to care about the draw frame
type

> 
>>>
>>> Specifically:
>>>
>>>   * The UNIT_BINDER class constructor
>>>   * The classes implementing the GetMsgPanelInfo function
>>>   * The classes implementing the GetSelectMenuText function
>>>
>>> The reason I'm asking is that to implement origin transforms these
>>> functions need access to the user's display option that chooses the
>>> display origin. This needs access to a function defined in the
>>> PCB_BASE_FRAME class. I can make that a common function defined in
>>> EDU_DRAW_FRAME and overridden in PCB_BASE_FRAME, or the code needs to
>>> know that parameter is really a pointer to a PCB_BASE_FRAME object.
>>>
>> Ask yourself this: is there anything PCB-specific about a settable
>> origin?  Is there any reason a settable origin wouldn’t also be
>> useful, say, a schematic?  If the answer is “no”, then you should push
>> the origin (along with its getters and setters) down in to EDA_DRAW_FRAME.
> 
> My submission last year was intended to allow any or all of the KiCad
> subsystems (eeschema, pcbnew, gerbview, etc.) make use of origin
> transforms. Gerbview could benefit, because if you set the place and
> drill origin to the lower left of you design when you generate Gerbers,
> Gerbview displays the page border below the displayed board. I'd thought
> about how to do this in eeschema, considering an origin at any of the
> four corners or at the aux origin (which is defined in eeschema but is
> not settable). But I did a full implementation on pcbnew because that
> was what I felt really needed it.That  implementation added a parameter
> to the functions I mentioned above, which resulted in the eeschema
> functions also having that parameter even though they didn't use it.
> 
> When Wayne sent out his last call for comments before he merged the
> changes, Seth complained that since I didn't implement support in
> eeschema then these changes should not affect any code in the eeschema
> directory. His suggestion was to use function overloading, but that
> would result in having two interfaces visible in the both eeschema and
> pcbnew, one that worked in that context and one that didn't. You
> wouldn't know that the code called the wrong function until you noticed
> broken behavior. I don't consider that an acceptable situation just to
> avoid a trivial parameter list change.
> 
> My approach this year is that code in the pcbnew directory knows it's
> dealing with a PCB_BASE_FRAME because it's PCB-specific code in the
> first place. Then it's easy to access the PCB-specific interfaces and
> data. Rather than changing the UNIT_BINDER class I've implemented a
> PCB_UNIT_BINDER class derived from UNIT_BINDER. It knows that it's
> getting a PCB_BASE_FRAME rather than an EDA_DRAW_FRAME, and passes that
> to the base class constructor. It's more tricky with the functions that
> are called indirectly through the EDA_DRAW_FRAME, such as
> GetMsgPanelInfo and GetSelectMenuText. The implementations are specific
> to pcbnew and live in the pcbnew directory.
> 
> I can do it with dynamic casts, but I'm an embedded systems guy and I
> hate to waste CPU cycles checking things that are defined to be true.
> And dynamic_cast is slow: my tests of the Icarus Verilog simulator
> appear to show VVP spends about 20% of its CPU cycles just resolving
> dynamic casts.
> 
>> If this really is PCB-specific, then you can either push just the
>> origin getters/setters down into EDA_DRAW_FRAME and override them in
>> PCB_BASE_FRAME, or do a dynamic_cast to PCB_BASE_FRAME and if non-null
>> call the getters/setters.
> 
> I believe there will need to be subsystem-specific implementations of
> portions of the origin transform code, based on what display options we
> offer to the user. However, I think a lot of the interfaces can be
> common. I'm willing to do either a PCB-only version or one that has
> support for pcbnew and latent support for the other subsystems. What I'm
> not willing to do is invest a lot of effort, only for someone to veto it
> at the last minute over a stylistic difference of opinion again. We,
> meaning myself and the core developers collectively, need to come to a
> consensus on how this should be implemented.
> 
> -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
>