OpenGL backgrounds (splash, menu, options) and cursor

[morganchristiansson]

Smallest possible step into OpenGL.
Terrain editor and UI components remain SDL/SGE/2D.

Tried to optimize for review by not maknig any refactorings.
Otherwise I might try to move closer to how s25client does things but that causes much bigger and messier PRs.
Don't have any experience with OpenGL I expect a lot of feedback.
Trying to improve and refactor usually ballons the size of the branch. Not sure what to improve other than what the AI gaslights me to think.

[morganchristiansson]

Btw I did have the thought also.
It would be possible to improve the 2D rendering a lot so it pushes less pixels.
The water animation updates every 127ms or ~8fps.
Most of the UI doesn't need to be redrawn most frames unless interacted with and even then only small parts.
Also zooming and panning will be laggy/CPU spike.

But also, just better to use OpenGL. s25client doesn't even support 2D.

[Flamefire]

s25client doesn't even support 2D.

Not sure what this means.

[morganchristiansson]

s25client only supports 3D videodrivers like OpenGL. No 2D SDL support.
s25client draws EVERYTHING with OpenGL: backgrounds, buttons, cursors, UI, terrain.

[Flamefire]

s25client only supports 3D videodrivers like OpenGL. No 2D SDL support.
s25client draws EVERYTHING with OpenGL: backgrounds, buttons, cursors, UI, terrain.

Actually s25client draws everything (which is 2D) with OpenGL using 2D coordinates. There is no actual "3D" anywhere in a 2D game and consequently all (code) interfaces are 2D.
s25client just uses a different abstraction.
You could also do 3D with SDL if you need, we just don't.

It isn't actually better not using SDL: SDL could do caching of e.g. font textures instead of us rendering each char separately in every frame in s25client. We'd need to implement everything ourselves without SDL
Here we just have 2 projects that have been developed independently and I wouldn't say one is better than the other. Only complaint on the SDL implementation could be that it hasn't been well optimized to what would have been possible, but that wasn't required so far.

[morganchristiansson]

If I want to use libsidelder2 ctrlButton instead of s25edit CButton it uses Draw3D()
https://github.com/Return-To-The-Roots/s25client/blob/master/libs/s25main/controls/ctrlButton.cp#L92-L94
UI components need some improvements needed and some UI components are missing. Would be nice to just use libsiedler2 UI components directly, eventually.

Would also be nice to re-use terrain renderer and other things.

With 4K monitor and my final gl2-terrain branch where everything is drawn with OpenGL I have 15% idle CPU usage with 60fps vs original 1-2fps @ >100% CPU usage.
Tho the 2D renderer really doesn't need to draw 60fps tho, water animation is ~12fps. UI is mostly static otherwise. Mouse cursor is tiny part of screen to redraw.
Only really need to push fps when panning map or smooth zooming.
But idk it might be easier and cleaner to just do OpenGL rather.

[morganchristiansson]

And OpenGL can be worth doing for 2D due to 4K 32bit surfaces being ~33MB/frame and to push 60fps it's ~2GB/s.

[morganchristiansson]

This is my 3rd or 4th attempt at OpenGL btw.
My strategy has evolved to minimal changes to stay close to upstream for ease of review, and if it doesn't get merged I can maintain my own s25edit fork with experimental features while still being close enough to upstream to send PRs upstream.

Earlier attempts made broader changes making more use of OpenGL and trying to use features that s25client uses or use different files and classes for rendering, but I now feel this was a mistake and causes too much divergence.

[Flamefire]

If I want to use libsidelder2 ctrlButton instead of s25edit CButton it uses Draw3D()

That's still 2D ;-)

Would be nice to just use libsiedler2 UI components directly, eventually.

Just to avoid the name confusion: "libsiedler2" is purely the file format handling.
The UI components are in s25client and the dependencies can't be easily extracted, although yes, would be nice.

With 4K monitor and my final gl2-terrain branch where everything is drawn with OpenGL I have 15% idle CPU usage with 60fps vs original 1-2fps @ >100% CPU usage.
Tho the 2D renderer really doesn't need to draw 60fps tho, water animation is ~12fps. UI is mostly static otherwise. Mouse cursor is tiny part of screen to redraw.

Ok, so what you mean is not a 2D renderer but a software renderer

FTR: I'd have simply used Texture as the class name, but GlTexture is "ok enough". It's just my aversion of prefixed classes that easily go over board. See the archive items hierarchies.

Most points are naming and use of the Position/Extent classes we just introduced using.

[morganchristiansson]

All Flamefire feedback addressed:

┌────┬───────────────────────────────────────────────┬───────────────────────────────────────────┐
│ #  │ Feedback                                      │ Resolution                                │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 1  │ glArchivItem_BitmapBase naming                │ Renamed to Texture                        │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 2  │ displayTex_ not a GlTexture                   │ Now Texture displayTexture_ with RAII     │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 3  │ Glad target recreated in CMakeLists           │ Removed entire block — parent provides it │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 4  │ Use Extent, rename linear, createTexture→load │ Done                                      │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 5  │ Extra parens in hasCK                         │ Fixed                                     │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 6  │ DrawFull → Draw                               │ Done                                      │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 7  │ Draw(x,y) → Draw(Position)                    │ Done                                      │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 8  │ width_/height_ → Extent size_                 │ Done                                      │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 9  │ Copyright for new files                       │ Updated to 2026                           │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 10 │ std::exchange for move ops                    │ Done                                      │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 11 │ Verbose comments in Render()/RenderPresent()  │ Trimmed to minimal                        │
├────┼───────────────────────────────────────────────┼───────────────────────────────────────────┤
│ 12 │ getBackground() unnecessarily public          │ Moved back to protected                   │
└────┴───────────────────────────────────────────────┴───────────────────────────────────────────┘

Your design feedback also addressed:
- Background rendering moved back into CMenu::render() with lazy-loaded Texture — no setBgTexture() needed
- menuBgMain_/menuBgSub_ removed from CGame (dead code)

    Review feedback

 CGame.cpp                    | 23 ++++-------------------
 CGame.h                      | 16 +++++++---------
 CGame_Init.cpp               | 28 +++++-----------------------
 CGame_Render.cpp             | 14 ++------------
 CIO/CControlContainer.h      |  2 +-
 CIO/CMenu.cpp                | 18 ++++++++++++++++--
 CIO/CMenu.h                  |  4 ++++
 CMakeLists.txt               | 18 ------------------
 CSurface.h                   |  1 -
 GlTexture.h                  | 40 ----------------------------------------
 GlTexture.cpp => Texture.cpp | 74 +++++++++++++++++++++++++++++++++++++++-----------------------------------
 Texture.h                    | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 12 files changed, 128 insertions(+), 160 deletions(-)

I think I will squash and force push before PR merge please. But will keep it incremental commits during review.
And I do not appreciate the squash button when merging PRs, I prefer to do it locally with git cli.

[morganchristiansson]

Great feedback this is better, thank you!

[Flamefire]

Nits only now, you asked for it ;-)

But: Did you test this? I see this now uses software rendering for some parts while it draws menu backgrounds directly.
Does that work correctly or cause wrong drawing order somewhere?

[morganchristiansson]

Yes this branch I test every time. But I missed some issues with window resize/resolution change which I just fixed.
But some PR I have submitted for early feedback without fully testing. I usually make these drafts.

Nits only now, you asked for it ;-)

Ok we're having fun now lolol

But: Did you test this? I see this now uses software rendering for some parts while it draws menu backgrounds directly.

Yes, only CMenu backgrounds are OpenGL in this PR. There's 2 follow up PRs for UI components and terrain editor. This is just GL groundwork and CMenu backgrounds. "Smallest possible step into OpenGL." as I wrote first line in PR.

Does that work correctly or cause wrong drawing order somewhere?

The CMenu/background needs to be drawn bottom, then SDL_Surface Surf_Display, then cursor last on top.
In early testing the cursor was drawn under UI components lol.

Flamefire review feedback (round 3)

#	File	Change
1	CGame_Render.cpp	Remove setGLViewport() from Render() — only needed on window create/resize, not every frame
2	CGame_Render.cpp	Remove AI-generated comment // Cursor is drawn in RenderPresent via GL (references removed code)
3	CGame_Render.cpp	Remove unnecessary braces around single-line if body (leftover from an earlier comment)
4	CGame.cpp	Remove weird extra {} scope block around cursor rendering in RenderPresent()
5	CMakeLists.txt	Remove list(APPEND CMAKE_MODULE_PATH …) — glad is already found via parent project
6	Texture.h + Texture.cpp	Rename load(Extent) → createEmpty(Extent) to clarify it creates an empty texture, not loads data
7	CGame_Init.cpp	Update call site displayTexture_.load(GameResolution) → displayTexture_.createEmpty(GameResolution)

Bug fixes

Bug 1 — Cursor turns into white/corrupted rectangle after resolution change

Two interrelated root causes:

1. Cross-context glDeleteTextures: ReCreateWindow() was destroying the old GL context first, then calling cursor_.load(…) which internally called
   glDeleteTextures on the old (now stale) texture ID, but inside the new GL context. This corrupted the new context's state.
2. Stale texture IDs: After the new context was created, cursor_, cursorClicked_, cross_, splashBg_, and menu bgTexture_ still held texture IDs from the
   destroyed context.

Fix (in ReCreateWindow()):

• Before destroying the old context, reset all GL textures via = Texture() — this calls glDeleteTextures while the old context is still active. Same for menu
  bgTexture_ via menu->resetBgTexture().
• After creating the new context, reload cursor/splash textures from the still-valid SDL surfaces in global::bmpArray (guarded by null check — no-op on first
  call from Init()).
• Used a loadGlTex lambda to avoid the repetitive bounds-checking boilerplate.

Bug 2 — Window resize doesn't update drawn area

Root cause: UpdateDisplaySize() (called on SDL_WINDOWEVENT_RESIZED) updated GameResolution and recreated Surf_Display + displayTexture_, but never called
setGLViewport(). The GL projection/viewport stayed at the old window size even though the window had changed.

Fix: Added setGLViewport() call in UpdateDisplaySize().

[morganchristiansson]

Pushed another fix for full screen res switching. It's really hairy for me maybe cause of Wayland.
It also doesn't work in s25client. But works with this fix. Can't really test other platforms.

[Flamefire]

ReCreateWindow() was destroying the old GL context first, then calling cursor_.load(…) which internally called
glDeleteTextures on the old (now stale) texture ID, but inside the new GL context. This corrupted the new context's state.

Before destroying the old context, reset all GL textures via = Texture()

This looks very brittle as you have to remember all textures created anywhere in the program and (re)create them in a single place. That will eventually become out-of-sync and fail, especially when you convert more to hardware-rendering
Either textures need to store their (CPU) data to be able to recreate itself and register a listener on the context or a weak pointer to it, or something else. To old code did (almost) that, the "registering" was the other way round though by explicit calls of "discard texture", but they kept their CPU data anyway.
Or you get to changing the window without destroying the context.

If I see understand that correctly recreating the window is currently done only to change resolution on UI requests, while it is not invoked in regular window-resize-events. In s25client we handle that by changing the window in explicit requests with fallbacks when the request cannot be fulfilled, worst case ignoring it IIRC.
Can you try that?

[morganchristiansson]

s25client fullscreen resolution switching is broken for me. Maybe because I use wayland.
I'm not even sure resolution switching in s25edit is useful, I only just added 4K UHD to list it was missing.
I think alway native resolution in fullscreen and resizable window is perfection and delete the resolution list, but I wonder if everyone will agree to this.

The list is super long and the UI list scroll is doesn't do press and hold to keep scrolling, there's no block to drag to scroll. Have to click repeatedly to scroll one step at a time.
deepseek4 flash said it's going crazy failing to fix resolution switching, Kimi K2.7 actually aces it but uses 30x more of my usage limit.
But I will see what I can do and test with further branches gl2-ui and gl2-terrain. Maybe accepting wayland is broken and testing with X11 is an option.

[Flamefire]

s25client fullscreen resolution switching is broken for me. Maybe because I use wayland.

Broken in which way and situations? E.g. fullscreen enabled already at start, then switching only resolution ingame. What happens/breaks? Does it work here (with the window recreation)?
At worst we could consider changing the fullscreen resolution on wayland requiring a restart. Not sure if we can do a "soft-restart", i.e. go back to the start of the program and redo everything, but that's probably overkill.
Would be good if we can detect this problem in code and then just show a message to restart instead of actually changing.

[morganchristiansson]

In s25client when I change the fullscreen resolution I get this:

It might be wayland doing async events while all other video drivers do synchronous events. Honestly not sure - this is what AI says it is.

I just wonder who actually wants to run s25edit at less than native resolution in fullscreen? I can't think of any usecase.

[morganchristiansson]

Also I did not reproduce corruption in gl2-ui branch. Maybe AI is being overly defensive with this. It doesn't actually run anything and does all changes by static analysis / reasoning about code.
You make a good point tho worth looking into further.
Having endless problems getting resolution switching to work correctly...

[morganchristiansson]

Hmm ok gl2-ui only adds displayTexture_ which replaces Surf_Display so there's only 1 texture which is re-loaded every frame.
edit: There's some more like fpsTex_ and terrain surface(=texture) but they are all reloaded.

[morganchristiansson]

Ok I managed to remove ReCreateWindow() without breaking full screen resolution switching.
Not all resolutions actually work, they are hardcoded instead of queried from what is supported. But I can submit a separate PR for that.

[Flamefire]

In s25client when I change the fullscreen resolution I get this:

I can remember someone reporting that too related to high-DPI settings: Return-To-The-Roots/s25client#1621
But that is on initial start already, not (just?) fullscreen switching.

It might be wayland doing async events while all other video drivers do synchronous events. Honestly not sure - this is what AI says it is.

I guess it's easier to debug here where only SDL is used. It looks like we get wrong/stale data when (re)creating the viewport or internal sizes. Hard to debug when not having access to a machine that has this issue as I'd check variables and events involved and e.g. compare to what they are after a restart.

I just wonder who actually wants to run s25edit at less than native resolution in fullscreen? I can't think of any usecase.

Same as for any other game: Someone might want a higher resolution for the game than for what they "normally" do on their computer.
If there wasn't demand for that then it wouldn't be something virtually all games offer. Although that might be a circular thing ;-)
So would be great if we get that working.

[morganchristiansson]

I can try to fix it in s25client too, but it doesn't actually bother me since I just use native resolution.

Someone might want a higher resolution for the game than for what they "normally" do on their computer.

Doesn't make sense to me. Why would they run a lower than full/native resolution for either of desktop or s25edit.
When games are released they are pushing the performance limits and some users may need lower resolution for performance reasons. This is not the case for us.

Latest commit I pushed keeps full screen switching working on wayland on my system, and removes ReCreateWindow() so don't need to invalidate textures. Also pushed PR #58 to query supported resolutions which should remove invalid resolutions from list.

[Flamefire]

I can try to fix it in s25client too, but it doesn't actually bother me since I just use native resolution.

First I would like to know what exactly the issue and fix were. Then we can check how to best port it there.

Doesn't make sense to me. Why would they run a lower than full/native resolution for either of desktop or s25edit.
When games are released they are pushing the performance limits and some users may need lower resolution for performance reasons. This is not the case for us.

I don't know either. Just having the option avoids users complaining they wanted that for some reason ;-)

Latest commit I pushed keeps full screen switching working on wayland on my system, and removes ReCreateWindow() so don't need to invalidate textures.

Looks good, would be good to know what exactly fixed it. I.e. if you can experiment a bit to pinpoint the actual fix. Too much going on with the switch to recreating windows and back to clearly see it.

Also pushed PR #58 to query supported resolutions which should remove invalid resolutions from list.

Early feedback: "Make the same as s25client" isn't a goal in itself. So that shouldn't be mentioned in descriptions, code or comments. Either there is a reasonable change then we can check it or there isn't. If the (only) argument is "it's done somewhere else" then it's not worth even looking at it.
So please make sure you (or the AI) can argue everything on its own merit.

[Flamefire]

Why do we need this? Wasn't the point of the last change to keep the textures? How would they be recreated?

[morganchristiansson]

I think it's rescaling as it's full screen and the size just changed. I already asked AI about this and this was reason it gave me but I can double check that it's not being lazy and making shit up lol

[Flamefire]

Yeah making shit up. The texture has no notion of screen size. Only the surface has

[morganchristiansson]

Ok you are right. Background rescales much smoother when resizing window now.

[Flamefire]

Is that the part that fixed your issue with half-covered screen/rendering?

[morganchristiansson]

I think this is the async events which can cause reverting of resolution switch.
Instead of the earlier suppressResizeEvents_ which was ugly but had no issues from it.

[morganchristiansson]

And as for what the fix was. Honestly not completely sure.
If I worked on it by hand I would've had a eureka moment when it started working correctly.

My process is I prompt AI about issue, it reads code and reasons about it, makes changes, then I build, run and test, repeat.
AI doesn't get direct feedback except follow up prompts and toolcall results. It's a visual game it's not ideal to feed back to AI.
I suppose if AI could see and validate results directly then it could make more precise changes. Or a separate test program that proves correct implementation as resolution switching has proven quirky in both s25edit and s25client.
Anyway I kinda like it minimal and limited access and it's working mostly fine like this.

[Flamefire]

The problem is that if we don't know what exactly caused and/or fixed the issue we cannot apply it to e.g. s25client or document it in code such that not in 2yrs someone sees strange code and decides to remove it.
I guess that if you try to change parts of this commit or ask your AI about what actually caused the fix and change that (with AI or manually) you'd get your eureka moment and we the fix for s25client ;-)

If it's too much work then we'll just need to keep it as-is

[morganchristiansson]

I can do it again in s25client with AI or we can make a small test program that reproduces and validates best practice.

Wayland doesn't even allow apps to change native monitor resolution, it instead rescales them, presumably in hardware. Which is also what modern LCD monitors do.

If you want to keep improving res switching I can make a standalone test program for this purpose.

[Flamefire]

If you can let your AI reduce the editor to a simple reproducer and minimal fix based on that commit that would great, yes. Like: Create fullscreen window with 2 textures, could be just red and blue where one rightclick you draw the other one and on leftclick you change the resolution. I expect this to be ~100 lines with most of it copied from s25edit.

[morganchristiansson]

Also it can automatically proceed resolution switch, get window events, log what actually happened/didn't happen.

For us humans it might be extra work. For the AI it will save a lot of tokens making a tiny program it can work out without all the surrounding complexity in context.

[Flamefire]

Looks good, thanks!

[morganchristiansson]

Awesome.
Would've liked to rebase before merge tho.
Next up is gl2-ui branch, #52. Will need to rebase that first tho.

I'm also thinking. each UI component still draws to SDL_Surface, this was just to make changes smaller and the surfaces are small and have minimal penalty.
Changes inside UI components tho will not conflict with other changes, and having AI update them is a non-problem. So I should probably remove SDL_Surface in UI components and go full OpenGL.
But anyway that is an issue to discuss in #52