kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

On 11/02/2013 05:38 PM, Lorenzo Marcantonio wrote:
> OK, I think I've got bug-for-bug compatibility with trunk in my branch,
> the split of the GAL layer number seems to work fine now.
>
> I know that it's all in early phase and work in progress, so I feel
> 'normal' that many things are not working (like color changing, the zoom
> function - and why meta-F1/F2 and not F1/F2 like before, can't hide some
> things and show on).

It would be great to put such stuff on a list, so it will not be 
forgotten and I can handle it when I am done with more important bugs.

> However since nobody ever mentioned yet there seems to be an off-by-one
> error or a polygon misclosing in the zone edge drawing: when there is an
> 'hole' in a zone instead of closing it and lifting the pen, jumps
> drawing to another area (with the pen down:P).

Then how do you recognize holes while drawing? I thought that only 
'end_contour' needs checking.

> I am not very fond of the approach of the PCB_PAINTER, which contains
> and switch-dispatch drawing of entities... why not delegate as usual to
> the single BOARD_ITEMs? (or maybe define a PCB_PAINTABLE interface, or
> something)

It is a matter of approach, I prefer to split model and view to 
different classes. Although - it is possible for drawable items to 
include routines for drawing. An example is selection box, it is drawn 
by ViewDraw(). The role of the PCB_PAINTER is to draw those items that 
may occur on a PCB.

> Also, more like of a curiosity, why use deques in the stroke font code?
> I'm a little rusty with STL but vectors seems a more likely fit... given
> the usage pattern I feel there's something with the performance of
> vector::push_back (IIRC deques allocate in blocks, I don't remember the
> details). Also for some reason text drawn with opengl is less 'crisp'...
> maybe it's the coordinate rounding, I don't know.

Correct me if I am wrong, but using vectors may lead to many 
reallocations if you resize it (as you have to assure continuous data 
space) and deques are cheaper for dynamic resizing. Although I see a few 
spots, where it could be optimized - vectors are good when you know the 
size apriori.
What do you mean by 'crisp' text? In comparison to the default renderer 
or Cairo?

> At last, the details on the type split: it works exactly at expected.
> The 'trick' is as before in layers_id_colors_and_visibility.h, attached
> as a preview for the curious ones, full code in my branch. It still uses
> the enum implementation so people which didn't like it a couple years
> ago still will not like this one :D
>
> GAL_LAYER_NUM has in fact the same numeric values of the int previously
> in use, however is better protected since it's not an int and it's type
> checked. Macros ITEM_GAL_LAYER and PCB_GAL_LAYER are used for moving
> from the PCB_VISIBLE/LAYER_NUM domain to GAL_LAYER_NUM. Sadly these are
> macros (and ugly ones) because these need to be used in const exprs, so
> overloaded operators or functions can't be employed... *maybe* the new
> constexpr keyword could help, I don't know exactly how it works. I'm
> pretty sure there is no performance penalty since all of these
> conversion can be actually compiler folded. For experience I know g++ is
> sufficiently smart.
>
> For the reverse thing: at the moment getting back the LAYER_NUM from a
> GAL_LAYER_NUM is in ROUTER_TOOL, members pickSingleItem and
> updateStartItem. I hand coded a substitution inline, probably the best
> way to handle it is to use a conversion function with a behaviour like
> dynamic_cast<>, returning UNDEFINED_LAYER if the conversion can't be
> done (or maybe trigger an exception/assert?).

I am not very familiar with push & shove code, but I guess it should be 
enough. Probably those functions work with copper layers (for which 
LAYER_NUM and GAL_LAYER_NUM should be equal).

> Another nuisance: even if LAYER_NUM IS-A GAL_LAYER_NUM (in the Liskov
> sense, too), enum are deemed distinct and can't inherit, so there is
> a need for overload utility functions (look for IsCopperLayer); as usual
> a class and the new class literals would help, but I don't know how much
> they are stable/implemented (look here
> http://www.preney.ca/paul/archives/636).  Also they are only in gcc 4.7,
> I think, so it's not a lightweight decision to take.
>
> Of course this is only if you like the type safe approach; if you want
> it I can backpatch to trunk; if you just want the type tag as a typedef
> for better code documentation only (like it was done for
> LAYER_NUM/LAYER_MSK) I can prepare a patch for this too. Otherwise just
> don't care about it:P:P

I like the idea, but I am not the one to decide whether it should be 
commited or not. As I would like our changes to be merged, I ought to 
stick to the master repository and its rules.
Anyway, I am really thankful for such a careful code review. Some of 
bugs may become fixed before discovered by users and other remarks gave 
me a few thoughts about the code.

Regards,
Orson