[10] RFR 8188062: Use Marlin renderer in JavaFX BasicStroke
Phil Race
philip.race at oracle.com
Wed Nov 8 22:35:51 UTC 2017
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.rushforth at oracle.com
> <mailto:kevin.rushforth at 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.laurent at gmail.com <mailto:bourges.laurent at gmail.com>> a
>> écrit :
>>
>> Hi Phil,
>> Thanks for your comments.
>>
>> Le 27 oct. 2017 18:28, "Phil Race" <philip.race at oracle.com
>> <mailto:philip.race at 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
>>>
>>
>>
More information about the openjfx-dev
mailing list