X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f X-Recipient: geda-user AT delorie DOT com X-Envelope-From: paubert AT iram DOT es Date: Wed, 8 Jul 2015 10:22:56 +0200 From: "Gabriel Paubert (paubert AT iram DOT es) [via geda-user AT delorie DOT com]" To: geda-user AT delorie DOT com Subject: Re: [geda-user] [RFC][PATCH] PCB: Allow non rounded clearances for rectangular/square pins and pads Message-ID: <20150708082256.GG13243@visitor2.iram.es> References: <20150625163731 DOT GA18117 AT visitor2 DOT iram DOT es> <1435268559 DOT 24445 DOT 13 DOT camel AT cam DOT ac DOT uk> <20150625233042 DOT GA32022 AT visitor2 DOT iram DOT es> <4f11772b1f791b161f8fd0109381f985 AT cam DOT ac DOT uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4f11772b1f791b161f8fd0109381f985@cam.ac.uk> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spamina-Bogosity: Spam Reply-To: geda-user AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: geda-user AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk Hi Peter, sorry for the delay, I've been distracted by other things. On Sat, Jun 27, 2015 at 11:12:59AM +0100, Peter C.J. Clifton wrote: > On 2015-06-26 00:30, Gabriel Paubert (paubert AT iram DOT es) wrote: > >I've not changed any internal API at all yet. For now the patches in > >this area boil down to these modifications in the structure > >definitions: > > > > > >diff --git a/src/polyarea.h b/src/polyarea.h > >index 43fd93d..f63e83d 100644 > >--- a/src/polyarea.h > >+++ b/src/polyarea.h > >@@ -82,8 +82,8 @@ struct VNODE > > { > > VNODE *next, *prev, *shared; > > struct { > >- unsigned int status:3; > >- unsigned int mark:1; > >+ unsigned char status; > >+ unsigned char mark; > > } Flags; > > CVCList *cvc_prev; > > CVCList *cvc_next; > >@@ -96,15 +96,15 @@ struct PLINE > > Coord xmin, ymin, xmax, ymax; > > PLINE *next; > > VNODE head; > >- unsigned int Count; > > double area; > > rtree_t *tree; > >- bool is_round; > > Coord cx, cy; > > Coord radius; > >+ unsigned int Count; > >+ bool is_round; > > struct { > >- unsigned int status:3; > >- unsigned int orient:1; > >+ unsigned char status; > >+ unsigned char orient; > > } Flags; > > }; > > > >The reasons are: > >- when the structure size is at least a multiple of 4 bytes (32 bit), > > and more likely 8 (64 bit machines and some 32 bit because there is > > a double in there), it is stupid to use 2 bit fields when two > > unsigned char fields will do (extracting from bit fields and > > especially assigning to is more expensive). > > Granted.. bitfields are ugly here. I didn't realise we used them. > > >- the is_round field in the middle of PLINE is very wasteful for > > alignment. > > I put that there.. next to the coordinates (to be close to the > variables it makes sense with). I understand, but in this case, put all the other fields which add up to the alignment constraints also, they won't waste memory. The coordinates are probably the most frequenctly accessed fields (maybe I'me wrong) and in any case filling alignment holes is unlikely to hurt. The structure declaration should group Count, Flags, and is_round together, perhaps making the last one a char to make sure that is is not 32 bit on some ABIs. Count has to be first or last, but the whole group may indeed be better put close to the coordinates. > > Great to move these, and with our piecewise linear circular circular > approximations, we do get a lot of VNODEs. Indeed, but what would it cost to have circular arc primitives in the polygon boundaries (much fewer VNODES, and more complex algorithms)? This said, when you have lots of items of a given type, standard memory allocators may become a performance concern (and fortunately we're not speaking of mutithreaded code here). In this case a magazine of reusable items. > I would, however, be surprised if this makes any measurable > difference in speed, but I might be wrong. I don't think it will make any, and it it does, it will probably be indirectly through cache footprint and maybe avoiding bit-field insertions. > > >I'm not sure for the area field , IIRC the alignment of doubles depends > >on the ABI for 32 bit architectures and anyway 64 bit is what > >really matters > >these days. > > Agreed. > > > Let me know if your shorted sliver cases is fixed by any > (combination) of my patches. I suspect they will. > It is fixed by the first patch. Before applying your patch, I thought it was some initialization problem because the sliver disappeared as soon as I changed anything (pad or line size, etc) and then reverted the change. For your second patch, I had started in a different direction: namely to adapt the callers of frac_circle so that they work according to the documentation: frac_circle draws all the points except the last. I'm not sure that this is the best API, but it is what is documented (in general I find the number of comments which obviously don't match the code really appalling, I'm speaking only of the polygon code). There are things in the code that don't make much sense, for example the code for contour creation (poly_NewContour) seems to support the creation of an initial empty contour, but it actually inserts a node at (0,0). So nobody actually calls it with the parameter set to NULL and the check is useless. Crashing would be better IMHO! So should we allow empty contours of not? I'm undecided, although in this case my tendency would be to say: in doubt do as ps/pdf/cairo (which allow empty contours) but this may be overkill. Another interface I don't like is frac_circle, as far as I can tell, it is the only interface with which you can create both open (quarter or half circles) contours and closed ones (full circles). I believe that trying to conflate these two fundamentally different types of contour creation functionality is the main issue here. Regards, Gabriel