Random error in margin calculation

[C-Core]

We have integrated RmlUi 6.1 in our engine and are using a stripped down version of the rmlui demo in our unit tests. Sometimes our image comparisons fail and we are see that the margins of the title text is not calculated correctly (seems to happen more often on linux than on windows):

This is how it looks

This is how it should look:

Do you have any idea what could cause these random inconsistencies?

Attached is our stripped version of the demo files:

Data.zip

[mikke89]

Hm, interesting. I don't immediately have a good idea about this. I haven't observed it myself.

Since it is a bit random, maybe it could be some unintialized variables being read? Are you able to run the sample through a tool like valgrind? It's very curious that it affects so many different elements in such a consistent manner.

Maybe you could dump the resulting boxes of these elements when it gives the wrong look? I.e. element->GetBox() for the h1 elements and paragraphs. It could be a place to start at least.

[C-Core]

Thanks for the fast response, took some time to get the data.

These are the first few margin boxes in the broken case:
Element: h1 - Box margin pos: 0.00, -9.00 - Box margin size: 250.00, 19.80
Element: p - Box margin pos: 0.00, 0.00 - Box margin size: 250.00, 18.00
Element: h1 - Box margin pos: 0.00, -9.00 - Box margin size: 250.00, 19.80
Element: p - Box margin pos: 0.00, 0.00 - Box margin size: 250.00, 27.00
Element: h1 - Box margin pos: 0.00, -9.00 - Box margin size: 250.00, 19.80
Element: p - Box margin pos: 0.00, 0.00 - Box margin size: 250.00, 45.00
Element: p - Box margin pos: 0.00, 0.00 - Box margin size: 250.00, 18.00
Element: h1 - Box margin pos: 0.00, -9.00 - Box margin size: 250.00, 19.80

And this is how it should look like:
Element: h1 - Box margin pos: 0.00, -12.60 - Box margin size: 250.00, 29.70
Element: p - Box margin pos: 0.00, -3.75 - Box margin size: 250.00, 25.50
Element: h1 - Box margin pos: 0.00, -12.60 - Box margin size: 250.00, 29.70
Element: p - Box margin pos: 0.00, -3.75 - Box margin size: 250.00, 34.50
Element: h1 - Box margin pos: 0.00, -12.60 - Box margin size: 250.00, 29.70
Element: p - Box margin pos: 0.00, -3.75 - Box margin size: 250.00, 52.50
Element: p - Box margin pos: 0.00, -3.75 - Box margin size: 250.00, 25.50
Element: h1 - Box margin pos: 0.00, -12.60 - Box margin size: 250.00, 29.70

The height is consistently smaller in the broken case. There are also these warnings in the broken case which do NOT appear when it looks good:

Warning: Syntax error parsing property declaration 'transition: top left 0.6s back-out, opacity 0.4s, image-color 0.4s 0.3s quadratic-out;' in Data/demo.rcss: 329.
Warning: Syntax error parsing property declaration 'transition: image-color 0.3s 0.0s quadratic-out;' in Data/demo.rcss: 343.
Warning: Syntax error parsing property declaration 'transition: top 0.4s back-out, opacity 0.4s cubic-in;' in Data/demo.rcss: 350.

But I don't know if they are related.

We haven't managed to run with valgrind yet but we will try that soon.

[C-Core]

Turns out to be a locale issue.
We had some other code that set the locale to german. In german we use comma instead of dot for decimal numbers, so in this case sscanf failed to parse all floating point numbers.
Since rcss expects floating points with a dot maybe the stylesheet parser should set the locale to the "C" locale and after it is done parsing set it back to whatever it was before?
What do you think?

[mikke89]

Oh, I see, that's a classic one.

We don't support changing the locale, and generally I think it's just a bad idea altogether to change it globally. There's just too much potential for breakage. Our formatting surface area is just too large to to try to protect it against locale-settings, it's not something we want to support.

One thing we could consider is to add a debug check during e.g. Context update, and emit a warning with improper locale settings. Although, I am a bit concerned there could be many false positives if we checked for C locale, since many other locales would be compatible.

[C-Core]

Agreed that it is a bad idea to change it globally, but sometimes it is not in your control since other thirdparty libaries do it. In our case it was Qt (which we use for our Editor and Testframework UI).
I see that it would be difficult for RmlUi to cover all cases. Do you have any plans to change to locale-independent parsing functions? E.g. in our engine we have written our own parsing for numbers, it is not that much code.

Maybe just do a small sscanf with a floating point number in debug builds and see whether it gives you the correct result, if not emit a warning? I think this would be more robust than checking for the C locale.

[mikke89]

I understand, sometimes it is outside of your control. Although I would argue it's an issue in the other library if it doesn't provide a way to control that. I thought Qt had their own global locale setting?

I added a commit just now to detect wrong locale at least, let me know if it looks okay to you.

Do you have any plans to change to locale-independent parsing functions?

I'd love to do that, but the situation in C++ isn't great right now. The only real viable std library options is to use std::to_chars and std::from_chars. I think we can bump to C++17 very soon, so in theory we'll then have access to those. However, particularly from_chars of float has really bad library support. Only just very recently did libc++ get support for that.

Did you actually implement this yourselves for floating-point too?

[C-Core]

Yes, we could easily fix it by using the locale settings in Qt once we found the issue.

Your check looks good. I think this will already help a lot if people are seeing unexpected results due to the wrong locale.

In know C++ is horrible in this regard. Had the same issue a while ago at work and wanted to use std::to_chars etc. We were on C++17 already but e.g. android just didn't implement it.

We wrote all of it by ourselves to have full control. Here is the string to float conversion for example, feel free to get inspired ;)
https://github.com/ezEngine/ezEngine/blob/0f0acd4c8c62593a3699c8367cf875552b0c8ce5/Code/Engine/Foundation/Utilities/Implementation/ConversionUtils.cpp#L162

And here is the reverse (note that this is not as self contained as the above since it is part of our own string formatting system which is a bit more involved)
https://github.com/ezEngine/ezEngine/blob/0f0acd4c8c62593a3699c8367cf875552b0c8ce5/Code/Engine/Foundation/Strings/Implementation/snprintf.cpp#L788

So from my side you can consider this issue as resolved.

[mikke89]

Glad to hear. You're not the first to encounter this, so it's good we have a check now at least.

Very interesting to see your implementation. I only loosely followed the std implementations, and it sounded like it was quite tricky. That does look quite reasonable though. I'll keep it in mind, maybe take some inspiration if I find some extra time for it. Thanks for letting me know about it!

I am closing this issue then, cheers!