Page MenuHomePhabricator

TinyXML: writing out double value seems to be buggy.
Closed, ResolvedPublic

Description

With the new ScalarType switched from float to double it became apparent that TinyXML has problems with values with many digits after the comma.
This became apparent in the mitkPlanarFigureIOTest. Here the value -1.123456 was written out by TinyXML as -1.12346. This task shall

  1. Reproduce this error with a simple test
  2. Create a patch for TinyXML to handle this problem
  3. Publish this fix to TinyXML

Event Timeline

New remote branch pushed: bug-16328-tinyXML_double_writeout

New remote branch pushed: bug-16328-u6-tinyXML_double_writeout

Two things:

  • the change in TinyXML is not provided as a patch but as a complete replacement of the two files. This is a bit dangerous. Since we do not have proper patch tool available on all platforms we should at least check that the original file is the correct one, e.g. with CMake's fiel(md5 ...) command.
  • from the description, the patch changes the interface of TinyXML in a way that MITK won't compile anymore with an unpatched version. This should be avoided. I would prefer a patch that changes the precision in all cases directly in the implementation. This would bloat the XML a bit for "float-users", but would be more compatible.

I will add the CMake check later.

[8395f5]: Merge branch 'bug-16328-u6-tinyXML_double_writeout'

Merged commits:

2013-10-29 11:36:49 Sebastian Wirkert [d510c1]
Patch to enable customized number of decimal values when writing out doubles.
Search e.g. for requiredDecimalPlaces.


2013-10-25 16:02:37 Sebastian Wirkert [bbc13e]
first completely working version of tests. Now patch for tinyXML can be created.


2013-10-25 15:25:14 Sebastian Wirkert [92b449]
small checkin.


2013-10-25 15:13:38 Sebastian Wirkert [dbb5bd]
changed interface.


2013-10-25 13:10:13 Sebastian Wirkert [d542b3]
First working set of tests.


2013-10-24 15:59:44 Sebastian Wirkert [5283d2]
Created test to show the problem.

[667b3f]: COMP: Merge branch 'bug-16328-u6-tinyXML_double_writeout'

Merged commits:

2013-10-29 18:00:13 Sebastian Wirkert [377a7d]
Merge branch 'bug-16328-u6-tinyXML_double_writeout' of mitk.org:MITK into bug-16328-u6-tinyXML_double_writeout


2013-10-29 17:55:20 Sebastian Wirkert [c139b4]
COMP: Removed test for extended SetDoubleValue signature because this uses

the new, changed interface, leading to compile errors if the normal version
of tinyXML is used.

2013-10-29 17:55:20 Sebastian Wirkert [9f30a6]
Removed test for extended SetDoubleValue signature because this uses
the new, changed interface, leading to compile errors if the "normal" version
of tinyXML is used.

Current release is finished. Reseting target milestone...

Sebastian you already made some merge commits. Is this fixed? Can we close this bug?

I think Marco wanted to add some check to verify that the correct file has been replaced. From my side it's finished.

(In reply to Sebastian Wirkert from comment #9)

I think Marco wanted to add some check to verify that the correct file has
been replaced. From my side it's finished.

Yes, now I remember, this refers to comment #3

As far as I can see, simply the check for the correct file is missing?
Sebastian, can you take care of this until next wednesday? I think this really should be fixed for the upcoming release.

A md5 check in the Patchtinyxml-2.6.2.cmake script should be easy to do. I could help with that on Wednesday.

Sebastian can you please take care of that today?

User wirkert has pushed new remote branch:

bug-16328-Add_md5_check_for_tiny_xml_patch

[a78bab]: Merge branch 'bug-16328-Add_md5_check_for_tiny_xml_patch'

Merged commits:

2014-11-05 14:11:45 Sebastian Wirkert [3320c1]
Added check if tinyxml patch is applied to correct version of tinyxml (2.6.2)
by checking md5sum.