s25edit backports from s25client by morganchristiansson · Pull Request #37 · Return-To-The-Roots/s25edit

morganchristiansson · 2026-06-16T15:03:24Z

Allow window resizing
Fix segfault with small map&large screen
Fix black area with small map&large screen
Fix mouse panning on Linux
Zoom with mouse wheel

Flamefire

Thanks for the PR, that definitely improves it!
Some notes inline, a common theme is that any pair of x/y or width/height values should use either Point/PointF or Extent which avoids C&P errors and makes processing easier (single assignment instead of 2, operator support for combining with other Points)
Note: In some places fixed-size variants are used. That is historical and not necessary in almost all cases, so no need to introduce new uses of them unless there is a specific reason for the size requirement.

Thanks again!

Flamefire · 2026-06-16T15:28:05Z

    // we subtract "TRIANGLE_WIDTH/2"  Xeven = (MouseX + displayRect.left) / TRIANGLE_WIDTH;
    Xeven = (MouseX + displayRect.left - TRIANGLE_WIDTH / 2) / TRIANGLE_WIDTH;
-    if(Xeven < 0)
+    while(Xeven < 0)


IIRC this could be a module operation and a single check to account for negative results (possible because neg. values are implementation defined)

Yeah agree. This was just smallest/simplest fix to a segfault and I didn't care to correct it.
But you're right we should use modulo operation.

Flamefire · 2026-06-16T15:28:45Z

+    while(MousePosY < 0)
        MousePosY += map->height_pixel;
-    else if(MousePosY > map->height_pixel)
+    while(MousePosY > map->height_pixel)


This might be an infinite loop

Flamefire · 2026-06-16T15:30:50Z

        moveMap(offset);

-        // this whole "warping-thing" is to prevent cursor-moving WITHIN the window while user moves over the map
-        SDL_EventState(SDL_MOUSEMOTION, SDL_IGNORE);


Why the change? Isn't handling this "issue" here better where it is caused than where mouse moves should be handled?
IIRC SDL offers a relative mouse mode. As we only support SDL here that might be the best solution if we change this

This is matching current s25client VideoSDL2.cpp which doesn't use SDL_EventState.

Flamefire · 2026-06-16T15:35:21Z

+    if(!mapObj || !mapObj->getMap())
+        return;
+    callback::PleaseWait(INITIALIZING_CALL);
+    TRIANGLE_HEIGHT = 28;


Can we have constants for that? Best using e.g. constexpr Extent DEFAULT_TRIANGLE_SIZE(28, 56) and use that here and in the initialization

This is how zoom is currently implemented in s25editor, by writing to uppercase variables.
I just extracted to shared function for mouse wheel zoom.
The zoom is listed as experimental in F1 help.

Flamefire · 2026-06-16T15:36:53Z

+            if(y == 0)
+                break;
+
+            const int deltaTH = (y > 0) ? 5 : -5;


Please use the Point/Position/Extent for X/Y or W/H values. We had too many C&P issues in the past

Flamefire · 2026-06-16T15:40:25Z

+            const float ratioX = TRIANGLE_WIDTH / oldWidth;
+            const float ratioY = TRIANGLE_HEIGHT / oldHeight;
+            const auto size = dr.getSize();
+            dr.left = static_cast<Sint32>((dr.left + mouseX) * ratioX - mouseX);


This can then use the Point operators like (dr.origin + mousePos) * ratioX - mousPos
Maybe even just dr.move()

Flamefire · 2026-06-16T15:41:19Z

+
+        case SDL_WINDOWEVENT:
+        {
+            switch(Event->window.event)


As long as this is the only even please use a simple if which is less verbose and avoids the double-break

s25client also uses switch with single case and double-break https://github.com/Return-To-The-Roots/s25client/blob/f299328f2109b8cfb5c13edb8f1863eef6da4f07/extras/videoDrivers/SDL2/VideoSDL2.cpp#L340-L357

Flamefire · 2026-06-16T15:43:32Z

+    GameResolution.x = width;
+    GameResolution.y = height;
+    displayTexture_.reset();
+    displayTexture_ = makeSdlTexture(renderer_, SDL_PIXELFORMAT_ARGB8888, SDL_TEXTUREACCESS_STREAMING, width, height);


The same code is in ReCreateWindow, and maybe somewhere else. Can you make those call this function (or the other way round if better)?

Flamefire · 2026-06-16T15:43:47Z

    return true;
 }

+void CGame::UpdateDisplaySize(unsigned width, unsigned height)


Use Extent for the parameter

Flamefire · 2026-06-16T15:45:05Z

-    // clear the surface before drawing new (in normal case not needed)
-    // SDL_FillRect( Surf_Map, nullptr, SDL_MapRGB(Surf_Map->format,0,0,0) );
+    // Clear the surface before drawing so areas not covered by triangles stay black
+    SDL_FillRect(Surf_Map.get(), nullptr, SDL_MapRGBA(Surf_Map->format, 0, 0, 0, 0));


Even though it may have been intended at some point: Why is this required?

To draw black background for small map&large screen.
Otherwise mouse and map leave trails that are not drawn over.

morganchristiansson · 2026-06-16T21:05:28Z

Maybe I should split this into smaller PRs.

I was working on import heightmap as I posted about on Discord, not sure I will submit as PR. And these were some quality of life improvements that felt important.

morganchristiansson added 5 commits June 16, 2026 16:22

Allow window resizing

e441969

Wrap mouse for large screens

dc864f1

Black area for small maps&large screens

29cdd56

Fix mouse panning

0c7a57a

Zoom with mouse wheel

695a60f

Flamefire requested changes Jun 16, 2026

View reviewed changes

Fix segfault when panning map

604b117

morganchristiansson force-pushed the resize-window branch from 67f2175 to 604b117 Compare June 16, 2026 16:11

morganchristiansson added 2 commits June 16, 2026 22:33

fixup! Wrap mouse for large screens

b9a064d

Use Extent and PointF

31b1e61

Conversation

morganchristiansson commented Jun 16, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganchristiansson Jun 16, 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!

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!

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!

morganchristiansson commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

morganchristiansson Jun 16, 2026 •

edited

Loading