Opened 12 years ago

Closed 3 years ago

Last modified 3 years ago

#1325 closed defect (fixed)

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

Reported by: historic_bruno Owned by: Stan
Priority: Nice to Have Milestone: Alpha 24
Component: Core engine Keywords: patch
Cc: Patch: Phab:D2399

Description (last modified by nf)

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 11 years ago.
compare some output formattings
canonical-xsdouble.patch (5.1 KB ) - added by Markus 11 years ago.
FUStringBuilder.hpp exists twice

Download all attachments as: .zip

Change History (15)

comment:1 by historic_bruno, 12 years ago

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 12 years ago by historic_bruno (previous) (diff)

comment:2 by Markus, 11 years ago

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 by historic_bruno, 11 years ago

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 11 years ago by historic_bruno (previous) (diff)

by Markus, 11 years ago

Attachment: comparison-format added

compare some output formattings

by Markus, 11 years ago

Attachment: canonical-xsdouble.patch added

FUStringBuilder.hpp exists twice

comment:4 by Markus, 11 years ago

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

comment:5 by historic_bruno, 11 years ago

Milestone: BacklogAlpha 14

comment:6 by historic_bruno, 11 years ago

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 by historic_bruno, 11 years ago

Milestone: Alpha 14Alpha 15

comment:8 by Jorma Rebane, 11 years ago

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 by leper, 10 years ago

Milestone: Alpha 15Backlog

comment:10 by nf, 4 years ago

Description: modified (diff)

Hi folks,

I have successfully built fcollada using

https://code.wildfiregames.com/P186

It uses musl libc (MIT licensed) implementation of ecvt(). Let me know what you think.

comment:11 by elexis, 4 years ago

Milestone: BacklogWork In Progress
Patch: Phab:D2399

comment:12 by Stan, 3 years ago

Owner: set to Stan
Resolution: fixed
Status: newclosed

In 24323:

Fix FCollada on platforms without ecvt

Based on a patch by: @NF
Tested by: @nephele
Tested on FreeBSD, Windows, Kubuntu, macOS
Fixes: #1325
Differential Revision: https://code.wildfiregames.com/D2399

comment:13 by Stan, 3 years ago

Milestone: Work In ProgressAlpha 24
Note: See TracTickets for help on using tickets.