One more comment about '(D)RendererSharedMemory in FX, but not 2D': This reduces the memory footprint of the RendererContext among Renderer and RendererNoAA implementations (shared arrays) that can co-exist at runtime if both AA and NonAA rendering is needed.
As this behavior is unique to JavaFX, I wonder if it is worth to backport this small difference in Marlin2D where there will be no gain. Laurent 2017-05-16 23:11 GMT+02:00 Laurent Bourgès <[email protected]>: > Hi Jim, > > I didn't notice anything functionally wrong with reviewing the webrev. >> I'm going to do some basic testing on it now while you create a JBS bug for >> it and submit for a formal review. But I did notice some discrepancies by >> diffing the sources against each other which it would be nice to know if >> they were oversights or "future work". If there is something you think >> should be addressed now, do that in the process of switching to a formal >> review (with JBS #) on it... >> > > I will create the JBS bug asap. > > >> I did a bit of diffing: >> >> - Marlin2D against MarlinFX >> - *NoAA against the AA versions >> >> and noticed: >> > > Thanks for your carefull analysis, here are my comments below: > > - RendererNoAA and DRendererNoAA line 38 are missing a "d" suffix. >> - RendererNoAA and DRendererNoAA lines 61,90 - missing comment about test >> > > I forgot to synchronize recent changes into RendererNoAA variants: good > catch ! > > >> - Helpers and DHelpers - some adjustment of "pts[ off+1 ]" lines that >> didn't happen in 2D >> > > I decided to revert those minor syntax changes. > > >> - Configurable constants for Inc/Dec/Quad/Cubic in 2D, but not FX >> > > I hesitated but decided to backport the support for these configurable > constants in all (D)Renderer(NoAA) (4 variants) > > >> - DRendererSharedMemory in FX, but not 2D >> > > >> - FloatMath.ceil_f deleted from FX, but not 2D (not used in 2D) >> - (FloatMath.floor_f is also only in 2D, but it is used in >> MarlinRenderingEngine) >> > > I decided to leave those FloatMath differences as Marlin2D (and FloatMath > Tests) do use these methods. > > > Finally I propose another MarlinFX webrev: > http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.2/ > > Changes: > - (D)Helpers: revert syntax changed in cubicRootsInAB() + fixed comment in > subdivide() > - (D)MarlinRenderingEngine: added logs for cubic/quad properties > - (D)Renderer and (D)RendererNoAA: use cubic/quad properties like Marlin2D > - (D)RendererNoAA: fixed syntax changes mentioned: missing "d" suffix + > comment about test > - MarlinProperties: added quality settings (cubic/quad) > > > PS: I will send an updated webrev for the Marlin2D patch. > > > Regards, > Laurent > -- -- Laurent Bourgès
