kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

Lorenzo,

I am going to try again.  I am spending my time here, and if this results in another
argument I feel you will have lost me forever.

You have to decide whether you want to be stubborn, or actually part of team, able to see
other people's points of view.  Stubborn (and right in your own mind) may leave you on the
outside just looking like a PITA.

Also, recognize that if I accept 19 out of your 20 propositions, I am not "hard to deal
with".  Focus on the 19, and give me credit for those.  *This is an important point.*
If I yield on 19, you don't get to put your foot down on one.




On 04/30/2013 10:21 AM, Lorenzo Marcantonio wrote:
> Still working on it, however this is what I've done till now; I'd like
> to have opinions i.e. if I'm on the right track.
> 
> Basic ideas:
> 
> - Angles are being migrated to double unless in eeschema
> 
> - UI is still integer based (as discussed)
> 
> - The current angle unit is the decidegrees (but I've read in some
>   comment that there are plans to change this to degrees). The 'ideal'
>   units would be of course radians but then detecting the common
>   90/180/270 degrees rotation would be difficult (if not impossible).
>   Anyway it should be reasonably easy to find later the spots where this
>   10 factor is used.
> 
> - When going from double to int (geometrically) a KiROUND should be used
>   instead of a int cast

Yes, as soon as you enable arbitrary degrees.  (We've not done that to this point.)

> 
> - Rounding should be done as early as possible: 
>   i.e. intvar + KiROUND(fvar) is better than KiROUND(intvar + fvar)
> 
>   Rationale: in this way a fixed point sum is done (even though
>   theorically there should be no loss of precision)
> 
> - Obviously nothing should break :D

That is a context specific issue, I am reluctant to say yes to that in a general sense.
Actually fvar is really a double, so adding an integer to it will be fine.  The integer
gets promoted to double (at least conceptually) with no loss of information yet.

> 
> - When possible distances should be computed using one of the existing
>   functions (GetLineLength, Distance, EuclideanNorm) or at least hypot (because
>   it's clearer)

And you isolate policy into fewer places, with fewer tuning knobs littered all over the place.


> - sqrt( x*x + y*y) is wrong with ints since the product can overflow (at least
>   one x and one y should be cast to double)
> 
> - When possible it's better to compare the distances squared

Yes, if the strangeness is worth the speed.  For something relative, i.e. a rough ranking,
Thomas Spindler used another technique that I wrapped into close_ness() in spectra_export.
 It could have been called "ball_park".


> - The DEG2RAD and RAD2DEG have their sisters DEG2RAD10 and RAD2DEG10,
>   which use decidegrees instead of degrees. This clean up a lot of *10
>   and /10 stuff (also eases switch to another eventual degree unit)
> 
> - The pattern r * sin(a), r * cos(a) is widely used (mostly for
>   building arcs and circles), so I added the sin10radius, cos10radius
>   functions with accepts the angle directly in decidegrees. Result is
>   double so it should be rounded if necessary (I'm not so sure about
>   this... maybe it's better to return the rounded int?)

I have found it better to return the most information possible, the double, and let the
caller establish usage (rounding) policy on a case by case basis.  Dumbing down a function
reduces its applicability.


> 
> - Likewise the atan2 function is usefully replaced by the (preexisting) 
>   ArcTangente one; the following are then equivalent:
> 
>   orient = -(int) ( RAD2DEG( atan2( (double)size.y, (double)size.x ) ) * 10.0 );
>   
>   orient = -ArcTangente( size.y, size.x );
>           
>   The two double cast are not necessary anyway, but maybe they where
>   there to make notice of the implicit cast;

Possibly MSVC++, which is now excluded.

 I think they are noisy, but they
>   could be left. Also atan2(0, 0) gives 0 by specification (since forever,
>   IIRC) so it's not needed to check it as a special case.
> 
> - Another widely used pattern is this:
> 
>   orient += 900;
>   if( orient >= 3600 )
>       orient -= 3600;
> 
>   I added to the normalization functions ADD_ANGLES to have this
> 
>   orient = ADD_ANGLES( orient, 900 );
> 
>   Maybe the function name is nicer if lowercase? I'd like it better (also it's
>   a simple function, the other normalizations where macros).
> 
> - Try to use the angle normalization functions when they are available
> 
> - Lot and lot of degrees <-> radians converted to use the inline functions so that
> 
>   double theta2 = EndAngle * M_PI / 1800.0;
> 
>   becomes
> 
>   double theta2 = DEG2RAD10( EndAngle );
> 
>   I hope everyone think is a better expression.

Yes I do, but the function name tells me nothing.  DECI_DEGRESS  is better clarity
somewhere in the name if that's what you are using.


> 
> - g_RotationAngle is an angle but remains an int since it's actually a UI
>   variable
> 
> - Some small functions in trigo.cpp moved as inline in trigo.h, using const
>   references when possible. Also I think that the previous DistanceLinePoint
>   was vulnerable to int overflow since the double cast was outside the
>   expression. I'd like a confirmation on this.
> 
> - I think that the only 'hardcoded' angles in decidegrees are 0, 900, 1800,
>   2700 and 3600, which is fine. Also there is the arrowhead half-angle of 27.5
>   degrees (I wonder from which standard this comes...)
> 
> - Most output function still keep their /10 factor for showing angles (there
>   are few of these, actually).
> 
> - s_thermalRot is actually a costant... kept it as a static variable
> 
> Also the patch is long but veeery boring :( all the points above repeated many
> many times.
> 
> Need to recheck the whole thing and look if I missed something.

If you want further clean up work.  I no longer think we need the BOARD_ITEM::Show() debug
functions.  It is very easy now to use

PCB_IO	io;

io.Format( board_object )

printf( "", io.GetString() )

or something like that.

In debug contexts and for eventual clipboard writing.

Dick


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