On 7/3/20 5:42 PM, Jeff Young wrote:
Hi Reece,
On 3 Jul 2020, at 21:32, Reece R. Pollack <re...@his.com
<mailto:re...@his.com>> 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.
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@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp