ValueTree->xml double issue


#1

I use xml generated from a ValueTree structure to store plugin preset data and am currently reworking my code to deal with using the correct types again after a restore. In the process I ran into a related issue. I wrote a short routine to show the problem:

const Identifier pid("prop");

ValueTree tst("Test");
tst.setProperty(pid, 1e+40, nullptr);
String xml = tst.toXmlString();
DBG(xml);

XmlDocument xmlDoc(xml);
ScopedPointer<XmlElement> xmlElem = xmlDoc.getDocumentElement();
auto fromXml = ValueTree::fromXml(*xmlElem);

double val = fromXml[pid];
DBG(val);

On Juce tip, OSX this is the output:

JUCE v5.4.2
<?xml version="1.0" encoding="UTF-8"?>

<Test prop="10000000000000000303786028427003666890752.000000"/>

1e+17

It seems serializing a double for some reason is capped at 10ˆ17. When I check the ValueTree in the debugger I see that the value is correct (double, 1e+40), but something in toXmlString() seems off.
It would be nice if toXmlString() would use the scientific format for large numbers (and small ones while we’re at it). I believe I’ve seen that in the past from JUCE, but maybe I was dreaming… or something has broken. The code can definitely parse the scientific format correctly, so this issue seems like some bug or oversight.


#2

10000000000000000303786028427003666890752.000000 contains 41 characters before the ., so it is close to 1e+40, not 1e+17. The serialization works, toXmlString() is fine.

What doesn’t work is double val = fromXml[pid], more precisely double val = String("10000000000000000303786028427003666890752.000000").getDoubleValue() (i.e., the XML format is not involved in the issue).

This is because juce::CharacterFunctions::readDoubleValue assumes that a double can have a maximum of 18 significant digits:

The smallest double with 18 significant digits is 1e+17.


#3

maxSignificantDigits was introduced by @t0m in https://github.com/WeAreROLI/JUCE/commit/7e6a650e8c35e2459abb73949b1198ee5aa15870 (between 5.0.2 and 5.1.0).

I tested with JUCE 5.0.0, and I got 1e+40 when looking at val.


#4

This is so wrong :man_facepalming:
1e+17 has a single signifiant digit: 1.

This sentence is also incorrect. Assuming that a double can have a maximum of 18 significant digits is totally fine. The issue comes from assuming that the string representation of a double cannot be longer than 18 characters:


#5

Ok … but this works:

double v = String("1e40").getDoubleValue();

So IMHO the toXmlString() method is still a problem (and wastes a lot of space with garbage digits). If toXmlString() would use scientific format things would be fixed. At the same time, the 17 digit assumption is bad for the decimal format.


#6

Ouch.

I’m surprised that bug has survived so long. Those double parsing changes were made a year and a half ago! People must not be reading very long double strings very often…

Here’s a fix. I’ll cherry-pick it onto the master branch when it’s had a few days of exposure to the outside world.


#7

Thanks for the fix! Now very large numbers like 10000000000000000303786028427003666890752.000000 get correctly parsed. However, the toXmlString() method remains unaffected and still does not use the scientific notation. This also leads to another problem I found in the meantime. If you store a very small double number like 3.7e-26, it gets written as 0.

ValueTree tst("Test");
tst.setProperty(Identifier("prop"), 3.7e-27, nullptr);
String xml = tst.toXmlString();
DBG(xml);

produces

JUCE v5.4.2
<?xml version="1.0" encoding="UTF-8"?>

<Test prop="0.00000000000000000000"/>

This seems weird as juce::String(3.7e-27) works correctly (it uses the standard library for these values).

The problem boils down to StackArrayStream::writeDouble which is used for very small and large values.

    size_t writeDouble (double n, int numDecPlaces)
    {
        {
            std::ostream o (this);

            if (numDecPlaces > 0)
            {
                o.setf (std::ios_base::fixed);
                o.precision ((std::streamsize) numDecPlaces);
            }

            o << n;
        }

        return (size_t) (pptr() - pbase());
    }

As the toXmlString() function uses numDecPlaces=20 this sets the std::ios_base::fixed flag which disables scientific notation. In a case where scientific notation is shorter than numDecPlaces, I don’t think the fixed flag should be set. Also I don’t get why xml output needs numDecPlaces to be 20.

To be honest I don’t understand why Juce even bothers doing those string<->double conversions in its own code. Why not just use the standard library conversion routines always?


#8

To retain full accuracy when converting back and forth. Clearly there were some problems with this, which the following commit addresses:


#9

thanks for the fixes. I hope I’m not annoying, but unfortunately, my example is still problematic

ValueTree tst("Test");
tst.setProperty(Identifier("prop"), 3.7e-27, nullptr);
String xml = tst.toXmlString();
DBG(xml);

still outputs prop=“0.00000000000000000000”

the ValueTree::toXmlString() method doesn’t go through XmlElement::setAttribute where you applied the fix, but instead uses the Var::toString() conversion for double which has scientific notation disabled and numDecimalPlaces set to 20.


#10

Ack, I missed that one. Thought my tests would go through that path…

Thank you for your persistence!