[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [10] RFR 8188062: Use Marlin renderer in JavaFX BasicStroke
From:       Laurent_Bourgès <bourges.laurent () gmail ! com>
Date:       2017-10-27 18:02:36
Message-ID: CAKjRUT5+2PK41O4SaB=kTnPLhBMxn=tBS6XuYP691bcD8iWNvw () mail ! gmail ! com
[Download RAW message or body]

Hi Phil,
Thanks for your comments.

Le 27 oct. 2017 18:28, "Phil Race" <philip.race@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
[prev in list] [next in list] [prev in thread] [next in thread]