← Back to team overview

kicad-developers team mailing list archive

Re: Patch set: Display Origin Transforms rebase


Sorry about the delay in responding. I've been traveling.

I don't believe overloading the base instance of GetMsgPanelInfo() and GetSelectMenuText() will achieve the desired goal. Overloading functions works great when the /caller/ wants to choose between variants of a function. In this case, though, the call is being made via a pointer to a EDA_ITEM (in EDA_DRAW_FRAME::SetMsgPanel(), currently at common/legacy_gal/eda_draw_frame.cpp:582). The caller has no knowledge of whether the object requires the third parameter or not. Since EDA_DRAW_FRAME is common to all applications, it's an all or none situation.

Realistically, I expect support for origin transforms will eventually be integrated into all of the applications. Pcbnew first, but users are going to expect consistency from Gerbview. The footprint editor would also benefit greatly, and thus cvpcb too. Okay, maybe the PCB Calculator won't support display origin transforms, and it seems the work sheet editor already has a similar concept.

That leaves Eeschema and the symbol editor. The Aux Origin is defined in Eeschema but there's no way to set it and I'm not sure that's the best way to handle the situation anyway. Although my previous patch set didn't tweak GetMsgPanelInfo() and GetSelectMenuText() in Eeschema code to perform display origin transforms, that could have be done easily since they already receive a reference to an ORIGIN_TRANSFORMS object, which is currently NullOriginTransforms. Then whatever scheme was developed for Eeschema would only require that flavor of ORIGIN_TRANSFORMS to be passed in and these would work. Only the Eeschema flavor of UNIT_BINDER and the appropriate changes to the dialogs would then be needed.

Also consider that much of the DRC marker code is shared between Pcbnew and Eeschema. While Eeschema can simply pass a reference to the NullObjectTransforms object to the common DRC code, this means Eeschema can't be completely isolated from these changes.

The alternative is for someone to come up with a scheme by which the editor hierarchy can provide display formatting functions to the structural hierarchy without passing a parameter to these display-oriented functions. I don't think that's likely, though I'm open to suggestions.

Perhaps the display formatting could be bundled into a class, rather than being classless, global functions as they are now. The editors could then pass a reference to this formatting class in place of the current EDA_UNITS parameter. The display formatting class could handle display units (mm vs in) and origin transformations internally. This could also help ease future formatting enhancements. However, this sort of change would be far broader in scope than my current patch set, and I was hoping for my entrance to the KiCad development community to be a bit less dramatic.


On 5/26/19 5:57 PM, Wayne Stambaugh wrote:

I'm fine with this approach.  I like your idea of deriving a new
UNIT_BINDER object so that it doesn't impact the other frames that do



On 5/26/19 4:15 PM, Seth Hillbrand wrote:
Hi All-

If Reece can separate this so that it doesn't affect eeschema, cvpcb,
pagelayout_editor, gerbview or libedit, I'm fine with merging as is and
refactoring more later.  If we merge this as is, it become much more
difficult to fix it later.

Since these are all GetMsgPanelInfo() calls, the fix is simply to
overload the base instance and then any call that doesn't use
ORIGIN_TRANSFORMS should not pass it as a parameter.

Once this change is implemented, the rebasing should not be nearly so
painful, so the extra holds on commits should not be required.


The fix for this is simply overloading

On 2019-05-26 15:54, Jon Evans wrote:
Hi Reece, Wayne, Seth,

Can you clarify the path forward here?  Are we going to merge the
second patchset and then do follow-ons to take care of the issues Seth
raised, or will there be another full patchset coming?
I have a backlog of things to cherry-pick from 5.1 to master that I've
been holding on to until this is resolved.



On Sun, May 26, 2019 at 2:08 PM Reece R. Pollack <reece@xxxxxxx>

So now it occurs to me that what I should have done was create
classes derived from UNIT_BINDER that handle the different types of
data (X-abs, Y-abs, X-rel, Y-rel) and instantiated those, rather
than adding a parameter to the UNIT_BINDER class.

However, that would have also required changing UNIT_BINDER to
accept and return _double_ values rather than _int_ to avoid the
_int_ -> _double_ -> _int_ -> _double_ conversion sequence
formatting for display, and _double_ -> _int_ -> _double_ -> _int_
conversions parsing from display.

Maybe I'll do that another time. Too many changes too late for this
iteration, I think.

On 5/26/19 11:01 AM, Reece Pollack wrote:

The specific problem with UNIT_BINDER is that it doesn't know what
sort of data it's handling. It could be a value like a track width
which shouldn't be altered,  a relative coordinate delta which may
need a sign flip, or an absolute coordinate which needs both
translation and sign flip. Nor does it know whether relative or
absolute coordinate is an X or a Y axis. Thus it must either have
a parameter identifying the type of data it's handling or a
different set of methods to transfer that data in or out based on
the type. I chose a constructor parameter, and I chose to pass an
object implemented to handle that type of data.

Follow ups