Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class RyuDouble returns inappropriate values #35

Closed
christianAppl opened this issue Mar 16, 2021 · 7 comments
Closed

Class RyuDouble returns inappropriate values #35

christianAppl opened this issue Mar 16, 2021 · 7 comments

Comments

@christianAppl
Copy link

christianAppl commented Mar 16, 2021

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
Copy link
Owner

jfree commented Mar 17, 2021

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

@jfree
Copy link
Owner

jfree commented Mar 19, 2021

Your analysis is correct. I'm not sure how to fix this yet, other than to revert the changes from #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
Copy link

ghost commented Mar 25, 2021

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
Copy link
Owner

jfree commented Mar 27, 2021

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 added a commit that referenced this issue May 23, 2021
@jfree
Copy link
Owner

jfree commented May 23, 2021

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
Copy link
Author

christianAppl commented May 25, 2021

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
Copy link
Owner

jfree commented May 30, 2021

Great, thanks for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants