Bitten by createWindow / _destroyWindow pair

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

I have been looking into OgreBites' ApplicationContextBase. There are pairs of virtual methods createWindow() and _destroyWindow(). There is a non-virtual destroyWindow()... it seems it is designed to destroy a window using its "name".

This is not the contents of this post, but I do not see a good reason for using a window name... looking for a window using its name sounds a lot non-portable to me. Since createWindow() returns a NativeWindowPair, even the non-developer should not use the window name, but the "token" returned by "createWindow()". There should be no "destroy window from its name", IMO. But probably people will not agree with that, and if they do, it would be only for Ogre 15.

The problem I want to address is that those methods are not acting in "pairs". One does not undo what the other does. SDL and Android descendants (Qt is implemented right) do not call ApplicationContextBase::createWindow. But they all call ApplicationContextBase::_destroyWindow (Qt and Android do not override).

This is a little odd, because createWindow / _destroyWindow are supposed to work in pairs, in my opinion. There are at least two consequences:

  1. The code inside the base virtual createWindow is replicated inside each new createWindow (for example -- twisted example continuation).
  2. The code inside the base needs to be aware of the code inside its child. :shock: :?

We can easily fix this. We should separate the roles of the base class and the derived class. The derived class is responsible for setting up the "native window" and the miscParams. The base class is responsible for using those two and setting up Ogre's render system.

Code: Select all

/* derived createWindow pseudo-code -- just like in Qt */
create native window;
setup miscParams;
ApplicationContextBase::createWindow(..., miscParams);

Notice that not doing this is the reason for this (unecessary) hack:
https://github.com/OGRECave/ogre/blob/8 ... #L414-L417

There should be no "#ifdef" there. Either this line should not be in the base createWindow(), or it should always be in destroyWindow(). The problem is that those two children (SDL and Android) fail to call "WindowEventUtilities::addRenderWindow()", while Qt does (because it calls the base createWindow()). Probably SDL and Android were implemented a long ago and got "out of sync" with the replicated code in the base class.

Shall I submit a PR?

PS: A side effect is that "WindowEventUtilities::_addRenderWindow()" will be called for SDL and Adroid. Is this okay? Otherwise, we have to call it inside Qt's createWindow(), and remove in Qt's _destroyWindow().

paroj
OGRE Team Member
OGRE Team Member
Posts: 2092
Joined: Sun Mar 30, 2014 2:51 pm
x 1128

Re: Bitten by createWindow / _destroyWindow pair

Post by paroj »

andrecaldas wrote: Thu Aug 29, 2024 1:24 pm

The problem is that those two children (SDL and Android) fail to call "WindowEventUtilities::_addRenderWindow()", while Qt does (because it calls the base createWindow()). Probably SDL and Android were implemented a long ago and got "out of sync" with the replicated code in the base class.

Shall I submit a PR?

the Qt implementation does not call the base createWindow() either (which is correct).

The rationale behind this code is that ApplicationContextBase acts as a third "hidden" implementation. Historically there was only ApplicationContextBase and ogre would create the windows using some low-level native API internally. Those windows need to be registered with WindowEventUtilities to get window events.

However this was a bad API and nowadays windows should be rather created externally, like done in ApplicationContextSDL and ApplicationContextQt - with those windows WindowEventUtilities must not be used as it would interfere with SDL and Qt.

If you can improve the code given the above constraints, feel free to create a PR.

andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

paroj wrote: Thu Aug 29, 2024 3:50 pm

the Qt implementation does not call the base createWindow() either (which is correct).

:lol: You are right, sorry!
I had written this post saying none would call createWindow(). And later I got confused with this. Sorry!

So, nobody actually uses it...

But as far as I can tell, except for "_addRenderWindow()", Qt, Android and SDL do exactly what the base createWindow() does, after setting miscParams.

paroj wrote: Thu Aug 29, 2024 3:50 pm

However this was a bad API and nowadays windows should be rather created externally, like done in ApplicationContextSDL and ApplicationContextQt - with those windows WindowEventUtilities must not be used as it would interfere with SDL and Qt.

<3 <3 <3 <3 <3 :-)
I really love the idea of using "externally created windows"!!!

paroj wrote: Thu Aug 29, 2024 3:50 pm

If you can improve the code given the above constraints, feel free to create a PR.

What about "ApplicationContextBase::pollEvents()"? I think it is not used, either. I suppose the original idea was not to have abstract virtual members so to allow derived classes to have a "default". But, shouldn't this change nowadays?

If "ApplicationContextBase::pollEvents()" was made pure virtual, "WindowEventUtilities" would become useless and possibly could be removed (in Ogre 15).

andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

paroj wrote: Thu Aug 29, 2024 3:50 pm

the Qt implementation does not call the base createWindow() either (which is correct).

:lol: You are right, sorry!
I had written this post saying none would call createWindow(). And later I got confused with this. Sorry!

So, nobody actually uses it...

But as far as I can tell, except for "_addRenderWindow()", Qt, Android and SDL do exactly what the base createWindow() does, after setting miscParams.

paroj wrote: Thu Aug 29, 2024 3:50 pm

However this was a bad API and nowadays windows should be rather created externally, like done in ApplicationContextSDL and ApplicationContextQt - with those windows WindowEventUtilities must not be used as it would interfere with SDL and Qt.

<3 <3 <3 <3 <3 :-)
I really love the idea of using "externally created windows"!!!

paroj wrote: Thu Aug 29, 2024 3:50 pm

If you can improve the code given the above constraints, feel free to create a PR.

What about "ApplicationContextBase::pollEvents()"? I think it is not used, either. I suppose the original idea was not to have abstract virtual members so to allow derived classes to have a "default". But, shouldn't this change nowadays?

If "ApplicationContextBase::pollEvents()" was made pure virtual, "WindowEventUtilities" would become useless and possibly could be removed (in Ogre 15).

andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

paroj wrote: Thu Aug 29, 2024 3:50 pm

If you can improve the code given the above constraints, feel free to create a PR.

Another question:

As far as I can tell, the passed miscParams value of "Full screen" is ignored in favor of "p.useFullScreen".

Is this the "correct / intended" behaviour?

paroj
OGRE Team Member
OGRE Team Member
Posts: 2092
Joined: Sun Mar 30, 2014 2:51 pm
x 1128

Re: Bitten by createWindow / _destroyWindow pair

Post by paroj »

andrecaldas wrote: Thu Aug 29, 2024 4:28 pm

If "ApplicationContextBase::pollEvents()" was made pure virtual, "WindowEventUtilities" would become useless and possibly could be removed (in Ogre 15).

as said above, ApplicationContextBase can be used directly, if you want to use the internally created Ogre windows. While I would avoid it, there is lots of legacy code out there doing it: https://github.com/search?q=WindowEvent ... &type=code

paroj
OGRE Team Member
OGRE Team Member
Posts: 2092
Joined: Sun Mar 30, 2014 2:51 pm
x 1128

Re: Bitten by createWindow / _destroyWindow pair

Post by paroj »

andrecaldas wrote: Thu Aug 29, 2024 7:05 pm

Another question:

As far as I can tell, the passed miscParams value of "Full screen" is ignored in favor of "p.useFullScreen".

Is this the "correct / intended" behaviour?

I suggest you put a breakpoint into that function to investigate this ;)

andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

paroj wrote: Thu Aug 29, 2024 11:28 pm
andrecaldas wrote: Thu Aug 29, 2024 7:05 pm

As far as I can tell, the passed miscParams value of "Full screen" is ignored in favor of "p.useFullScreen".

Is this the "correct / intended" behaviour?

I suggest you put a breakpoint into that function to investigate this ;)

I can barely execute Ogre. :lol:

But I can say exactly what the code says:

  • What decides for full screen or not is the value of "p.useFullScreen" returned by "getRenderWindowDescription()". So, even if the miscParams passed to createWindow has a "Full screen" parameter, it will be ignored.

Therefore, my question is:

  • Is this the intended behaviour?
Last edited by andrecaldas on Fri Aug 30, 2024 2:17 am, edited 1 time in total.
andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

andrecaldas wrote: Thu Aug 29, 2024 11:48 pm

I can barely execute Ogre. :lol:

I have just managed to execute SampleBrowser in Wayland.
I had to use SDL3 with an SDL2-compat library. You link to it as if it was SLD2, but SDL3 runs behind the scenes.

The examples are amazing!!!
Now I can setup break points and step through the code. :-)

Last edited by andrecaldas on Fri Aug 30, 2024 2:15 am, edited 1 time in total.
andrecaldas
Halfling
Posts: 54
Joined: Mon May 06, 2024 1:06 am
x 3

Re: Bitten by createWindow / _destroyWindow pair

Post by andrecaldas »

paroj wrote: Thu Aug 29, 2024 3:50 pm

If you can improve the code given the above constraints, feel free to create a PR.

I did. I think... mabye you will not agree.

I have implemented and SDL3 version.

But I did not care for the "constraints". :mrgreen:

I have made the base "pure virtual" and created a "Dummy" version of it. So, in my branch I have:

  1. ApplicationContextDummy
  2. ApplicationContextSDL3
  3. A pure virtual ApplicationContextBase.

It seems I probably broke some "java bindings" or something like that. I guess those bindings either need to use ApplicationContextDummy, or they do not cope easily with pure virtual classes and they need some "trampoline class".

I think this approach is better because the Dummy part implementation inside Base was getting in the way and making it hard to put the "common code" in the base class. For that reason, the derived code was re-implementing what should be reused from the Base class.

CONS:

  1. I do not know what to do with the "java bindings", but it must not be hard to fix.
  2. Conditionals related to EMSCRIPTEN are probably wrong in cmake files.
  3. NativeWindowType is a bad alias for the type "void". :mrgreen:

Future work:

  1. Do not decide on Wayland or X11 at compile time, but at runtime.

Show changes.