John, I finally go around to merging your patch set. I'm assuming this patch set will apply to the stable 5 branch as well. Thank you for your contribution to KiCad.
Cheers, Wayne On 07/10/2018 11:55 AM, John Beard wrote: > Hi Wayne, > > (Feel free to tell me to come back to this later if busy with v5!) > > On closer inspection, qa_geometry is NOT a general test of geometry > code, but rather a few tests to ensure that some refactored code > (SHAPE_POLY_SET) matches the original CPolyLine-based code. > > As for the fillet code testing, I had a go at making some actual tests > for the SHAPE_POLY_SET. It basically does the following for a few > combinations of radius/error: > > 1) Make a square shape > 2) Fillet it > 3) Check there's a single resulting polygon > 4) Look at the segments generated by the code and ensure that: > a) The end points are the right distance from the radius centre > b) The mid point error from the ideal arc is within tolerance > c) The line is (roughly, due to rounding) perpendicular to the > line joining midpoint and radius centre > 5) Check the fillet is at least one segment > > These tests do appear to pass for the SHAPE_POLY_SET code. > > You could also do testing against expected co-ordinate results, but > that requires that the Fillet code contracts to return exact, > reproducible result, not just "good enough to fit the given error, but > with latitude within that brief". > > Attached are some patches to illustrate what I am talking about. I'm > not asking to merge before v5, though I believe the first three are > generally harmless in that respect, as they just enable ctest, add the > working qa_geometry tests (renamed to qa_shape_poly_set_refactor) and > add the Python tests. > > The fourth is the new fillet tests, which are still just a concept, > though feedback is welcome. > > Cheers, > > John > > On Tue, Jul 10, 2018 at 4:12 PM, Wayne Stambaugh <[email protected]> wrote: >> I'm not sure what bothers me more, the fact the test fails or the fact >> that the test used different fillet code as the pass/fail criteria >> rather than a hard coded known correct test. Which fillet code was/is >> correct? We really do need to start doing a better job with our >> geometry tests and testing in general. Is the fillet code we use to >> generate polygons and other geometry actually correct? >> >> As far as fixing qa tests, I don't think that will impact the v5 release >> but pushing it off to 5.0.1 or 5.1 probably makes more sense at this >> point. I do agree that there should be an easier way to run tests. >> >> Cheers, >> >> Wayne >> >> On 7/9/2018 5:35 AM, John Beard wrote: >>> Note to Wayne: Nothing here concerns v5 release, I was just trying to >>> get a geom test working for future. >>> >>> On Mon, Jul 9, 2018 at 9:12 AM, John Beard <[email protected]> wrote: >>>> Hi, >>>> >>>> Let's come back to the make test behaviour after v5, I think we'll >>>> need to discuss that separately. However, I think this does illustrate >>>> why we need the tests to be runnable easily, otherwise they suffer >>>> bit-rot, and then the tests are useless. >>>> >>>> Looking at that change, the test is now iterating the second parameter >>>> which is now "max error", not "number of segments". Does this test >>>> still make any sense? It's now comparing: >>>> >>>> SHAPE_POLY_SET::Fillet( radius, error ); >>>> >>>> with >>>> >>>> CPolyLine::Fillet( radius, segments ); >>>> >>>> The original test was designed to ensure SHAPE_POLY_SET::Fillet and >>>> CPolyLine::Fillet were the same, but they now have different >>>> interfaces and semantics. Wouldn't it be better to check >>>> SHAPE_POLY_SET::Fillet (and Chamfer) against some ground truth? >>>> >>>> Cheers, >>>> >>>> John >>>> >>>> On Fri, Jul 6, 2018 at 8:40 PM, Nick Østergaard <[email protected]> wrote: >>>>> Hi >>>>> >>>>> I guess we could add it to the qa target somehow? What I don't >>>>> particularyly >>>>> like with this patch is that executing "make test" does not check for >>>>> dependency changes. >>>>> >>>>> Back to the status about qa_geometry... it did pass a long time ago, >>>>> doing a >>>>> bit of git bisect points at this commit as the one breaking the test. >>>>> >>>>> fbf10e941bdf26bb3618aba0a1b7c44fd0bafed2 is the first bad commit >>>>> commit fbf10e941bdf26bb3618aba0a1b7c44fd0bafed2 >>>>> Author: Jeff Young >>>>> Date: Thu Mar 22 18:02:45 2018 +0000 >>>>> >>>>> Switch zone fillets to absolute-error algorithm. >>>>> >>>>> And some general cleanup to related constants, etc. >>>>> >>>>> :040000 040000 8b6ad8d44a7b38e0355ce5c8897f823d6255f811 >>>>> 8d54d43a9bd6e5062d6b804890a779e785e430cc M common >>>>> :040000 040000 5a90dc20fe7cb3f74ae1768a5b5024a902c9354d >>>>> a2be92ebd64fd46ad17427e8e3c12da7f10df699 M include >>>>> :040000 040000 af9f333c0f56dca3a90fb7b04f385dbf39425e8d >>>>> 99b5f9757c78216a08220b7eb056f343658b961d M pcbnew >>>>> >>>>> >>>>> Den tor. 5. jul. 2018 kl. 12.13 skrev John Beard <[email protected]>: >>>>>> >>>>>> Hi, >>>>>> >>>>>> Are the qa_geometry test supposed to all work? >>>>>> >>>>>> When I run `qa_geometry`, I get 1160 errors like this: >>>>>> >>>>>> error: in "ChamferFillet/Fillet": check { chainPoints.begin(), >>>>>> chainPoints.end() } == { polyPoints.begin(), polyPoints.end() } has >>>>>> failed. >>>>>> >>>>>> Mismatch at position 0: [ 40 | 14 ] != [ 40 | 12 ] >>>>>> Mismatch at position 1: [ 40 | 15 ] != [ 40 | 13 ] >>>>>> Mismatch at position 2: [ 44 | 10 ] != [ 40 | 14 ] >>>>>> Mismatch at position 3: [ 44 | 18 ] != [ 40 | 15 ] >>>>>> Mismatch at position 4: [ 50 | 10 ] != [ 40 | 16 ] >>>>>> Mismatch at position 5: [ 51 | 14 ] != [ 40 | 17 ] >>>>>> Collections size mismatch: 6 != 25 >>>>>> >>>>>> Attached is a patch that enabled CTest tests and adds qa_geometry as a >>>>>> test. Then you can run `make test` or `ctest` to run all tests. I >>>>>> think it would be good to have a single unambigous and easily >>>>>> understood command to be able to run unit tests? >>>>>> >>>>>> This patch explicitly excludes the "ChamferFillet/Fillet" tests as >>>>>> they are failing, but if those tests can be fixed, it would be good to >>>>>> run them too. >>>>>> >>>>>> Cheers, >>>>>> >>>>>> John >>>>>> _______________________________________________ >>>>>> Mailing list: https://launchpad.net/~kicad-developers >>>>>> Post to : [email protected] >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>>>> More help : https://help.launchpad.net/ListHelp >>> >>> _______________________________________________ >>> Mailing list: https://launchpad.net/~kicad-developers >>> Post to : [email protected] >>> Unsubscribe : https://launchpad.net/~kicad-developers >>> More help : https://help.launchpad.net/ListHelp >>> >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~kicad-developers >> Post to : [email protected] >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
