kicad-developers team mailing list archive

[Vladimir Uryvaev <vovanius@xxxxx>]

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!

-- 

--- KeyFP: DC07 D4C3 BB33 E596 CF5E  CEF9 D4E3 B618 65BA 2B61