Hi Laurent,
I'm not sure why you added the pathClosed variable back. Before your
fix we used to rely on all of the downstream consumers correctly
implementing the case of CLOSE followed by a non-MOVE command so
specifically enforcing a MOVE here should be unnecessary and just
complicates the code...?
...jim
On 3/16/16 8:16 AM, Laurent Bourgès wrote:
Jim,
Here is another webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.3/
Changes:
- restored pathClosed flag to fix the 'hour glass' issue below
- added pathClosedTest in CrashNaNTest
Comments below:
2016-03-15 23:45 GMT+01:00 Jim Graham <[email protected]
<mailto:[email protected]>>:
The new tests look good.
Thanks.
There is one logic error in the new version of the degenerate
coordinate filter.
It is valid to have a PATH_CLOSE immediately followed by a <geom>To
call and the subpath will start at the previous close/moveto point.
For example:
moveto 40, 40
lineto 0, 0
lineto 80, 0
closepath
lineto 80, 80
lineto 0, 80
closepath
should draw an hourglass, but with your fix it will lose the lower
half because the lineto after the close will be turned into a moveto
(and the second subpath then becomes empty.
I tested my previous patch and you were right: it was buggy.
I think it should be fine just getting rid of the subpath=false in
the close case.
I prefered reverting to the ductus approach even if your proposal seems
working.
I agree my patch is a bit tricky but it explicitely emits a moveTo()
after pathClosed() instead of relying to the path consumer to implement
the same behaviour (in dasher / stroker)...
It is arguable, though, if we have closepath followed by a NaN if
perhaps that NaN should break the subpath from the previous close,
but I don't think that is in keeping with the current philosophy of
simply pretending that bad coordinates don't exist. Essentially,
the close of a previously valid path gives us a valid start to the
new subpath regardless if it immediately feeds into more bad
coordinates - it still started its run on a valid point. So, I'd
argue that simply passing along the prior value of the subpath flag
would be appropriate, whether it is true or false (and, obviously,
also closing the path in the consumer if it is true)...
Please tell me what solution do you prefer.
I can adopt yours which seems really simple.
Laurent
