Patches you may want to backport

[sfan5]

Hi, I'm one of the Minetest developers.
A few months ago we released the first version of Minetest that's no longer based on upstream Irrlicht. We forked because waiting for an upstream release to fix many issues that have been plaguing users for years was no longer feasible.
In the long run we plan to get rid of Irrlicht altogether either by incorporating parts, writing functionality from scratch or using other libraries (e.g. SDL).

Anyway over the course of development we made some fixes that upstream could benefit from so I thought I'd highlight them here:

• SExposedVideoData.X11Window set incorrectly: https://github.com/minetest/irrlicht/co ... 24636b83bc
• Copy constructors inhibit certain optimizations so we removed ones that were not special: https://github.com/minetest/irrlicht/co ... 8f6afc0b55
• GLX failed to initialize on certain systems: https://github.com/minetest/irrlicht/co ... 60a2e3eda6
• Broken bounds check in CMemoryFile: https://github.com/minetest/irrlicht/co ... e095aa7af4
• Resizing broken on macOS: https://github.com/minetest/irrlicht/co ... ca1f6f8ab4
• Compose/deadkey support on X11: https://github.com/minetest/irrlicht/co ... bf8cddc4d5
• COGLES2Driver: screenshots were color-swapped: https://github.com/minetest/irrlicht/co ... 3924feedae
• Missing normals initialization when loading .x meshes: https://github.com/minetest/irrlicht/co ... b3340124c1
• getViewPort was sometimes incorrect: https://github.com/minetest/irrlicht/co ... d0063394f4
• Fixed clipboard on X11 (incl. changes to make it UTF-8): https://github.com/minetest/irrlicht/co ... f7db7e2b33
• Prevent integer overflow/crash in image loaders: https://github.com/minetest/irrlicht/co ... 627f3c1d42
• Mesh animation related fixes: https://github.com/minetest/irrlicht/co ... f9a378ed67 and https://github.com/minetest/irrlicht/co ... dcc10c9a63
• Crash fix in .b3d loader: https://github.com/minetest/irrlicht/co ... ed3edc3a24
• SDL support updated to SDL2: https://github.com/minetest/irrlicht/co ... 9624cf3d61
• Remove SetThreadAffinityMask calls: https://github.com/minetest/irrlicht/co ... 35a171c52d
• COSOperator::getSystemMemory() was broken https://github.com/minetest/irrlicht/co ... cce7152888

[CuteAlien]

Thanks for the patches.

Applied:

• "Resizing broken on macOS" in svn trunk [r6351]
• "Broken bounds check in CMemoryFile" in svn trunk [r6352]
• "Crash fix in .b3d loader" in svn trunk [r6353]
• "Missing normals initialization when loading .x meshes" in svn trunk [r6354]. Changed patch a bit.
• Copy constructors. Partly applied in trunk [r6355]. Bunch of those were in Irrlicht already.
  ~SMD3QuaternionTag, ~SMD3QuaternionTagList and ~IShader also must be removed (since c++11) when depending on default copy and assignment.
  Otherwise they should not be removed, but missing operators added. But in those 3 cases the virtual destructors seemed all unnecessary.
  So I removed them as well now.
  SMaterial operator= can only be removed if you also remove the copy constructor (I did that now, slight extra cost but easier to read code).
  You guys should rewrite that as well or you risk hard to find crashes when you ever set MATERIAL_MAX_TEXTURES_USED != MATERIAL_MAX_TEXTURES.
  matrix4: I remember that one used memcpy for speed reasons (should profile). But also a bit of a c++ problem there.
  Copy assignment is deprecated (which I think means it's still produced, but not guarantee this will stay that way) when there is a copy constructor.
  Now matrix4 has a constructor which is like a copy constructor, but with additional parameter, but which is set to a default-value.
  So not 100% sure if that counts as the kind of copy constructor which deprecates default copy assignment constructor.
  Could use default keyword, but then it breaks with older compilers. So in short - not gonna touch matrix4 for now.
• COSOperator::getSystemMemory() in svn trunk [r6369]
• SExposedVideoData.X11Window set incorrectly and GLX failed to initialize in svn trunk [r6370]
• COGLES2Driver: screenshots were color-swapped in svn ogl-es [r6383]
• Prevent integer overflow/crash in image loaders. Modified it a bit and checking data-size instead of fixed width/height.
  Applied in svn trunk [r6387].

Declined:

• Remove SetThreadAffinityMask calls - still recommended in a 2022 article from Microsoft: https://docs.microsoft.com/en-us/window ... processors
• Compose/deadkey support on X11. I understand what it fixes and that is useful. But it breaks using dead-keys as gaming-keys as it suppresses events needed there. I suspect we have to handle that by only changing .Char as result (which is used for input). But haven't done too much tests yet... for now will stay on TODO, maybe after 1.9 I'll give that one a shot.

[CuteAlien]

Are 1st and 3rd patch about same thing? First alone seems to be missing parts which are in 3rd. Not sure if rename is needed (bad to break user code in libs and it's been named like that a while and not totally wrong as in the other places it's used in CIrrDeviceLinux it actually seems to be an X11 Window. Maybe better to just add a comment). The other check you added is added already in Irrlicht trunk.

[sfan5]

The third patch only goes together the first patch, yes. (so either both or none)
We had code that would use X11Window to set the window icon on Linux, this code worked fine with 1.8 and stopped working with trunk at some point. So in that sense the patch only restored functionality.

[CuteAlien]

Ah, yes. I didn't read it correct last evening (sorry, was minutes before sleeping when I saw the thread). I read it as GLXWindow replacing X11Window and thought it's a rename. Just checked again and see it adds it. Well, will probably need some time to go over them, quite a ton of patches and I'm not that familiar with some of the stuff. But on first quick view most look OK.

[erlehmann]

Thanks for the patches.

[…]

Declined:

• Remove SetThreadAffinityMask calls - still recommended in a 2022 article from Microsoft: https://docs.microsoft.com/en-us/window ... processors

Thank you. I kept telling sfan5 that this patch is wrong, he did not believe it: https://github.com/minetest/irrlicht/issues/94

However, I think there might be room for improvement:

First, the goal that sfan5 actually seemed to have was to prevent a potential rescheduling of a thread. Is that even a performance problem? And if it is, is it possible to pin a thread to the processor it is already on?

Second, the comment in the code does not explain the problem well. It can be that the BIOS should inform the Windows operating system of how timing is measured … but the problem can exist on any system where timing between cores is not synchronized. A lot of these systems are still used. As I quoted in the linked issue, I do own such a system and Linux reports the following for it:

Code: Select all

[    0.351568] TSC synchronization [CPU#0 -> CPU#1]:
[    0.351568] Measured 521224 cycles TSC warp between CPUs, turning off TSC clock.
[    0.351568] tsc: Marking TSC unstable due to check_tsc_sync_source failed

[erlehmann]

Test files for unit tests

Test b3d file I used to crash Minetest before sfan5 patched it.
https://github.com/minetest/irrlicht/issues/70

Test images files with which I crashed Minetest before sfan5 limited all image dimensions to 23000x23000:
32000x32000 JPEG: https://github.com/minetest/irrlicht/issues/67
1111498827×1083981 BMP: https://github.com/minetest/irrlicht/issues/68

The files are attached to the issues I have linked.

I am sorry that I do not have finished unit tests for you.
A lot of stuff was deleted from Irrlicht when it became Irrlichtmt – this included the tests.

[erlehmann]

Correct rounding on x86 with GCC

Quote https://github.com/minetest/irrlicht/pull/83

On a x86 system (could affect other platforms that handle floats this way) the x87 fp unit
stores floats with 80-bit precision, but when those floats are moved to other registers they suffer from
loss of precision and double rounding.

see
https://gcc.gnu.org/wiki/FloatingPointMath
https://gcc.gnu.org/wiki/x87note

The solution to correct those two issues: (1) loss of precision and (2) double rounding, is to have all floating-point operations occur in sse registers.

Note that using SSE for x86 builds with GCC is not necessarily the only possible fix for this issue.

For systems without SSE, you could use -mpc64 to set the 80387 floating-point precision to 64-bit.

[CuteAlien]

Thanks for the tests. Compile flags for 32-bit... uh... let's say I'll keep it in mind, but not the biggest problem I have right now unless there's some important stuff that doesn't work well otherwise.
Still wondering about why images tests for 23000 width/height instead of the product (yeah, I know that's from our code... still got no clue, but haven't though yet much about it. edit: actually not from our code it seems, but from an earlier patch you did).
b3d already applied - seems to work (aka not load now).
Removing tests - bad idea ;-) Seriously, those tests helped me so often already, I really love 'em!
SetThreadAffinityMask - I really didn't dig deeper so far. I do have todo's about replacing timer calls by 64-bit versions which is likely the biggest problem. Also 2 todo's about replacing it by faster versions on Linux (except people can't agree what to use, so I'll have to dig deeper there some day). But not really on my 1.9 list (still trying to avoid delaying release more years - bugs and easy stuff ok - hard stuff will have to wait).