jfree / jfreesvg

A fast, lightweight Java library for creating Scalable Vector Graphics (SVG) output.
http://www.jfree.org/jfreesvg
GNU General Public License v3.0
319 stars 58 forks source link

Class RyuDouble returns inappropriate values #35

Closed christianAppl closed 3 years ago

christianAppl commented 3 years ago

First of all: This is a nice and simple solution and I really like it!

Issue: Using this fine library I tried to draw a path that contained a point with the Y-coordinate 9.62E-4. In the created SVG the Y-coordinate for said point was set to 9.62 instead, which lead to a unrecognizable Glyph in the result.

Observations: Following this, I tried to find the cause of the issue and ended up playing arround with the class "RyuDouble" at the end. As far as I understood it, Ryu is a method to create a String representing a floating point number (with increased speed and performance, when compared to other methods) and as far as I could see your intention was, to not only implement that method, but to also limit the maximum number of fractional digits to 4.

I tried to pinpoint my issue, by comparing the results of "RyuDouble.doubleToString(double value, int decimals)" to those of a simple DecimalFormat (which is simply dropping fractional digits beyond 4):

public static String doubleToString(double value, int decimals) {
   DecimalFormat decimalFormat = new DecimalFormat("#.#");
   decimalFormat.setMaximumFractionDigits(decimals);

   DecimalFormatSymbols decimalFormatSymbols = new DecimalFormatSymbols();
   decimalFormatSymbols.setDecimalSeparator('.');
   decimalFormat.setDecimalFormatSymbols(decimalFormatSymbols);

   return decimalFormat.format(value);
}

My results were as follows:

>>> LIMIT ALL VALUES TO 4 FRACTION DIGITS 
VALUE: '0'
   RYU_DOUBLE: '0.0'
   DECIMAL_FORMAT: '0'

VALUE: '0.000000000001'
   RYU_DOUBLE: '1.'
   DECIMAL_FORMAT: '0'

VALUE: '-1'
   RYU_DOUBLE: '-1.0'
   DECIMAL_FORMAT: '-1'

VALUE: '-0.0001'
   RYU_DOUBLE: '-1.0E-'
   DECIMAL_FORMAT: '-0.0001'

VALUE: '0.0001'
   RYU_DOUBLE: '1.0E-'
   DECIMAL_FORMAT: '0.0001'

VALUE: '0.01'
   RYU_DOUBLE: '0.01'
   DECIMAL_FORMAT: '0.01'

VALUE: '1000'
   RYU_DOUBLE: '1000.0'
   DECIMAL_FORMAT: '1000'

VALUE: '10000'
   RYU_DOUBLE: '10000.0'
   DECIMAL_FORMAT: '10000'

VALUE: '1000000000'
   RYU_DOUBLE: '1.0E9'
   DECIMAL_FORMAT: '1000000000'

Issues with "RyuDouble":

  1. Some of the results yielded by "RyuDouble.doubleToString" are clearly incorrect or incomplete. (i.e. results for values '0.0001' and '0.000000000001')

  2. I am not entirely sure, whether SVG does support the scientific notation at all!? Hence I did enforce a "raw number format" in my approach.

  3. Even though I could find a recommendation to only use 2 fractional digits in SVG paths, I don't know whether I like the intended loss of precision (4 fractional digits). Is it really necessary to do this? I tried replacing "doubleToString" in "SVGGraphics2D" with my primitive approach - not limiting the maximum number of fractional digits - and the resulting SVG displayed just fine. (If it is necessary, I would rather round values?)

Assumption: I did not read the paper concerning the Ryu method, therefore I can not claim this to be correct: But I would assume, that the combination of the Ryu method and the limitting of the fractional digits may cause the issue here.

jfree commented 3 years ago

Thanks for the report, I'm going to take a closer look.

jfree commented 3 years ago

Your analysis is correct. I'm not sure how to fix this yet, other than to revert the changes from https://github.com/jfree/jfreesvg/pull/30. I would recommend using JFreeSVG 4.1 (which didn't have the RyuDouble code in it) until this is resolved. Or your own modified version of course.

ghost commented 3 years ago

https://github.com/ulfjack/ryu/tree/master/src/main/java/info/adams/ryu

Try dropping in the original RyuDouble code. I believe I removed the DEBUG statements because they should not be necessary for production code, but didn't verify whether there were any side-effects.

jfree commented 3 years ago

I'll do that. I can't see (yet at least) a simple way to add the decimal places limit on the RyuDouble conversion, so I will look for a way to make the whole mechanism pluggable. That way people can use the old DecimalFormat solution, or the RyuDouble, or any implementation they care to provide.

jfree commented 3 years ago

I've committed a change for this, to be included in the next release (version 5.0 because it is an API change). I'd be happy to hear any feedback on the approach. Right now the default conversion is 4 decimal places for geometry values and 6 decimal places for transform values, but I'm considering making the Ryu algorithm the default (with no attempt to limit the decimal places) as it is a lot faster. If smaller output is preferred, it is easy to change back to the limited decimal place version via the setGeomDoubleConverter(DoubleFunction converter) and setTransformDoubleConverter(DoubleFunction converter) methods.

christianAppl commented 3 years ago

Had a quick look into it and as far as I can tell, it does now work like a charm. I also really like the solution via interface "DoubleFunction", more flexibility always is nice. However: Using Ryu as the default is absolutely fine with me. Assuming that it indeed is more performant - I will gladly use it myself.

Above given "standard" solution was given rather for the purpose of providing a temporary workarround and to point out reproducable erroneous results.

As I already learned in another ticket, that the scientific notation should be supported, this does more than resolve this issue. Thank you very much! I am really looking forward to version 5.0! :)

jfree commented 3 years ago

Great, thanks for the feedback.