[10] RFR 8188062: Use Marlin renderer in JavaFX BasicStroke
Kevin Rushforth
kevin.rushforth at oracle.com
Wed Nov 8 22:56:50 UTC 2017
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.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