-------- 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
Jean,
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
написал:
Vladimir,
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
changes.
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.
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
EDA_DRAW_FRAME).
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.
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!
