kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

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 <stambaughw@xxxxxxxxx> 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 <john.j.beard@xxxxxxxxx> 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 <oe.nick@xxxxxxxxx> 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 <john.j.beard@xxxxxxxxx>:
>>>>>>
>>>>>> 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     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>> _______________________________________________
>>> Mailing list: https://launchpad.net/~kicad-developers
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp