Opened 6 years ago

Last modified 5 years ago

#1325 new defect

[PATCH] Replace ecvt() in FCollada with something else

Reported by: Ben Brian Owned by:
Priority: Nice to Have Milestone: Backlog
Component: Core engine Keywords: patch
Cc: Patch:

Description

FCollada uses the deprecated POSIX function ecvt to convert floats to strings, but it's not available on FreeBSD and it would be nice to have a portable replacement. The result needs to be compatible with the COLLADA schema which uses XML's xs:double type for floats.

Possible solutions:

It's a good idea to avoid ecvt implementations that depend on dtoa, because it's also not available on FreeBSD and there are numerous buggy versions of that code around which apparently break with compiler optimizations.

Attachments (2)

comparison-format (14.6 KB) - added by Markus 5 years ago.
compare some output formattings
canonical-xsdouble.patch (5.1 KB) - added by Markus 5 years ago.
FUStringBuilder.hpp exists twice

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Ben Brian

We could do what OpenCOLLADA does, using their implementation of dtoa. It's fairly readable and the main benefit is we know it works for OpenCOLLADA.

The problem with sprintf and similar, in my opinion, is they're just not exacting enough to meet the spec.

Last edited 6 years ago by Ben Brian (previous) (diff)

comment:2 Changed 5 years ago by Markus

I just copied the FloatToString? and the corresponding append function to a small test program. For me it does not support negative exponents and the format does not conform to the "canonical representation" of the document linked above.

What are these functions actually used for? Only development?

Achieving a "canonical representation" should be possible with some simple math and formating... I will try this WE.

comment:3 Changed 5 years ago by Ben Brian

I don't believe we use these functions for anything at the moment, I assume they are for exporting COLLADA documents or something, but if we do in the future, it would be preferable to not have them broken :)

FWIW, it looks like the FreeBSD port of 0 A.D. copy-pasted some code from a forum, but I don't know if they actually confirmed the license is compatible with FreeBSD, so that's naughty and I also don't know that the code works beyond compiling.

Last edited 5 years ago by Ben Brian (previous) (diff)

Changed 5 years ago by Markus

Attachment: comparison-format added

compare some output formattings

Changed 5 years ago by Markus

Attachment: canonical-xsdouble.patch added

FUStringBuilder.hpp exists twice

comment:4 Changed 5 years ago by Markus

Keywords: patch review added
Summary: Replace ecvt() in FCollada with something else[PATCH] Replace ecvt() in FCollada with something else

comment:5 Changed 5 years ago by Ben Brian

Milestone: BacklogAlpha 14

comment:6 Changed 5 years ago by Ben Brian

Interesting, so it looks like the ecvt implementation only keeps 6 digits and anything on the order of 1e-8 or smaller becomes 0. That's not canonical xs:double but it's valid? I guess we could check if Collada importers can also handle negative exponents without trouble.

comment:7 Changed 5 years ago by Ben Brian

Milestone: Alpha 14Alpha 15

comment:8 Changed 5 years ago by Jorma Rebane

Keywords: review removed

Taking a look at this patch, I think we're really missing the point, which is to print valid numbers (that make sense) into a Collada file, no?

In this case, any value that is ridiculously small (for example, smaller than +/- 0.00001) should be considered as 0.0; For that matter, ridiculous large float values should also be ignored and set to NAN - because they're obviously invalid vertex coordinates.

The goal is not to produce a float print function that works for every universal solution - the goal is to get valid and reasonable output. In this case NAN, +/- 0.00001 to +/i 10000000.0.

In this case, we can also get away with sprintf(buffer, "%d", ...);

comment:9 Changed 5 years ago by leper

Milestone: Alpha 15Backlog
Note: See TracTickets for help on using tickets.