← Back to team overview

kicad-developers team mailing list archive

Re: Patch set: Display Origin Transforms

 

Wayne, 

Thanks for the feedback. 

Re the header guards: It's common in my experience to define include/exclude type symbols with a non-zero numeric value. This avoids problems when someone writes "#if SYMBOL" rather than the intended "#ifdef SYMBOL", as a preprocessor symbol with no value evaluates to zero in this context. The majority, though certainly not all, of the include files in /usr/include follow this convention for this very reason. 

I'll get you a patch to address the "const" parameters and your other comments as soon as I get a chance. A patch for "Move Exactly" and Bezier will follow in a few days. 

-Reece 


From: "Wayne Stambaugh" <stambaughw@xxxxxxxxx> 
To: "KiCad Developers" <kicad-developers@xxxxxxxxxxxxxxxxxxx> 
Sent: Tuesday, May 21, 2019 10:21:10 AM 
Subject: Re: [Kicad-developers] Patch set: Display Origin Transforms 

Hey Reece, 

I tested your patch set and everything seems to work as advertised. I 
have a few minor comments: 

The ORIGIN_TRANSFORMS references should be passed as const in all places 
where they are used internally by another object that doesn't modify them. 

It's not necessary to add '1' to the header compile guards. AFAIK, none 
of the other header files do this. 

I'm fine if these fixes are implemented in a separate patch. 

I'm fine with merging this patch set as long as there are no objections. 

I answered some of your questions in-line below. 

On 5/19/19 10:13 PM, Reece R. Pollack wrote: 
> I've attached a Zip file containing 11 patches. These implement the 
> Origin Transforms feature I've been talking about since KiCon. They 
> should apply cleanly to the master branch at this commit (currently HEAD): 
> 
> 9d56102 Prevent unannotated components from driving connectivity 
> 
> In summary, this adds Pcbnew user preferences that allow the user to 
> select the origin from which absolute coordinates are displayed and 
> entered. The supported origins are the Page Origin, the Auxiliary 
> Origin, and the Grid Origin. If the user preference has not been set the 
> default is Page Origin, which looks just like what we have now. 
> 
> Additionally, two other Pcbnew user preferences are added to allow the 
> user to select which way the X and Y axes increase: Left or Right for 
> the X axis, and Up or Down for the Y axis. If the user preference has 
> not been set the default is X Right and Y Down, which again looks just 
> like what we have now. 
> 
> I added a new panel to the Pcbnew "Preferences" dialog called "Origins & 
> Axes" to allow the user to change these options. I did not add any 
> toolbar icons as I expect these will be "set and forget" options for 
> most users. 
> 
> These patches do not alter the content of the board file, nor do they 
> change the internal representation of coordinates. The user can change 
> preferences without causing revision-managed data churn. The only affect 
> is how the user sees and enters coordinate values. 
> 
> My intent has been to implement these transforms only in Pcbnew, but the 
> changes to common data structures necessarily affect all KiCad 
> applications. Thus support for display origin and axis shifts is latent 
> in the Footprint Editor, GerbView, Eeschema, and the Symbol Editor, and 
> can be implemented with minimal effort. However, at this point there 
> should be no user-visible changes in any of these applications. 
> 
> Some notes: 
> 
> 1. The new file "origin_transform.cpp" is currently in common/widgets/ 
> because that's where unit_binder.cpp was located. It might ought to 
> be in common/ instead. 

Should be in common. It's not really a widget object. 

> 2. I believe I've addressed all user-visible Pcbnew displays and dialog 
> boxes other than the Move Exactly dialog. If I missed something 
> else, let me know. 
> 3. I haven't decided how the "Move Exactly" dialog should work yet; I 
> think it needs axis orientation support but not origin translation. 
> I'd be happy to get feedback before I code a patch for this. 

It probably should be implemented in the move exactly dialog for 
absolution positions. 

> 4. I did not touch the Bezier coordinates because it appears this is 
> not fully implemented in Pcbnew and I couldn't figure out how I 
> would test such changes. 

I thought we did go live with this recently so Bezier coordinates will 
need to be supported unless this was only in the footprint editor. 

> 5. I'm willing to make a pass through the code to unify the name of the 
> Auxiliary Origin once there is a consensus on what to call it. 

By auxiliary origin, I'm assuming you mean the place and drill origin in 
pcbnew. If so, the latter is more descriptive. Auxiliary origin is not 
very descriptive. 

> 6. Patching the file containing the list of developers to add my name 
> felt kinda presumptuous. I'd be happy if these patches constitutes 
> cause to do so. 

I will do that once your patches are merged. 

> 7. Would someone send Jeff Young on holiday for a week or two? I'm 
> getting burned out just trying to keep these patches rebased on his 
> changes. :-) 

Jeff, you want to field this question? 

> 
> 
> -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 

References