← Back to team overview

kicad-developers team mailing list archive

Re: Fwd: Re: Concerns about clearing disagreements before committing.


My first mail seem lost.
Sorry if it is duplicated.

Le 21/11/2011 07:04, jean-pierre charras a écrit :

-------- Message original --------
Sujet: Re: [Kicad-developers] Concerns about clearing disagreements before committing.
Date : Sun, 20 Nov 2011 22:24:11 +0400
De : Vladimir Uryvaev <vovanius@xxxxx>
Répondre à : vovanius@xxxxx
Pour : kicad-developers@xxxxxxxxxxxxxxxxxxx


The qustions you ask are most essential, and I have to give clear answers, for
the concept to be understood.

В сообщении от Воскресенье 20 ноября 2011 18:10:51 автор jean-pierre charras
First, Thank you for your work, and the answers to our questions.

My main concern is about the complexity of the new code.

I explain:
If I read
VECTOR_PCB m_Pos; // pad Position on board
instead of
wxPoint m_Pos; // pad Position on board

I do not think this is a complex code.

But when I read:

// Returns the position of the pad.
const wxPoint D_PAD::ReturnShapePos()
if( m_Offset[0] == ZERO_LENGTH && m_Offset[1] == ZERO_LENGTH )
return TO_LEGACY_LU_WXP( m_Pos );

wxPoint shape_pos;
int dX, dY;

dX = TO_LEGACY_LU( m_Offset[0] );
dY = TO_LEGACY_LU( m_Offset[1] );

RotatePoint( &dX, &dY, m_Orient );

shape_pos.x = TO_LEGACY_LU( m_Pos.x() ) + dX;
shape_pos.y = TO_LEGACY_LU( m_Pos.y() ) + dY;

return shape_pos;
I think this is a complex code.

The main reason is (in this case, but there are a lot of similar cases)
the pad position is in new units but its shape position is in old units,
and there is no reason to do that. (this shape pos is used in DRC and zone
filling calculations and should be in internal units)

As a consequence, some parameters that have the same meaning are in new
units and other are in old units. And remember when I must use m_Pos.x and
when I must use TO_LEGACY_LU( m_Pos.x() ) is not an easy task.

First, macros FROM/TO_LEGACY_LU... should be need only through period when not
all the code is converted. They're 'gateways' between already converted code
and the code did not processed yet, and will not be needed after the
conversion is done.

For many reasons the all the code cannot be be converted in time. For many
reasons it is not possible to track all needed changes in more simple way. And
that bunch of typedefs and macros is the tool to track units through all
sources. Trust me, I tried to do conversion several times in my local forks
and failed. Possibly I missed only few bugs but I could not track and fix the
that way, so I developed this one.

A side effect of these changes is the number of changes needed.
Obviously this will create bugs
Example: the line if( m_Offset[0] == ZERO_LENGTH && m_Offset[1] ==
ZERO_LENGTH ) fixes a bug due to a copy/paste command: the actual string
is currently:
if( m_Offset[0] == ZERO_LENGTH && m_Offset[0] == ZERO_LENGTH ).

You're absolutely right, I'll take thought how to make conversion in less

BTW, there should be no such string in final code, as this task

Can you tell me if this complexity is just due to the fact your work is a
work in progress I mean, when all changes will be done, if lines like:
shape_pos.x = TO_LEGACY_LU( m_Pos.x() ) + dX;
shape_pos.y = TO_LEGACY_LU( m_Pos.y() ) + dY;
will be changed to
shape_pos.x = m_Pos.x() + dX;
shape_pos.y = m_Pos.y() + dY;
that is a "reasonable" code.

Thats true, and I heading toward this.

If fact, from my point of view (I may be wrong) change from old to new
internal units is mainly located in Save/Read functions (that should
handle a scaling factor),
and should not involve many changes outside these functions.

This is possibly true, but you know, I have no right to miss single place
where intrusion is need, so the only way to be is to move through the code
step by step, revising line by line.
Are you sure ?
Many time we must choose between drawbacks, not between advantages.
Using intrusive code to track a possible bug (i.e. mainly overflow) is certainly good for new code.
But for an existing code, this way has a hight cost:
A *lot* of changes are needed, and many changes made twice:
 once to add new code.
 once to simplify this new code or remove it in many places.
The cost is very hight:
 It takes a long time and a lot of work.
 It creates new bugs (typos... when adding and when simplifying the code).

I am not sure the benefit is hight ( I am pretty sure it is low). Here are my reasons:
 - the current code is well known and debugged. There are very few places where overflows can happen:
   - 2 or 3 functions using segment slope (I already fixed one)
   - Some polygon calculations ( but for this case, the new code cannot trap overflow, I think)
   - And (but this is not an overflow issue) we can have issue with very small scaling factors in draw functions
     (mainly under Windows).
So my concern is: new intrusive code will create
(because the numerous changes in code, and typo errors created by these changes)
more bugs than you can catch.

So here is my opinion ( 1 and 2 are very important ):
1 - accept the risk to find an unexpected overflow inside Kicad code (this is in fact a low risk).
2 - do not accept the risk to create some bugs due to typo errors and new complexity code (this is in fact a hight risk).
3 - do not spent time to modify existing code, outside Read/Write functions and few other functions.
4 - take in account there is already a support inside Kicad to handle conversion
   from/to internal units and strings in dialogs, or plotter units that never use decimils.
5 - Issues can be found outside Kicad code and will not trapped by intrusive code.

Please, note also issues (not bugs) can be found in dialogs that display values: for instance:
 1 mm is displayed as 1.0 mm
but in inches:
 0.0393700784 inch.
Is 0.0393700784 pleasant in a dialog ?
But if less digit are displayed (said 0.03937, more pleasant),
what happen when the internal unit will be created from the dialog?

Other changes (but this is also the case in this new code) are related to
some constants in constructors and some dialogs.

Note in dialogs, values are always converted to string using a scale factor
depending on the internal units see PutValueInLocalUnits ,
ReturnValueFromString functions and m_InternalUnits member of

Currently to mantain original version in working state, I wrote second pair of
conversion functions, only one of them could remain at the end.

BTW lets give some critic: names of that functions give no idea that they are
pair of inverses to each other, so I proposed another pair.

Better names make better code, at low cost.

Draw functions (due to very different values of draw scale factors) could
have some issues, that can be fixed by a prescaling factor (in new code,
this is already done).

I understand your code is mainly to trap bugs relative to the new internal
values throughout the Kicad code. But are you sure this changes (new
internal unit value) can create bugs everywhere. (changes everywhere can
create bugs everywhere)

That's the reason why I tried to convert all the code in the same time and I
failed. Every step of conversion could be tested and I do this as far as it
possible. My work is not only create bugs, but also fix them. :)

outside Read/Write and constructors functions (very few files) the main
hazard is related to arithmetic overflow Note this can happen only with 32
bits machines.

I am thinking there are only few functions that can create this problem:
mainly some (duplicated code) that calculate if 2 segments are collinear or
use the slope of a segment in intermediate calculations. This is easy to
fix, just find them ... and note they are currently already broken. An
other overflow possibility is inside BOOST::polygon classes, but the
current new code cannot fix that.

My position that only assuming what functions may create problem is
insufficient, we should be sure on this.

My intention in final to give simple and efficient framework for dealing with
spacial data.

Again, Thanks.

Thank you for great project!

Jean-Pierre CHARRAS
KiCad Developers team.
KiCad Developers <kicad-developers@xxxxxxxxxxxxxxxxxxx>

Follow ups