I did a fairly quick review and ran tests on two platforms. It looks
good to me. I have some minor formating nits:
PathConsumer2D.java:
1. In the following:
+ public DPathConsumer2D wrapPath2d(Path2D p2d)
+ {
the opening curly brace can go at the end of the method declaration
(same pattern appears in a few other files)
2. Can you add a blank line before the Path2DWrapper class declaration?
Other than that, as long as you address Phil's concerns it is OK to go in.
Thanks.
-- Kevin
Laurent Bourgès wrote:
Just a gentle reminder...
Kevin, could you have a look ?
I have other patches coming soon...
Cheers,
Laurent
Le 27 oct. 2017 8:02 PM, "Laurent Bourgès" <bourges.laur...@gmail.com
<mailto:bourges.laur...@gmail.com>> a écrit :
Hi Phil,
Thanks for your comments.
Le 27 oct. 2017 18:28, "Phil Race" <philip.r...@oracle.com
<mailto:philip.r...@oracle.com>> a écrit :
38 private final Path2DWrapper wp_Path2DWrapper =
new Path2DWrapper();
Are there tabs here ? The formatting looks odd.
It is manually aligned with followings lines (only space).
44 public DPathConsumer2D wrapPath2d(Path2D p2d)
The method naming pattern I see elsewhere is "2D" not "2d" .. so can
we fix this one ?
ie make it wrapPath2D
This occurs in at least two different files in this webrev:
[D]TransformingPathConsumer2D.java
I agree and I can fix it here and also in Marlin2D (same code).
I really don't like the changes in BasicStroke that not use direct
literals instead of copying
the values from Stroker. It can't possibly help performance and you
lose the implied relationship.
I agree your point of view. However the reference or origin was
the public BasicStroke in 2d and Marlin checks that constants
match in static initializers (MarlinRenderingEngine).
I prefer applying the same pattern in FX so I did that change
which removes the reference to OpenPisces.Stroker.
- public static final int CAP_BUTT = Stroker.CAP_BUTT;
- public static final int CAP_ROUND = Stroker.CAP_ROUND;
- public static final int CAP_SQUARE = Stroker.CAP_SQUARE;
-
- public static final int JOIN_BEVEL = Stroker.JOIN_BEVEL;
- public static final int JOIN_MITER = Stroker.JOIN_MITER;
- public static final int JOIN_ROUND = Stroker.JOIN_ROUND;
+
+ /** Constant value for end cap style. */
+ public static final int CAP_BUTT = 0;
+ /** Constant value for end cap style. */
+ public static final int CAP_ROUND = 1;
+ /** Constant value for end cap style. */
+ public static final int CAP_SQUARE = 2;
+
+ /** Constant value for join style. */
+ public static final int JOIN_MITER = 0;
+ /** Constant value for join style. */
+ public static final int JOIN_ROUND = 1;
+ /** Constant value for join style. */
+ public static final int JOIN_BEVEL = 2;
The next one is not something I think must be fixed but an observation that it
seems odd to have to decide which Rasterizer to use every time some one
calls this
API rather than making it a one time initialisation.
I wondered the same question. As PrismSettings are constants,
hotspot will optimize that code as a direct call to the proper
method (dead code elimination).
+ public static Shape createCenteredStrokedShape(Shape s,
BasicStroke stroke)
+ {
+ if (PrismSettings.rasterizerSpec ==
RasterizerType.DoubleMarlin) {
+ return DMarlinRasterizer.createCenteredStrokedShape(s,
stroke);
+ }
+ if (PrismSettings.rasterizerSpec ==
RasterizerType.FloatMarlin) {
+ return MarlinRasterizer.createCenteredStrokedShape(s,
stroke);
+ }
+ // JavaPisces fallback:
+ return createCenteredStrokedShapeOpenPisces(s, stroke);
+ }
+
Laurent
-phil.
On 10/25/2017 02:11 PM, Laurent Bourgès wrote:
Please review this simple patch that moves the
BasicStroke.createCenteredStrokedShape() implementation into ShapeUtil
in
order to use Marlin instead of OpenPisces:
JBS: https://bugs.openjdk.java.net/browse/JDK-8188062
<https://bugs.openjdk.java.net/browse/JDK-8188062>
Webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8188062.0/
<http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-8188062.0/>
Changes:
- BasicStroke: fixed definition of CAP / JOIN constants +
createCenteredStrokedShape() calls ShapeUtil
createCenteredStrokedShape()
(delegation)
- ShapeUtil.createCenteredStrokedShape() delegates to
(D)MarlinRasterizer
(if enabled) or to OpenPisces (fallback)
- (D)MarlinRasterizer: applied the same approach as in Marlin2D
createStrokedShape() except I adopted the lineWidth trick as in
OpenPisces
- (D)MarlinPrismUtils: some refactoring to let the new strokeTo() method
reuse the initPipeline() + simplified Path2D special handling
- (D)RendererContext / (D)TransformingPathConsumer2D: added cached
Path2D
instance and Path2DWrapper (like in Marlin2D)
PS: Removing OpenPisces in the future will be easier as remaining
references are in ShapeUtil, OpenPiscesPrismUtils and few sw pipeline
classes. Use the following command to get usages:
find . -name "*.java" -exec grep -H "openpisces" {} \;)
Cheers,
Laurent
