Re: [Kicad-developers] Misc findings during GAL_LAYER_NUM work
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 ___ Mailing list:
Re: [Kicad-developers] Misc findings during GAL_LAYER_NUM work
On Mon, Nov 04, 2013 at 09:40:55AM +0100, Maciej Sumiński wrote: Then how do you recognize holes while drawing? I thought that only 'end_contour' needs checking. No idea, it's only an assumption from the symptoms... see screenshots. Bottom center, the big catode pad; Bottom right the big hole (it's a screw hole for a huge heat sink:D) 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. I know, it's only a philosophic issue :D 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 Exactly what it meant. However these are one shot adds and many time retrieve... and anyway the performance difference would probably swamped by the opengl/cairo machinery. What do you mean by 'crisp' text? In comparison to the default renderer or Cairo? Against the default renderer. Cairo antialiases everything but OTOH is so slow it's nearly unusable (about 20 seconds for the initial caching!)... see screenshots, again. It's a nearly unavoidable rounding issue (in fact there are patents for text hinting systems!); look upper left the P17/P18 stem width; it's not uniform because rounding happens in different stages: - Standard text has scaled vertices and scaled width and it's stroked with that width, in pixels - consistent stroke width. - OpenGL probably scales everything during projection so width is subject to rounding. It's possible that result changes with OpenGL drivers and/or hints and/or phase of the moon. In fact OpenGL doesn't guarantee line widths different from 1, either (look at glLineWidth specs). In short: don't worry about it. Maybe there is an extension somewhere to fix it but it's not important. 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). That's the idea; however since it call the GetTopLayer there is no static guarantee it will be a pcb layer instead of an item one, unless it can be proven that the function is called under that condition. Under the classic animals OO the problem is like: if I put a tiger in a cage, nobody ensures me that an elephant will come out... more or less :D Such a conversion could catch issues like the -1 bitset index. 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. As usual :P if you want it I'm there. 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. No problem for me... I also do code reviews for a living: when you have safety related software and electronics certification *is* hairy... and some standard (like MISRA-C) are nearly unworkable with (example: you substantially can't use pointers since you could eventually do bad arithmetic; good luck with communication buffers). -- Lorenzo Marcantonio Logos Srl ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
[Kicad-developers] Misc findings during GAL_LAYER_NUM work
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). 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). 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) 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. 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?). 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 -- Lorenzo Marcantonio Logos Srl layers_id_colors_and_visibility.h.gz Description: application/gunzip ___ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
