I am fine for Kevin to push this as is.
-phil.
On 11/08/2017 01:36 PM, Laurent Bourgès wrote:
Kevin & Phil,
Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8188062.1/
<http://cr.openjdk.java.net/%7Elbourges/marlinFX/marlinFX-8188062.1/>
Changes:
- wrapPath2d typo => wrapPath2D()
- whitespace / braces in (D)TransformingPathConsumer2D
As mentioned in my answer to Phil's review, I moved the CAP/JOIN
constants into BasicStroke (as in Java2D) as it is the public API (for
me) instead of importing them from renderer implementation (openpisces
or Marlin).
Build OK on up-to-date OpenJFX10 + tested with MapBenchFX.
If that webrev is OK, please push it for me.
FYI the remaining OpenPisces references are listed below (and
OpenPisces can be easily removed in other OpenJFX release):
./src/main/java/com/sun/prism/impl/shape/ShapeUtil.java: new
com.sun.openpisces.Stroker(p2d, lw, stroke.getEndCap(),
./src/main/java/com/sun/prism/impl/shape/ShapeUtil.java:
pc2d = new com.sun.openpisces.Dasher(pc2d, stroke.getDashArray(),
./src/main/java/com/sun/prism/impl/shape/OpenPiscesPrismUtils.java:import
com.sun.openpisces.Dasher;
./src/main/java/com/sun/prism/impl/shape/OpenPiscesPrismUtils.java:import
com.sun.openpisces.Renderer;
./src/main/java/com/sun/prism/impl/shape/OpenPiscesPrismUtils.java:import
com.sun.openpisces.Stroker;
./src/main/java/com/sun/prism/impl/shape/OpenPiscesPrismUtils.java:import
com.sun.openpisces.TransformingPathConsumer2D;
./src/main/java/com/sun/prism/impl/shape/OpenPiscesRasterizer.java:import
com.sun.openpisces.AlphaConsumer;
./src/main/java/com/sun/prism/impl/shape/OpenPiscesRasterizer.java:import
com.sun.openpisces.Renderer;
./src/main/java/com/sun/prism/sw/SWContext.java:import
com.sun.openpisces.Renderer;
./src/main/java/com/sun/prism/sw/DirectRTPiscesAlphaConsumer.java:import
com.sun.openpisces.AlphaConsumer;
./src/main/java/com/sun/prism/sw/DirectRTPiscesAlphaConsumer.java:import
com.sun.openpisces.Renderer;
./src/main/java/com/sun/openpisces/Curve.java:package com.sun.openpisces;
./src/main/java/com/sun/openpisces/TransformingPathConsumer2D.java:package
com.sun.openpisces;
./src/main/java/com/sun/openpisces/Helpers.java:package
com.sun.openpisces;
./src/main/java/com/sun/openpisces/Dasher.java:package com.sun.openpisces;
./src/main/java/com/sun/openpisces/Stroker.java:package
com.sun.openpisces;
./src/main/java/com/sun/openpisces/Renderer.java:package
com.sun.openpisces;
./src/main/java/com/sun/openpisces/AlphaConsumer.java:package
com.sun.openpisces;
./src/main/java/com/sun/marlin/MarlinAlphaConsumer.java:import
com.sun.openpisces.AlphaConsumer;
(the openpisces.AlphaConsumer interface should be kept and moved in
another package like marlin ?)
Cheers,
Laurent
2017-11-08 0:04 GMT+01:00 Kevin Rushforth <kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@oracle.com>>:
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
