[NOSQUASH] Move stuff from renderingengine

[Desour]

Moves the platform dependent stuff from renderingengine.cpp. Also implements the stuff for SDL (2nd commit).
This fixes the remaining point in #137.

I don't know how to implement getDisplayDensity properly. SDL_GetDisplayDPI doesn't seem to be what we need. Please help.

FYI, this doesn't break anything if merged without minetest/minetest#13348. (The X11 setup stuff is done twice, but that doesn't seem to hurt.)

(The X11 class name and the windows icon thing are hardcoded to minetest's values. I don't think passing the values to irrlicht is worth doing, as we're planning to drop non-SDL backends anyway.)

It would be nice if someone could test this with Windows.

[sfan5]

I don't know how to implement getDisplayDensity properly. SDL_GetDisplayDPI doesn't seem to be what we need. Please help.

Why not?

[Desour]

I'm not entirely sure.
SDL2 queries the display size in pixels and mm with some xrandr functions (XRRGetOutputInfo and XRRGetCrtcInfo), we use DisplayHeight and DisplayHeightMM.
The resulting values are different on my machine.
(Idk what it does for non-X11 devices.)

It's also worth noting that SDL_GetDisplayDPI does no longer exist in SDL3 (https://github.com/libsdl-org/SDL/blob/main/docs/README-migration.md):

• SDL_GetDisplayDPI() - not reliable across platforms, approximately replaced by multiplying display_scale in the structure returned by SDL_GetDesktopDisplayMode() times 160 on iPhone and Android, and 96 on other platforms.

(SDL_DisplayMode doesn't yet have display_scale in SDL2.)

I'm also not sure what RenderingEngine::getDisplayDensity() is actually supposed to return. And I don't know much about dpi stuff.

Should we maybe just assume a logical density of 96 screen coordinates per inch, and return the logical ppi density?
(It would work well enough on my machine (current code assumes 100 logical ppi or so), but maybe our function is supposed to query the actual physical screen size.)

[numberZero]

And I don't know much about dpi stuff.

That stuff is way more complicated than it needs to be. In a nutshell,

• DisplayHeightMM returns DisplayHeight/3.78 on recent X.Org versions
• On X11 and Wayland, SDL_GetDisplayDPI returns actual screen unit to inch ratio (assuming no bad configuration)
• On X11, screen unit is exactly pixel (even if scaling is used: X.Org just scales the whole framebuffer)
• On Wayland, if you don’t set SDL_WINDOW_ALLOW_HIGHDPI the behavior is essentially the same, application-wise
• On Wayland, if you do set SDL_WINDOW_ALLOW_HIGHDPI, screen unit may not be the same as pixel; you can query window size in screen units with SDL_GetWindowSize and the same size in real pixels with SDL_GL_GetDrawableSize

I don’t know how does that work on other platforms.

Also, there are things like “font DPI,” “UI DPI,” etc. which aren’t DPI at all but rather, scale factors.

Should we maybe just assume a logical density of 96 screen coordinates per inch

That is what the current code does on X11 effectively. Here is the result:

I'm also not sure what RenderingEngine::getDisplayDensity() is actually supposed to return.

UI&font scaling factor, I suppose. That should be a user preference and not a function of DPI, but getting it is highly platform-dependent (e.g. Qt and GTK have different ways for that) so simply returning actual_dpi/96 is likely a good idea.

[Desour]

Thanks for the explanations!

To get the same results as we get on non-SDL backends, I've decided to take window size / drawable size, as this is the same as in the X11 backend, according to:

DisplayHeightMM returns DisplayHeight/3.78 on recent X.Org versions

---

That should be a user preference and not a function of DPI, but getting it is highly platform-dependent (e.g. Qt and GTK have different ways for that) so simply returning actual_dpi/96 is likely a good idea.

Minetest also has its display_density_factor setting. Maybe we could in the future add special values for this setting (i.e. 0, -1, or -2) to use the physical dpi or SDL3's display_scale.
For now I've decided against SDL_GetDisplayDPI, because it would scale differently on different IrrDevice backends, and because it will be removed in SDL3.

---

(Also rebased.)

---

This is ready for review now.

Someone should test with a windows device.

[numberZero]

Tested.

Window icon

X11: still works on DeviceLinux, and with minetest/minetest#13348 it now also works on DeviceSDL.
Wayland: no changes; SDL apparently doesn’t support window icon yet, so no luck here.

Display density

X11: no changes; that’s expected.
Wayland: no changes either. I guess it’s missing SDL_WINDOW_ALLOW_HIGHDPI.

[Desour]

I guess it’s missing SDL_WINDOW_ALLOW_HIGHDPI.

This should be done in another PR. I wouldn't even be able to test it.

Thanks for testing!

[sfan5]

should take an IImage IMO

[Desour]

Done.

Note though that win32 will now also require the (unused by win32) icon image file to work.

[sfan5]

As of now that doesn't work, the file is not installed on Windows.
(assuming you didn't change that in the relevant PR)

[Desour]

(Windows installations would also need the file for the SDL driver.)

assuming you didn't change that in the relevant PR

I didn't. Idk how to fix this, and I couldn't test.

[numberZero]

btw here is my untested version of copyToNoScaling. not that it feels related to this PR... though after rebase, no one would see which commits came from a single PR.

bool CImage::copyToNoScaling(void *target, ECOLOR_FORMAT format, u32 pitch) const
{
	if (IImage::isCompressedFormat(Format))
	{
		os::Printer::log("IImage::copyToNoScaling method doesn't work with compressed images.", ELL_WARNING);
		return false;
	}

	if (!target || !Size.Width || !Size.Height)
		return false;

	const u32 bpp=getBitsPerPixelFromFormat(format)/8;
	if (0==pitch)
		pitch = Size.Width*bpp;

	if (Format == format && pitch == Pitch) {
		memcpy(target, Data, (size_t)Size.Height*pitch);
		return true;
	}

	if (!CColorConverter::canConvertFormat(Format, format))
		return false;

	u8* tgtpos = (u8*) target;
	const u8* srcpos = Data;
	for (u32 y = 0; y < Size.Height; ++y) {
		CColorConverter::convert_viaFormat(srcpos, Format, Size.Width, tgtpos, format);
		tgtpos += pitch;
		srcpos += Pitch;
	}
	return true;
}

[Desour]

Fixed the comment.
I'd like to keep the copyToNoScaling implementation simple for now (it's curretnly just copied from copyToScaling).
The image class will probably be reworked at some point anyways.

[SmallJoker]

Technically speaking, those names do not belong to the framework. However, IrrlichtMt is basically modified to fit to Minetest's needs.

Thing is - I would like to use IrrlichtMt over stock Irrlicht for an unrelated project to benefit from the well-maintained code. However, if keeping it generalized is too much effort, I would understand that decision as well.

[Desour]

I have strong doubts about the usefulness of using irrlichtmt for another application, as there will always be annoying breaking changes for you.

But if you do still want to do this, you can also just choose the SDL driver, it takes these names from the executable's filename.

=> It's not worth doing imo, sorry.

[SmallJoker]

Tested on Linux/X11 (the classic) with the Minetest PR and could not find any issues. Also based on the buildbot results and numberZero's testing I will give an approval after clarifying the comment above.

[SmallJoker]

Works.

[sfan5]

LGTM