OpenGL UI components by morganchristiansson · Pull Request #52 · Return-To-The-Roots/s25edit

morganchristiansson · 2026-06-27T13:37:17Z

Draw all UI components with OpenGL.
Keeps UI component SDL_Surface for a smaller PR but avoids 4K penalty of uploading a 4K 32bit 33MB surface each frame 2GB per second. Only small component surfaces are uploaded each frame.
Splash, main menu and options screens are entirely OpenGL.

Replacing SDL_Surface in each component with OpenGL to be addressed in future PR.
Terrain editor Surf_Map remains 2D/SDL/SGE in this branch.

morganchristiansson · 2026-06-29T17:57:48Z

Also I can push this PR all the way replacing the per-component SDL_Surface with OpenGL. It's not hard at all with AI agent. I only left it out of scope for simpler review. Main benefit is already achieved.

Flamefire · 2026-06-30T07:55:55Z

Draw all UI components with OpenGL.
Keeps UI component SDL_Surface for a smaller PR but avoids 4K penalty of uploading a 4K 32bit 33MB surface each frame 2GB per second. Only small component surfaces are uploaded each frame.
Splash, main menu and options screens are entirely OpenGL.

Can you give a bit more background? This code is old with old design decisions etc. which I don't remember especially as it wasn't written by me.
So a short summary of what and where issues are, how they manifest, what the change actually does and how that solves the problem.

IIRC SDL does use OpenGL as the backend/renderer, so the code alread is "using OpenGL" (to some extent)

morganchristiansson · 2026-06-30T10:44:14Z

s25edit doesn't use OpenGL at all.
SDL and SGE is entirely 2D. Software CPU rendering.
Personally I get 1-2fps on 4k display so it's unusable. Idk if it's cause I have 13 year old 4.5GHz quad core i7-4770k CPU and DDR3 RAM and newer CPUs can easier push ~2GB of 32bit pixel data per second @ 60fps.

And say I want to use ctrlButton from libsiedler2 instead of s25edit CButton.
It draws with 3D https://github.com/Return-To-The-Roots/s25client/blob/master/libs/s25main/controls/ctrlButton.cpp#L92-L94

Keep small surface per UI component that is drawn with OpenGL Avoids big 4K penalty while keeping change small

morganchristiansson · 2026-07-03T18:07:17Z

Hmm this PR is currently really small and nice.

But replacing getSurface() with getTexture() is keeping with 2D blitting patterns, s25client uses Draw() which enables components to draw directly. Much bigger PR tho.
I guess getTexture() is fine tho and gets us most value from least change.
But if Draw() is better than render loop doing getSurface().Draw(), and the UI components should not have both SDL_Surface and Texture, then should just eat the bigger PR now rather than having a follow up after.

And hmm maybe #47 gl2-terrain should really be parallel PR and not on top of this one. It doesn't change UI components at all.

morganchristiansson · 2026-07-03T18:59:19Z

Ok I went ahead and implemented Draw() with removed getSurface() and getTexture()

# before:
12 files changed, 143 insertions(+), 78 deletions(-)
# after:
25 files changed, 632 insertions(+), 903 deletions(-)

bigger change but removing 280 lines!

Flamefire

I admit this does simplify some things.

Added comments where AI overlooked stuff or we can be more succinct or consistent.

Also remove mentions like "uses OpenGL". That looks like AI added it because it changed it. Not because it is useful information at that point.
Especially given now most is rendered with OGL it would be noteworthy if something does not. But that is usually clear by e.g. passing a destination surface

morganchristiansson · 2026-07-04T23:04:33Z

59 comments! I reviewed them with the AI guiding me and grouping them.
I'm trying to not be a slop cannon!
Good stuff.

The AI might have missed something tho, cause you know, computers have shit memory and forget thing nowadays.

morganchristiansson · 2026-07-04T23:15:23Z

I admit this does simplify some things.
Glad you like it!

But that is usually clear by e.g. passing a destination surface

There's two full screen SDL_Surfaces left:

Surf_Display is only UI components: windows, buttons, text inputs etc etc.
It used to also have CMenu backgrounds.
Surf_Map is terrain editor, map objects, screen frame, even the icon buttons in the frame (which are not UI components).
Surf_Map gets blitted to Surf_Display in master (with transparency I guess).

This branch gl2-ui deletes Surf_Display and makes Surf_Map the only full screen surface.
gl2-terrain #59 deletes Surf_Map.
Will need to resolve some minimal conflicts when one is merged but the branch changes do not overlap, much.
There shouldn't be any SDL_Surface left after these unless we missed some small internal surface.

Flamefire

But that is usually clear by e.g. passing a destination surface
There's two full screen SDL_Surfaces left:

yeah, just do a quick grep for comments and mentions of OpenGL/OGL on the diff to check for such issues please.

The AI might have missed something tho, cause you know, computers have shit memory and forget thing nowadays.

They are becoming too human 😆

Most things are (still) about using Position/Extent. I'm caring because we had subtle bugs where a C&P used e.g. "x" twice instead of "y". Those classes avoid such cases completely and make code (usually) more readable through the ops. They can even be combined like base types Position() + Extent() yields a Position, because one of those is signed which is usually what you want. Although it is not like int + unsigned yielding unsigned

Flamefire · 2026-07-05T10:51:06Z

+{
+    static std::vector<std::unique_ptr<Texture>> cache;
+    static std::vector<bool> linearFlags;
+    if(idx < 0 || idx >= static_cast<int>(global::bmpArray.size()))


want to do that (in a separate commit) or is that change to large?

Flamefire · 2026-07-05T11:44:23Z

-            return false;
-    }
+    const Position origin = pos_;
+    const Rect winRect(origin, size_);


Can use the method directly at the single place this is used, maybe also origin/getPos and using Position arithmetic

inlined origin and winRect

Flamefire

few left-over from last round and some new after changes

Flamefire · 2026-07-05T14:49:44Z

-        }
+        // Clamp window position to screen bounds
+        const auto res = global::s2->getRes();
+        const int maxX = static_cast<int>(res.x - size_.x) - 1;


Just thinking about the cast: Do we ensure size < res? Otherwise we might need:
elMax(Extent::all(1), res - elMin(size_, res))
otherwise we have UB (large uint->int, lo>hi in clamp)

Flamefire · 2026-07-05T14:51:06Z

+    {
+        return (global::s2->getRes() - size) / 2;
+    } else


Suggested change

{

return (global::s2->getRes() - size) / 2;

} else

return (global::s2->getRes() - size) / 2;

else

Flamefire · 2026-07-05T14:52:57Z

-        CSurface::Draw(surface, global::bmpArray[resizebutton].surface,
-                       Position(w_, h_) - global::bmpArray[resizebutton].getSize());
+        getBmpTexture(resizebutton)
+          .draw(origin + Position(static_cast<int>(size_.x), static_cast<int>(size_.y))


Suggested change

.draw(origin + Position(static_cast<int>(size_.x), static_cast<int>(size_.y))

.draw(origin + Position(size_))

Flamefire · 2026-07-05T14:54:59Z

+    return *cache[idx];
+}
+
+void ensureBmpTex(int idx)


Flamefire · 2026-07-05T15:02:19Z

+        return;
+
+    const Extent tileSize = getSize();
+    if(static_cast<int>(tileSize.x) <= 0 || static_cast<int>(tileSize.y) <= 0)


(half-)invalid check, check for zero only

Flamefire · 2026-07-05T15:03:15Z

+    if(static_cast<int>(tileSize.x) <= 0 || static_cast<int>(tileSize.y) <= 0)
+        return;
+
+    glColor4f(1, 1, 1, 1);


color does need to be reset in some places. But can be moved to those specific places rather than reset globally.

Yes, OGl calls are costly. So don't do them if not necessary, but only where you actually change things

It's very cheap compared to copying 33MB surface from system RAM 60 times per second. 2GB/s.

Also gl commands are queued until glSwapBuffers()
(I didn't know this, just asked my AI)

I'm actually adding more gl calls in gl2-terrain #59 to improve reviewability of diff, reverting to one DrawTriangle() at a time. Essential for reviewability. And actually fixed a bug when reverting to make it closer to original.
I think it's fine but can improve it post-merge.

Sure, but then driving your car around the block for no reason is also cheap compared to flying overseas ;-)
Just want to avoid "premature pessimization" by adding unnecessary things that take more cycles than a couple additions.

Yep. I fixed it's good call. OCD matters for code.

Flamefire · 2026-07-05T15:06:18Z

 #include <Point.h>
 #include <SDL.h>

-/// Wraps a texture with RAII and provides draw methods.


40407bd5d0a385d3e6b6fbac531a352b33b7cb0b07809f9213765ac09c71df6c removed the docstrings.
Those were relevant

I thought it was mostly redundant. This comment doesn't really add anything?

Docstrings do help as they get rendered in IDEs at usage site.
The class one could mention it wraps an "OpenGL texture (handle)" as "texture" is quite generic: All the bmps are textures

And e.g. for the load method:

/// Load from a 32-bit or 8-bit paletted SDL surface. /// For 8-bit surfaces, optional colorkey is respected (keyed pixels become transparent).

That is more than the the signature provides. So a short sentence per method in the header is useful
Especially e.g.:

/// Draw the texture stretched to fill the given rect. void draw(const Rect& destRect) const;

How else would you know it is stretched?

I mean Texture class name is self-explanatory.
draw with target size is implied stretched.
it was called drawStretched in previous PR but you wanted it just draw which is fine.

I mean Texture class name is self-explanatory.

As I said: It could mean a graphics file or a hardware texture. So knowing this is an "OpenGL texture handle" at the cost of 1 line is fine.

My point about comments/docstrings is whether they save time.
Bad:

/// Get color unsigned getColor(); // Make ARGB: alpha<<24 | red <<16 [...] color = a << 24 | r << 16 | g << 8 | b

But the short docstrings quoted above do add information/context:

OpenGL texture handle -> ready to use with OpenGL

support for 8 & 32 bit with transparency

stretching semantics

So for docstrings in headers I'd lean to including them (but keep them succinct) unless they state literally the same as the method name, like getColor above, but even then: "Get color as ARGB value" would add value.

See the borderBeginSize case: In 4yrs time you won't remember what you meant even when it's obvious now ;-)

Ok fair. I've not used docstrings in c++ before and AI started adding them unprompted and I thought it was more slop comments.

Flamefire · 2026-07-05T15:14:33Z

+        const int arrowIdx = MAPPIC_ARROWCROSS_ORANGE;
+        const auto& dispRect = map->getDisplayRect();
+        const Position arrowCenter =
+          dispRect.getOrigin() + Position(dispRect.getSize().x / 2, dispRect.getSize().y / 2);


Suggested change

dispRect.getOrigin() + Position(dispRect.getSize().x / 2, dispRect.getSize().y / 2);

dispRect.getOrigin() + dispRect.getSize() / 2u;

morganchristiansson mentioned this pull request Jun 27, 2026

OpenGL Terrain editor #47

Closed

morganchristiansson force-pushed the gl2-ui branch 2 times, most recently from 22129c8 to fbd0a59 Compare July 1, 2026 00:14

morganchristiansson mentioned this pull request Jul 1, 2026

OpenGL backgrounds (splash, menu, options) and cursor #50

Merged

morganchristiansson force-pushed the gl2-ui branch 2 times, most recently from 9319866 to 6dd57e4 Compare July 3, 2026 15:55

Draw UI with OpenGL, delete Surf_Display

8a33c93

Keep small surface per UI component that is drawn with OpenGL Avoids big 4K penalty while keeping change small

Draw()

c650bdd

morganchristiansson force-pushed the gl2-ui branch from 6dd57e4 to 3af4317 Compare July 3, 2026 18:53

drawMinimap

76531bd

morganchristiansson force-pushed the gl2-ui branch from 3af4317 to 76531bd Compare July 3, 2026 18:54

morganchristiansson changed the title ~~OpenGL UI (on top of #50 OpenGL backgrounds & cursor)~~ OpenGL UI components Jul 3, 2026

Flamefire requested changes Jul 4, 2026

View reviewed changes

morganchristiansson added 12 commits July 4, 2026 19:47

Obvious review fixes, remove multiline

4332feb

Revert changes to Texture

eca9e13

getPos and struct FrameInserts

71ace85

drawButtonBox()

e4163e8

Rect/Position/Extent

fcf7e67

Rect/Position/Extent

b20ae0d

draw()

96e188c

unsigned idx

37be0ae

Position/Extent

19e0f0f

color args

795cbbb

draw() checks isValid

22c6a56

Texture.drawTiled

1ded529

morganchristiansson added 5 commits July 5, 2026 00:48

setText early return

33c9ee0

makePos simpler

e398e7a

num_x/y > scale

a64dda3

std::vector<Texture>

00cc538

Fix text in load&save

c1c7b98

morganchristiansson force-pushed the gl2-ui branch from 0397f02 to c1c7b98 Compare July 4, 2026 22:48

Extent textSize

5feba0d

morganchristiansson commented Jul 4, 2026

View reviewed changes

Comment thread CIO/CControlContainer.cpp Outdated

Flamefire requested changes Jul 5, 2026

View reviewed changes

morganchristiansson added 7 commits July 5, 2026 15:30

Cleanup and dead code

599428d

drawButtonBox

8b749da

Position/Extent

9f1f347

clamp

506e0a3

getBmpTexture().draw()

c165b70

clang-format

b37716b

unused forward declaration

345ddc6

morganchristiansson force-pushed the gl2-ui branch from c01d7c5 to 345ddc6 Compare July 5, 2026 14:24

Flamefire requested changes Jul 5, 2026

View reviewed changes

morganchristiansson added 10 commits July 5, 2026 19:36

clamp window

e9610ce

Positioning fixes, window size clamped to on screen

a85ffea

glColor is already reset where used

09f9e19

dead code ensureBmpText()

dd1f304

unsigned bmpArray idx

8ab3cb2

inlined origin and winRect

0d3f877

tidy

84608ca

clang-format

5b6284c

Restore docstrings

e364d69

Restore docstrings

66bb4be

	.draw(origin + Position(static_cast<int>(size_.x), static_cast<int>(size_.y))
	.draw(origin + Position(size_))

	dispRect.getOrigin() + Position(dispRect.getSize().x / 2, dispRect.getSize().y / 2);
	dispRect.getOrigin() + dispRect.getSize() / 2u;

Uh oh!

Conversation

morganchristiansson commented Jun 27, 2026

Uh oh!

morganchristiansson commented Jun 29, 2026

Uh oh!

Flamefire commented Jun 30, 2026

Uh oh!

morganchristiansson commented Jun 30, 2026

Uh oh!

morganchristiansson commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganchristiansson commented Jul 3, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

morganchristiansson commented Jul 4, 2026

Uh oh!

morganchristiansson commented Jul 4, 2026

Uh oh!

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson commented Jul 3, 2026 •

edited

Loading

morganchristiansson Jul 5, 2026 •

edited

Loading

morganchristiansson Jul 5, 2026 •

edited

Loading