s-nakaoka / choreonoid

An integrated graphical robotics application framework
http://choreonoid.org
Other
94 stars 58 forks source link

Make VRML parser locale-free #244

Closed arntanguy closed 4 years ago

arntanguy commented 4 years ago

The VRML loader fails to parse floating point numbers when the locale is using coma-separated numbers instead of dot-separated numbers. I ran into this issue in the past, and this was brought back to my attention yesterday my two independant german users (the official separators for decimal points is the coma in german).

See for the original issue: https://github.com/jrl-umi3218/lipm_walking_controller/issues/14#issuecomment-658695844

This is the same problem that Pierre reported a while ago for openhrp3: https://github.com/fkanehiro/openhrp3/pull/144. I applied the same solution here:

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.

To test this you can run with a german locale (assuming you have german locales installed) on any simulation that loads VRML files with floating-point numbers. It should fail with the current master and succeed with the patch.

LC_ALL="" LC_NUMERIC="de_DE.UTF-8" choreonoid simulation.cnoid

However, on my machine, running the above commands leads to a fully working simulation, but the "Scene" view is completely black and I cannot see the robots (it works fine with english locale). This bug happens even without this patch, but just briefly looking at the code, I could not understand why this would happen. I think Niels had a similar issue on his laptop (I'll forward this issue to him too).

s-nakaoka commented 4 years ago

Thank you for letting me know about the problem in the German language environment. I investigated the problem, and I found that the problem was caused by a bit strange behavior of the Qt library. To put it simply, Qt changes the LC_NUMERIC locale of the C runtime library to that of the system configuration, although the C runtime does not change LC_NUMERIC usually. You can find the details of this problem in the following discussion: https://stackoverflow.com/questions/25661295/why-does-qcoreapplication-call-setlocalelc-all-by-default-on-unix-linux

And the best solution to this problem is to reset the locale just after the initialization of Qt, which is described in the Qt manual: https://doc.qt.io/qt-5/qcoreapplication.html#locale-settings

Based on the above information, I have added the code to reset the locale in the application initialization function: ecb46a166359cdbd9b016f81a37520c5f7463a31 I confirmed that the problem is resolved by this modification even in the Germany locale.

I really appreciate your pointing out this issue. This is important in promoting Choreonoid over the world.

However, let me say a few words on my honor. You mentioned "you've been yet another victim of the crazy VRML loader of Choreonoid" in the following post: https://github.com/jrl-umi3218/lipm_walking_controller/issues/14#issuecomment-659091490 Frankly speaking, I think this is an overstatement, and it is very rude expression. I am the original author of the VRML loader, and I think the VRML loader itself is not crazy. It works well in the usual C runtime environment, and other popular libraries like libyaml are also implemented in the same manner. In fact I didn't know that Qt changes the LC_NUMERIC locale and comma is used as a decimal point in the Germany locale. However, the Qt behavior is a bit strange as is discussed in the above link, and how could I find the Germany locale behavior before pointing out it? Given the circumstances, I think your comment is more rude than it needs to be.

Let me point out one more thing. The official repository of Choreonoid was moved to the Choreonoid organization. https://discourse.choreonoid.org/t/announcement-of-github-repository-changes/387 The address of new repository is https://github.com/choreonoid/choreonoid . So please use the new repository when you post something about Choreonoid. (Or you can use the above board.) Thank you for your cooperation in advance.

arntanguy commented 4 years ago

First of all, let me apologize for the harsh comment. I did not intend to be rude, and for that I am sorry. In this particular case, the fault is mostly ours for not pointing out this issue sooner. I've personally seen it pop up more than once over the years, and the first time I came across it, it took me a few hours to realize where this issue was coming from.

I was mainly frustrated in seeing yet some other users wishing to use Dr Caron's LIPMWalking controller struggling with setting up their dynamics simulation (if you look into the issues, you'll see that they are almost exclusively related to the simulation environment). Now I'm well aware that their issues are not exclusively related to choreonoid, but rather the whole stack of software on which these simulations rely on (openrtm, corba, hrpsys, openhrp, etc), and how we distribute them. In its current form, I find it very difficult to reliably provide reliable ways of running dynamic simulation outside of the lab setting. If you have advice on the topic, I'd be interested in discussing this further with you.

Many thanks for your quick response to the issue, and for pointing out the change of remote repository, I'll be sure to report future issues there directly.

s-nakaoka commented 4 years ago

Thank you very much for your understanding. I understand your situation as well. I've also struggled with managing systems involving various software, and it's often difficult to sufficiently mature software you're developing in academic research, so I can understand your difficulties as well.

In fact Choreonoid has been one of such software programs, and it may have caused some inconvenience to the users. Now I have established a company to run the Choreonoid business, and we have to make it a viable business, so I will try to improve the situation. I appreciate this kind of feedback and would like to thank you for your continued support.