search

fkanehiro / openhrp3

Open Architecture Human-centered Robotics Platform
http://www.openrtp.jp/openhrp3
Other
26 stars 34 forks source link

Always use locale-free variant of strtod #144

Closed gergondet closed 4 years ago

gergondet commented 5 years ago

In some environment (e.g. within Choreonoid with a locale that doesn't have "." as a decimal separator), the VRML parser will fail to parse decimal numbers correctly leading to problems that are difficult to diagnostic.

As far as I can see, float parsing happens within the EasyParser class, ultimately using the strtod function. Howver, strtod is locale-dependent so this PR avoids calling it. This is already the case on Windows (for other reasons) so I'm just re-using the Windows implementation for every platform.

In some local benchmarkings I did, the current MSVC version of the function actually out-performs strtod but numerical results are slightly different (the difference is around 1e-11 difference)

gergondet commented 5 years ago

@fkanehiro @k-okada I don't know what is wrong with the tests, I've tried it locally (but I have an Ubuntu 18.04) install and it seems ok.

From the log, it could like the issue is:

FAILURE: Test Fixture Nodes ['hrpsys'] failed to launch
  File "/usr/lib/python2.7/unittest/case.py", line 331, in run
    testMethod()
  File "/opt/ros/indigo/lib/python2.7/dist-packages/rostest/runner.py", line 121, in fn
    self.assert_(not failed, "Test Fixture Nodes %s failed to launch"%failed)
  File "/usr/lib/python2.7/unittest/case.py", line 424, in assertTrue
    raise self.failureException(msg)

But I have no idea why this would fail and how this could relate to the code I changed...

arntanguy commented 4 years ago

Hi @fkanehiro

This issue was brought back to my attention yesterday by Niels at LIRMM using a coma-separated german locale, with the same issues as reported by Pierre. Could we see about getting this merged and why the CI is currently failing?

I also proposed a similar patch for Choreonoid that also uses a copied version of the same VRML loader: https://github.com/s-nakaoka/choreonoid/pull/244

fkanehiro commented 4 years ago

@k-okada Can you solve this CI problem? Due to this problem, some pull requests have not been merged.

k-okada commented 4 years ago

@fkanehiro could you check https://github.com/fkanehiro/openhrp3/pull/149 ? I think this PR pass the CI tests https://travis-ci.org/github/fkanehiro/openhrp3/builds/675115452

fkanehiro commented 4 years ago

@k-okada Thank you for the PR.

@gergondet Please update your PR.

fkanehiro commented 4 years ago

@k-okada BTW, can you please update travis settings of hrpsys-base too?

gergondet commented 4 years ago

@fkanehiro I've rebased this PR on the current master