The new tests look good.
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 think it should be fine just getting rid of the subpath=false in the
close case.
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)...
...jim
On 3/15/2016 6:46 AM, Laurent Bourgès wrote:
Dear Jim,
Here is a new webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144938.2/
Changes:
- pathToLoop() rewritten to simplify the logic (skip and pathClosed
flags removed)
- added several createStrokedShape() tests in CrashNaNTest class
I hope my changes to the filtering algorithm are correct:
I advocate not understanding why it was so complicated (3 flags => 1)
but maybe I am wrong and I missed several corner cases...
My comments below:
2016-03-15 0:34 GMT+01:00 Jim Graham <[email protected]
<mailto:[email protected]>>:
Hi Laurent,
Did you consider adding a test with a completely degenerate path to
make sure that we don't have any state errors if all of the segments
are eaten by the "finite path" filter?
Fixed and it produces empty paths (only pathDone):
I wonder if it is time to remove the following lines 317-322 in
MarlinRenderingEngine.strokeTo():
// Every path needs an initial moveTo and a pathDone.
If these
// are not there this causes a SIGSEGV in libawt.so (at
the time
// of writing of this comment (September 16, 2010)).
Actually,
// I am not sure if the moveTo is necessary to avoid
the SIGSEGV
// but the pathDone is definitely needed.
pc2d.moveTo(0f, 0f);
For me, it is useless as only pc2d.pathDone() is mandatory.
I like your approach. It is worth noting that:
- non-uniform scales are not common
- your "always overflow-filter path at device resolution" fixes the
issues with measuring overflow in user space that I pointed out in
my prior email and only does so at a likely small expense in terms
of non-uniform scale performance. Common cases will see no change
(other than the fact that you have new code in the path feeder).
Thanks.
It has an important consequence for me:
it's possible to introduce a new stage in the pipeline (before
inverseDeltaTransformConsumer) that implements path clipping in
device-space ONLY compatible with the rectangular bounding box.
With respect to the optimization that I gave a rough outline of. It
is true that:
- it should help eliminate all of the inverse transforms in the
strokeTo code
- the math is incomplete and would need some work
- it only targets the non-uniform case which may not be a high priority
- it would probably get rid of the entire TransformingPathConsumer
module, but that module is not complex or trouble-prone and seems to
be working fine
I totally agree.
I think improving Marlin's Stroker to better optimize square caps /
mitter joins or adding path clipping would provide more benefits as it
would minimize the number of point / edges used later by the Renderer stage.
Thanks for your comments,
Laurent
