Maniphest T24396

Vertices coordinates after exporting to COLLADA are wrong
Closed, Resolved

Assigned To
Jeroen Bakker (jbakker)
Authored By
Sv. Lockal (lockal)
Oct 25 2010, 10:42 PM
Tags
  • Platform: Linux
  • BF Blender
Subscribers
Jeroen Bakker (jbakker)
Nathan Letwory (jesterking)
Ralf Hölzemer (cheleb)
Sv. Lockal (lockal)

Description

Vertices coordinates are wrong after exporting to COLLADA even with a default cube.

Steps to reproduce:
1) Open Blender
2) File -> Export -> Collada (.dae) (export the default cube)
3) File -> Import -> Collada (.dae) (import the same file)
-> get the huge stretched cube.

Expected: get the same cube as it was before export.

Inside exported *.dae file you can see the line:
<float_array id="Cube-mesh-positions-array" count="24">1 1000000 -1 1 -1 -1 -1 -1000000 -1 -1000000 1 -1 1 1000000 1 1000000 -1.000001 1 -1 -1000000 1 -1000000 1 1</float_array>

which should be:
<float_array id="Cube-mesh-positions-array" count="24">1 1 -1 1 -1 -1 -1 -1 -1 -1 1 -1 1 1 1 1 -1 1 -1 -1 1 -1 1 1</float_array>

Tested with Blender 2.54.0 r32707 (Ubuntu 10.04/10.10, GeForce GTX 275).

Event Timeline

Sv. Lockal (lockal) edited a custom field.Oct 25 2010, 10:42 PM
Sv. Lockal (lockal) attached 1 file(s): F11246: uncube.dae.
Nathan Letwory (jesterking) added a comment.Oct 26 2010, 1:06 AM

This seems to happen only on Linux systems, and freakabcd did some investigation into the matter, and IIRC it had to do somehow with the function for conversion the floats to strings.

Nathan Letwory (jesterking) added a comment.Nov 5 2010, 11:44 PM

Is this 32bit or 64bit linux?

Sv. Lockal (lockal) added a comment.Nov 6 2010, 11:58 AM

32-bit ubuntu with packages from https://edge.launchpad.net/~cheleb/+archive/blender-svn

Self-compiled version of libftoa from svn passes all included tests, except for last test for doubles
don't match 0.9999999999999999: 0.999999999999999888977 and 0.9999999999999998

Sv. Lockal (lockal) added a comment.Nov 9 2010, 10:10 PM

After having recompiled blender-32970 on my home machine I could not reproduce this bug anymore.

I have compiled using scons with preferences
WITH_BF_COLLADA = True
BF_OPENCOLLADA = '/home/lockal/bin/opencollada'

I have reported the PPA owner Ralf Hölzemer about this bug and hope the investigation will have a positive outcome.

Ralf Hölzemer (cheleb) added a comment.Nov 9 2010, 10:54 PM

Hi Lockal,
guy with the PPA on Launchpad here.

First, thanks for the heads up. I just tried to reproduce this error with the PPA build from today (32970) and couldn't find any issue here on my 64bit Maverick system.

Maybe this was some general error that was fixed recently?

Sv. Lockal (lockal) added a comment.Nov 10 2010, 11:50 AM

Hi Ralf Hölzemer, this bug is still reproduceable with 32970 from ppa.

Just tried ppa version with fresh install of 32-bit Ubuntu 10.10 on VirtualBox with no success: still get 1000000 instead of 1.000000.

Jeroen Bakker (jbakker) added a comment.Dec 3 2010, 12:15 PM

Hi Lockal S

I am trying to isolate the bug. With the latest attempt did you also did an custom compile of libftoa?

We have a hard time reproducing this issue and you are the only one reported this issue. Is this issue also happing to you when you download the 2.55 32 Linux binary build from blender.org?

Regards,
Jeroen Bakker.

Sv. Lockal (lockal) attached 1 file(s): F11250: fabs.diff.Dec 4 2010, 4:29 PM

It's seems I have found the solution. Btw, Collada export works fine with binary build from blender.org, also it works fine with -O0 build of OpenCollada. It even works well if I add printf("%f", f); to ftoa (looks like compiler cant inline or do other optimisations for f, so that hides this problem). So I added
printf("copyToBufferAsChar %x %.12f %s\n", *((unsigned int*)&f), f, getCurrentPosition());
after ftoa call (line 68 in common/libBuffer/src/CommonCharacterBuffer.cpp) and got something like this:
copyToBufferAsChar 3f7fffff 0.999999940395 1000000
copyToBufferAsChar bf7ffffd -0.999999821186 -1000000
copyToBufferAsChar bf7ffffa -0.999999642372 -1000000

I still don't understand, how libftoa works, because it should not work at all.

In common/libftoa/src/Commonftoa.cpp at line 56 you can see
float fabs = abs(f);

and few lines later
if ( fabs < 0.00001 )

abs implicitly converts its argument to int, so abs(-0.999999821f) is 0, but not 0.999999821. Probably fabs is the function, which should be used here. Same applies to Commondtoa.cpp

Patch is attached to this message. It resolves the problem for me (I also hope this case is not like printf line described above).

Jeroen Bakker (jbakker) added a comment.Dec 4 2010, 7:10 PM

Oke,

Thanks, will review the patch, and if oke will let Nathan commit it.

Thank you very much for the effort!
Jeroen

Jeroen Bakker (jbakker) added a comment.Dec 5 2010, 8:47 AM

Lockal,

the abs function does not convert an float to an int,
it convert an negative float to a positive float.

don't think you have solved the actual issue.

Jeroen.

Sv. Lockal (lockal) added a comment.Dec 5 2010, 12:28 PM

If you use <math.h> and <stdlib.h> and do not know the difference between abs and fabs, please read
http://www.kernel.org/doc/man-pages/online/pages/man3/abs.3.html
http://www.kernel.org/doc/man-pages/online/pages/man3/fabs.3.html

Also you can use gdb disassembler to see the difference.

Also you can use flag -fdump-tree-optimized to see the code IR:
fabs = (float) ABS_EXPR <(int) f>;

Also you can use -Wconversion flag to see warnings about imlicit conversions:
common/libftoa/src/Commonftoa.cpp: In function 'float Common::roundingSummand(float, int, int&)':
common/libftoa/src/Commonftoa.cpp:56: warning: conversion to 'int' from 'float' may alter its value
common/libftoa/src/Commonftoa.cpp:56: warning: conversion to 'float' from 'int' may alter its value

Btw, there are many double2float and float2double implicit conversions in collada exporter, but those are not so important.

Jeroen Bakker (jbakker) added a comment.Dec 10 2010, 8:09 AM

I have to admit, that the collada method only working for numbers |x| >= 1.0. Smaller nummers don't get to the expected if check. Will clean up the path and send to opencollada project. Don't see any harm in the fix.

Can you check if the attached fix also work for you before I send it to opencollada?

Jeroen Bakker (jbakker) added a comment.Dec 10 2010, 8:12 AM

Index: common/libftoa/src/Commondtoa.cpp
===================================================================
--- common/libftoa/src/Commondtoa.cpp (revision 785)
+++ common/libftoa/src/Commondtoa.cpp (working copy)
@@ -50,7 +50,7 @@


double roundingSummand(double d, int maxLength, int& dezmialPointPos)

{

- double fabs = abs(d);

+ double fabs = fabs(d);

dezmialPointPos = 0;



if ( fabs < 0.00000000000001 )

Index: common/libftoa/src/Commonftoa.cpp
===================================================================
--- common/libftoa/src/Commonftoa.cpp (revision 785)
+++ common/libftoa/src/Commonftoa.cpp (working copy)
@@ -53,7 +53,7 @@


float roundingSummand(float f, int maxLength, int& dezmialPointPos)

{

- float fabs = abs(f);

+ float fabs = fabs(f);

dezmialPointPos = 0;



if ( fabs < 0.00001 )

Sv. Lockal (lockal) added a comment.Dec 10 2010, 5:10 PM

Nope, your patch is broken, you should use other name for fabs variable.

Commonftoa.cpp: In function ‘float Common::roundingSummand(float, int, int&)’:
Commonftoa.cpp:56: error: ‘fabs’ cannot be used as a function

Jeroen Bakker (jbakker) added a comment.Dec 15 2010, 12:54 PM

http://code.google.com/p/opencollada/issues/detail?id=148

fix is patched in Collada revision 786. I see in their source that they have left the name.

http://code.google.com/p/opencollada/source/browse/trunk/common/libftoa/src/Commonftoa.cpp?r=786

Sv. Lockal (lockal) added a comment.Dec 15 2010, 6:26 PM

Variable name was changed in r788.
http://code.google.com/p/opencollada/source/detail?r=788

I think this problem can be marked as resolved and fixed now.

Nathan Letwory (jesterking) added a comment.Dec 20 2010, 10:32 AM

Has been fixed upstreams. I will update the libraries for win32 and win64 right away.

Closing.

Nathan Letwory (jesterking) changed the task status from Unknown Status to Resolved.Dec 20 2010, 10:32 AM
Nathan Letwory (jesterking) added a comment.Dec 20 2010, 11:51 AM

r33806 and r33807