OK, I'll do it tomorrow.

-- Kevin


Phil Race wrote:
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



Reply via email to