Kevin & Phil,
Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/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>:
> 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> a
> écrit :
>
>> Hi Phil,
>> Thanks for your comments.
>>
>> Le 27 oct. 2017 18:28, "Phil Race" <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
>> Webrev: http://cr.openjdk.java.net/~lbourges/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
>>
>>
>>
>>
>>
