Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Irrlicht + SDL 2 #10738

Closed
v-rob opened this issue Dec 22, 2020 · 42 comments
Closed

Irrlicht + SDL 2 #10738

v-rob opened this issue Dec 22, 2020 · 42 comments
Labels
@ Client / Controls / Input Concept approved Approved by a core dev: PRs welcomed! High priority Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature

Comments

@v-rob
Copy link
Contributor

v-rob commented Dec 22, 2020

Problem

SDL 2 is a very useful, well-supported, and consistently cross-platform library. Irrlicht, on the other hand, not so much in many ways. SDL has many features that Irrlicht doesn't do so well with, especially SDL_Event versus irr::SEvent. We have lot of problems with Irrlicht's system, such as the lack of touch input, inconsistent joystick events, no X scroll or side mouse buttons, non-QWERTY keyboards, etc. SDL 2, on the other hand, is well-structured and has quite a few extra useful features. In addition, SDL works much better as a windowing system than Irrlicht, e.g. properly handling fullscreen.

Solution

Combine SDL 2 with Irrlicht. Now, there was an IRC discussion a few days ago wondering if that's even possible. If Irrlicht and SDL are standalone and separate, then probably no, but that's not the case. I've looked into it and have found that Irrlicht has a way to use SDL internally by compiling it with SDL (_IRR_COMPILE_WITH_SDL_DEVICE_). Unfortunately, this is <= SDL 1.2, not SDL 2. If we were to use our own Irrlicht fork instead, we could probably modify it to use SDL 2 without too much of a problem.

At that point, find a way to hook directly in to SDL's event and window system, and slowly transfer to using it instead of SEvent. Obviously, it wouldn't work to do it all at once (for instance, Irrlicht's whole GUI uses SEvent), but we could replace individual parts with SDL.

I'm not suggesting we replace Irrlicht with SDL in any way but to combine the two, SDL as our input and windowing system and Irrlicht for all the rendering. It would also finally give us an excuse to start using our Irrlicht fork and fix more things at the source.

Alternatives

Continue using Irrlicht for everything and working around its problems.

@v-rob v-rob added Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature @ Client / Controls / Input labels Dec 22, 2020
@randomMesh
Copy link

If we were to use our own Irrlicht fork instead

The soon to be released Irrlicht 1.9 has a lot of improvements and bug fixes, so i'd wait and fork that instead.
See https://sourceforge.net/p/irrlicht/code/HEAD/tree/trunk/changes.txt

@v-rob
Copy link
Contributor Author

v-rob commented Dec 22, 2020

The soon to be released Irrlicht 1.9 has a lot of improvements and bug fixes, so i'd wait and fork that instead.
See https://sourceforge.net/p/irrlicht/code/HEAD/tree/trunk/changes.txt

I think (?) there is (or was) some reason we aren't using Irrlicht 1.9, but I'm not sure. Regardless, I doubt it would make a huge difference whether we started with one or the other, and who knows when 1.9 will be released?

@rubenwardy
Copy link
Contributor

rubenwardy commented Dec 22, 2020

1.9 has been in-dev since like 2013, it has a lot of big changes

it's not going to fix all the problems though \o/

@sfan5
Copy link
Collaborator

sfan5 commented Dec 22, 2020

The soon to be released Irrlicht 1.9 has a lot of improvements and bug fixes

Irrlicht 1.9 has been "soon to be released" for about 4 years, we can't wait for this. Pulling in changes from it is a good idea however.

AFAIK Minetest does not run correctly with the last revision of 1.9 for reasons that are partly(?) not MT's fault, so that'd need to be looked at too.

@randomMesh
Copy link

randomMesh commented Dec 22, 2020

Irrlicht 1.9 has been "soon to be released" for about 4 years, we can't wait for this.

I quote CuteAlien, the current maintainer of Irrlicht:

Last ETA was last January :-( I hope I find soon time to take off another 1-2 weeks for Irrlicht coding and maybe that will help me finish it. Basically it's nearly done, but last tasks are stuff I can't just do on the side but need a few days in a row to work on it. My current hope is "summer". But no promise.

Source: http://irrlicht.sourceforge.net/forum/viewtopic.php?f=2&t=52615#p305541

There's hope.

AFAIK Minetest does not run correctly with the last revision of 1.9 for reasons that are partly(?) not MT's fault, so that'd need to be looked at too.

What's the problem? Care to elaborate?

@v-rob
Copy link
Contributor Author

v-rob commented Dec 23, 2020

It would seem that modifying Irrlicht to use SDL 2 wouldn't be too difficult because there are attempts to do this already; a simple search turned up https://github.com/okuoku/irrlicht-generic-sdl and https://github.com/akadjoker/IrrlichtSDL2 as two possible things to reference. Also useful-looking for compiling Irrlicht with SDL is http://irrlicht.sourceforge.net/forum/viewtopic.php?t=24993. Currently, I'm testing Irrlicht's SDL 1.2 support, and I'll see what I can do with it.

@nerzhul
Copy link
Contributor

nerzhul commented Dec 23, 2020

remember that SDL 2 is lower level than irrlicht. I tried it for personal purposes and it's harder to use than irrlicht, but yeah it works

another thing, is SDL 2 working on android ?

@randomMesh
Copy link

randomMesh commented Dec 23, 2020

is SDL 2 working on android ?

It does work on Android, see http://www.libsdl.org/ :

SDL officially supports Windows, Mac OS X, Linux, iOS, and Android.

@v-rob
Copy link
Contributor Author

v-rob commented Dec 23, 2020

remember that SDL 2 is lower level than irrlicht. I tried it for personal purposes and it's harder to use than irrlicht, but yeah it works

That's true, but this would mainly apply to updating Irrlicht to use SDL 2. Code used directly in Minetest isn't likely to delve deep into the low level functions; that's the point of Irrlicht.

another thing, is SDL 2 working on android ?

SDL 2 works on a lot of platforms, more than we'll ever need and more than Irrlicht even supports. A comprehensive list can be found at https://wiki.libsdl.org/Installation

@hecktest
Copy link
Contributor

If you're gonna modify Irrlicht to insert SDL underneath, how much harder would it be to just not use Irrlicht at all and just extract the few features that we actually use?

@v-rob
Copy link
Contributor Author

v-rob commented Dec 23, 2020

If you're gonna modify Irrlicht to insert SDL underneath

Well, this is a bit of a false premise. Irrlicht already can use SDL 1.2 with little to no front-end changes if a compile time option is switched, and so I suggest we update the version of SDL to 2 and slowly replace irr::SEvent with SDL_Event. It's essentially some Irrlicht code modification and then using SDL's events in our codebase. So, there's actually no "inserting SDL underneath" involved. Replacing Irrlicht completely is a whole different ballgame and requires a lot more thought and discussion than this.

If someone really went to work and actually replaced Irrlicht with something else, that could be pretty awesome, but I don't know anyone who really has the time, know-how, and motivation. So, my plan of action is to slowly reduce our reliance on Irrlicht, especially by removing the GUI.

@randomMesh
Copy link

There's already a patch available to use Irrlicht with SDL 2. It should be merged shortly before 1.9 is released. See this post.

Too bad none knows the release date.

@hecktest
Copy link
Contributor

Oh, I thought it would be more work than that.

@v-rob
Copy link
Contributor Author

v-rob commented Dec 25, 2020

Oh, I thought it would be more work than that.

Fortunately, no :)

There's already a patch available to use Irrlicht with SDL 2. It should be merged shortly before 1.9 is released. See this post.

Too bad none knows the release date.

I can't seem to find the patch anywhere, but it would be nice to use. Really, there appear to be at least five SDL2 patches hanging around for Irrlicht from around 2014 up, but it's not in Irrlicht yet because ogles takes higher priority. Some forks of Irrlicht have SDL2, like IrrlichtBAW, but we aren't using those for other reasons that I don't remember.

@TheBrokenRail
Copy link
Contributor

AFAIK Minetest does not run correctly with the last revision of 1.9 for reasons that are partly(?) not MT's fault, so that'd need to be looked at too.

My desktop touch support PR (#10729) adds support for the latest revision of Irrlicht ogl-es.

@nerzhul
Copy link
Contributor

nerzhul commented Dec 26, 2020

i just tried to bind SDL with our irrlicht 1.8.4 standard but it's not possible currently as far i see.
We need to have some deeper changes on irrlicht to create SDL window on irrlicht to have proper access to input events, it's not possible to map SDL window context in irrlicht currently.
If it's a way we want to explore i may give it a try in our irrlicht repository in order to ensure it's possible in our build. We all know that irrlicht is stuck, and forking can be the only solution, like i said 5 years ago. Now we see that 5 years after that irrlicht didn't moved more haha :)

@nerzhul
Copy link
Contributor

nerzhul commented Dec 26, 2020

There's already a patch available to use Irrlicht with SDL 2. It should be merged shortly before 1.9 is released. See this post.

Too bad none knows the release date.

did you see that the post was done 4 years ago ? haha

@v-rob
Copy link
Contributor Author

v-rob commented Dec 26, 2020

i just tried to bind SDL with our irrlicht 1.8.4 standard but it's not possible currently as far i see.

Well, I've tried it with the current revision of Irrlicht 1.9 dev, and SDL works fine. Specifically, after compiling it in, using createDeviceEx with the DeviceType set to EIDT_SDL, it will use SDL internally. It is not possible to use e.g. SDL_PollEvent because Irrlicht does that itself (hence me saying that some jury-rigging would be necessary), but SDL_GetKeyState and similar functions work fine automatically.

I haven't tried it with our Irrlicht, but it doesn't look like we changed it it much, so I would hazard to guess that it would work.

@randomMesh
Copy link

did you see that the post was done 4 years ago ? haha

I did see this. But there are frequent updates of Irrlicht trunk nowadays and i guess it doesn't take that long until 1.9 is released. Patience is the key. There are no current show-stoppers with Irrlicht now and there's no point in rushing.

Having a working SDL2 integration in a bug-fixed Irrlicht 1.9 would be the best foundation for forking.

@v-rob
Copy link
Contributor Author

v-rob commented Dec 26, 2020

There are no current show-stoppers with Irrlicht now and there's no point in rushing.

Is there a roadmap anywhere saying what needs to be done before 1.9 can be released? That would be useful to get a rough estimate at least.

Having a working SDL2 integration in a bug-fixed Irrlicht 1.9 would be the best foundation for forking.

Most assuredly, but there are two downfalls to waiting for 1.9: no one knows the release date, and Minetest has problems with it. Minetest's issues could probably be fixed (and apparently someone has fixed it already as said in a previous reply here), but the problem is waiting. SDL2 would have many benefits, but it also holds things up.

Specifically, if SDL2 will be a reality, that holds up #8679 (because events and keycode names would be different) and a formspec replacement (proper input like IME should be part of the design, not an afterthought). Joystick improvements currently going on could greatly benefit from SDL2. So, I think we need a working SDL2 implementation now, and if we can update later, then great. Therefore, I would propose to use some existing version of Irrlicht modified to use SDL2, at least for now.

@sfan5
Copy link
Collaborator

sfan5 commented Dec 26, 2020

Minetest has problems with it. Minetest's issues could probably be fixed

I just tested again and as of r6176 the things that were broken before still are.
More specifically MT has issues accessing the native X11 window to set its icon and other stuff:

2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: X Error: BadWindow (invalid Window parameter)
2020-12-26 23:48:55: WARNING[Main]: Irrlicht: From call : X_ChangeProperty

Fortunately this is the only issue, ingame is fine. But unless Irrlicht added a different way to access the window handle, this isn't our fault and an Irrlicht bug.

Here's the branch with my changes: master...sfan5:irr19

@v-rob
Copy link
Contributor Author

v-rob commented Dec 27, 2020

Well, the hope here is to make SDL2 handle the windows (therefore including the icon), so it would handle part of the problem. Good to hear that Minetest only has a few problems with 1.9.

@nerzhul
Copy link
Contributor

nerzhul commented Dec 28, 2020

do we need to continue to support irrlicht directly or make our own fork as there is only one developer and the quality/time of the release is very... slow ?
Forking permit to use it directly and cleanup the unneeded features, like for example using SDL directly for game window and drop the whole portability layer inside irrlicht

@v-rob
Copy link
Contributor Author

v-rob commented Dec 29, 2020

I would expect that if we use a fork, we can make some biggish changes, such as removing support for all platforms except SDL2. To take full advantage of SDL2's features, there's not much else we could do, unless we left the other platforms but compiled them out.

Are you planning on working on using SDL2? If that's the case, I'll focus my efforts elsewhere. If you aren't, then I can try doing it myself.

@nerzhul
Copy link
Contributor

nerzhul commented Dec 29, 2020

it's my goal yes, i think having the feedback from @rubenwardy @sfan5 and @SmallJoker around this initiative can help a lot to see if i should continue in this way.
currently i'm replacing the linux X11 parts with SDL2, compile & mapping is fine but i should fix the runtime issues now :), after than i will look deeper in the code to see how we can drop all the devices to use only SDL2, as it offers all we need

@SmallJoker
Copy link
Contributor

Would be interesting to see how feasible this is. I hope the source code diffs won't blow up, and will help testing PRs to shorten the transition phase.

@nerzhul
Copy link
Contributor

nerzhul commented Dec 29, 2020

@SmallJoker currently it's more code removal than other, because SDL calls are shorter than calling the X11 API directly.
For the MR part on MT itself it should shorten the code by removing our X11 hacks, i think

@sfan5
Copy link
Collaborator

sfan5 commented Dec 30, 2020

I think it'd make more sense to use SDL2 internally inside Irrlicht as a first step while keeping the APIs Minetest uses the same (as far as possible), so not replacing SEvent with SDL_Event.
It would still be possible to take advantage of the improved SDL input system that way without having to refactor potentially everything that touches SEvent.

I also don't know how much work it would be to make SDL2 work with Android, so I'd rather not be too quick with making SDL2 a hard dependency.

@v-rob
Copy link
Contributor Author

v-rob commented Dec 30, 2020

My suggestion is essentially to do something like so:

struct SEvent
{
    /* Other declarations here */

    // And the SDL event as well
    SDL_Event event;
};

That way, either SDL or Irrlicht's events can be used for Minetest's primary input system (although not for the GUI) for a temporary API. But, of course, the logical first step is to work on Irrlicht and get that working completely before working on Minetest's code, so nothing in our codebase needs changing yet.

@jingkaimori
Copy link

Are there IME support in SDL?If not ,supporting IME will be harder due to SDL.

@rubenwardy
Copy link
Contributor

rubenwardy commented Jan 3, 2021

I doubt that SDL will make supporting IME harder

@v-rob
Copy link
Contributor Author

v-rob commented Jan 3, 2021

Yes, SDL has good support for IME. See SDL_TextEditingEvent, SDL_TextInputEvent, and the corresponding tutorial. This can be toggled on and off, and it supports all necessary things like the candidate list, etc. This is one of the reasons I want to use it.

@Emojigit
Copy link
Contributor

Emojigit commented Jan 4, 2021

Can this solve #9008 ?

@nerzhul
Copy link
Contributor

nerzhul commented Jan 5, 2021

yep it has support but in our current form we cannot use it easily if we don't modify MT in depth a bit.
I'm currently trying to have same mapping as irrlicht events, but i'm not sure we can easily do it for the text input, SDL 2 inputs are different, we will see where we can go

@numberZero
Copy link
Contributor

I also don't know how much work it would be to make SDL2 work with Android, so I'd rather not be too quick with making SDL2 a hard dependency.

Never tried that but it shouldn’t be too hard: https://wiki.libsdl.org/Android

@v-rob
Copy link
Contributor Author

v-rob commented Jul 7, 2021

OK, so @sfan5 marked this as possible close because we have fixed a lot of the listed problems in IrrlichtMt (https://irc.minetest.net/minetest-dev/2021-07-06#i_5844373). I want to strongly protest this sentiment. Irrlicht's event system is garbage and still is no good. May I point out many existing issues:

  • KeyPress has to check both SEvent::SKeyEvent::Key and SEvent::SKeyEvent::Char because EKEY_CODE doesn't have mappings for every character, notably for / (this, for instance, is why Replace MyEventReceiver KeyList with std::unordered_set #10419 didn't quite work). In addition, even mouse codes are in EKEY_CODE, where they obviously don't belong.
  • Non-QWERTY keyboards still don't work right.
  • Joystick support is not very good, while SDL2 has whole gamepad mappings for commonly used game controllers
  • I don't know for certain, but I doubt that Irrlicht supports mice and keyboards on Android. SDL2 does.
  • Irrlicht's event system plus our hacks are just gross. I mean, we've got raw system events exposed for Android in SEvent for some reason, which is totally non-portable.
  • SDL2 is totally replete with the features we (or basically anyone) needs, and its whole reason for existence is to make things cross-platform.

OK, we could fix these issues. But why? SDL2 can support everything we'll ever need right away, without any disgusting hacks or platform-specific code. I don't see any reason to prefer Irrlicht's system over SDL2 except that it's already in place. That just sounds like a bad reason to me.

If necessary (actually, it's highly likely), I'll do the SDL2 code in IrrlichtMt myself because I really, really, really don't want to make a GUI using Irrlicht's event system.

@sfan5
Copy link
Collaborator

sfan5 commented Jul 7, 2021

  • KeyPress has to check both SEvent::SKeyEvent::Key and SEvent::SKeyEvent::Char because EKEY_CODE doesn't have mappings for every character, notably for / (this, for instance, is why Replace MyEventReceiver KeyList with std::unordered_set #10419 didn't quite work). In addition, even mouse codes are in EKEY_CODE, where they obviously don't belong.

Key input is an issue we discussed before but I don't think there's anything wrong with the basic premise. Just the way it's used is wrong: Keys should be identified by the character they produce and only if they don't produce one they should get an EKEY_CODE. (Though what happens to lower-uppercase?)

By the way if you look at SDL2's list you will find that not every scancode has a matching keycode or vice-versa.

  • Non-QWERTY keyboards still don't work right.

The default WASD bindings will not match e.g. AZERTY but text input and things like the number keys absolutely work.
Do other games get default bindings right on AZERTY?

  • Joystick support is not very good, while SDL2 has whole gamepad mappings for commonly used game controllers

True, lack of support also came up during the joystick PRs.

  • I don't know for certain, but I doubt that Irrlicht supports mice and keyboards on Android. SDL2 does.

It appears to have code for keyboard input but not mouse, no idea if that works.

  • Irrlicht's event system plus our hacks are just gross. I mean, we've got raw system events exposed for Android in SEvent for some reason, which is totally non-portable.

So does SDL. We don't even use this, why is it a problem?

OK, we could fix these issues. But why? SDL2 can support everything we'll ever need right away, without any disgusting hacks or platform-specific code. I don't see any reason to prefer Irrlicht's system over SDL2 except that it's already in place. That just sounds like a bad reason to me.

I don't disagree that it's better to use SDL2 for input and windowing. It's just less urgent now.
Furthermore transitioning SEvent to SDL_Event is not a small task. You either have to switch over all users at once (that includes the GUI, either port that or have a replacement ready) or introduce annoying compatibility code that translates at least some SDL events to SEvent's.

For completeness these are the issues that are solved in IrrMt:

  • lack of touch input (Android obviously has it but also exists on Linux now)
  • non-qwerty keyboards
  • properly handles fullscreen
  • IME input

@v-rob
Copy link
Contributor Author

v-rob commented Jul 7, 2021

I'll admit that SDL is less urgent, however, I do think it should be used. Anyhow, two points I'd like to address:

By the way if you look at SDL2's list you will find that not every scancode has a matching keycode or vice-versa.

When it comes to keyboard input where the keyboard acts like a bajillion button game controller, where the location of keys is important (e.g. WASD scancodes should always be in the same position regardless of keyboard layout), scancodes are used. Keycodes show what is shown on the key itself (i.e. QWERTY scancodes will be AZERTY keycodes on an AZERTY keyboard) and can mostly be ignored by Minetest except to get a human-readable name of the key. It's not like Irrlicht keycodes, which act like SDL keycodes but missing some keys, like /.

In other words, Minetest would get the right bindings on AZERTY keyboards automatically, i.e. ZQSD instead of WASD, if it used SDL scancodes.

It's a little confusing, yes, but a good system. Not having some scancodes map to keycodes is an unfortunate necessity on SDL's part, but it would have very little impact on Minetest. Unlike Irrlicht's keycodes + characters system, which is bad.

Furthermore transitioning SEvent to SDL_Event is not a small task. You either have to switch over all users at once (that includes the GUI, either port that or have a replacement ready) or introduce annoying compatibility code that translates at least some SDL events to SEvent's.

My plan would be to continue having SEvent, but like so:

struct SEvent
{
	EEVENT_TYPE EventType;
	union
	{
		struct SMouseInput MouseInput;
		struct SKeyInput KeyInput;
		// ...
	};
	SDL_Event sdl;
};

Such that any part of the code can use either event type without issues. Compatibility code would be necessary for a time, yes, but it's no different than the existing SDL 1.2 Irrlicht device does. Overall, we'd drop a lot more code immediately (all seven (!) other devices) than we'd gain (one single SDL device).

@kas1e
Copy link

kas1e commented May 7, 2022

@ALL
I don't know how relevant this still, but i do have SDL2 added to Irrlicht year ago for my amigaos4 port of Irrlicht, with all clean "ifdefs" and co. There are relevant commits:

kas1e/Irrlicht@62efbf0 - this one for general adding SDL2
kas1e/Irrlicht@0088f79 - this one on top to enable OpenGL support.

@Zughy Zughy added the Discussion Issues meant for discussion of one or more proposals label Jun 4, 2022
@rubenwardy rubenwardy added Concept approved Approved by a core dev: PRs welcomed! and removed Discussion Issues meant for discussion of one or more proposals labels Aug 14, 2022
@Desour
Copy link
Contributor

Desour commented Sep 15, 2022

What's the status of this?
AFAIK irrlicht supports SDL2, but some things don't work yet (i.e. item movement in inventory).
Does it already make sense to open issues to report bugs?
What is the end-goal? Do we want to drop every driver but the SDL one?

@sfan5
Copy link
Collaborator

sfan5 commented Sep 15, 2022

Does it already make sense to open issues to report bugs?

A list would be helpful but I wouldn't open individual reports for this yet.

What is the end-goal? Do we want to drop every driver but the SDL one?

Yes.

@numberZero
Copy link
Contributor

numberZero commented Apr 1, 2023

minetest/irrlicht#157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Controls / Input Concept approved Approved by a core dev: PRs welcomed! High priority Request / Suggestion The issue makes a suggestion for something that should be done but isn't a new feature
Projects
None yet
Development

No branches or pull requests