Skip to content

s25edit backports from s25client #37

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.

Sign up for GitHub

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

Open
morganchristiansson wants to merge 7 commits into
base: master
Choose a base branch
from
Open

s25edit backports from s25client #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
67c147c
Wrap brush for large screens
morganchristiansson Jun 16, 2026
7b71843
Black area for small maps&large screens
morganchristiansson Jun 16, 2026
511324b
Fix mouse panning
morganchristiansson Jun 16, 2026
05fcd18
Zoom with mouse wheel
morganchristiansson Jun 16, 2026
74d768d
Fix segfault when panning map
morganchristiansson Jun 16, 2026
20cd336
fixup! Zoom with mouse wheel
Jun 30, 2026
9694bea
positiveModulo
morganchristiansson Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 81 additions & 46 deletions CGame_Event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,41 @@
#include "callbacks.h"
#include "globals.h"

namespace {

@morganchristiansson morganchristiansson Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why am I using anonymous namespaces like this?

Please advice, my review overlords.

@morganchristiansson morganchristiansson Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it's just file local helper function that is not available from other object files.
So it's fine. Not needed but whatever.
I'd assumed that not being added to .h would've made it inaccessible but I guess this is explicit and goes further.
AI adds these on it's own initiative and then makes a mess and chokes on it when there's multiple namespace scopes in same file.

bool tryZoom(Position deltaTriangle, int deltaInc, CMap* mapObj)
{
if(!mapObj || !mapObj->getMap())
return false;
const int newInc = triangleIncrease + deltaInc;
if(newInc < 1 || newInc > 10)
return false;

callback::PleaseWait(INITIALIZING_CALL);
triangleWidth += deltaTriangle.x;
triangleHeight += deltaTriangle.y;
triangleIncrease += deltaInc;
bobMAP* myMap = mapObj->getMap();
myMap->updateVertexCoords();
CSurface::get_nodeVectors(*myMap);
callback::PleaseWait(WINDOW_QUIT_MESSAGE);
return true;
}

void resetZoom(CMap* mapObj)
{
if(!mapObj || !mapObj->getMap())
return;
callback::PleaseWait(INITIALIZING_CALL);
triangleWidth = TRIANGLE_WIDTH_DEFAULT;
triangleHeight = TRIANGLE_HEIGHT_DEFAULT;
triangleIncrease = TRIANGLE_INCREASE_DEFAULT;
bobMAP* myMap = mapObj->getMap();
myMap->updateVertexCoords();
CSurface::get_nodeVectors(*myMap);
callback::PleaseWait(WINDOW_QUIT_MESSAGE);
}
} // namespace

void CGame::EventHandling(SDL_Event* Event)
{
switch(Event->type)
Expand Down Expand Up @@ -84,45 +119,11 @@ void CGame::EventHandling(SDL_Event* Event)
#endif
// F5 - F7 is ZOOM, F5 = zoom in, F6 = normal view, F7 = zoom out
case SDLK_F5:
if(triangleIncrease < ZOOM_INCREASE_MAX && MapObj->getMap())
{
callback::PleaseWait(INITIALIZING_CALL);
triangleHeight += ZOOM_STEP_HEIGHT;
triangleWidth += ZOOM_STEP_WIDTH;
triangleIncrease += ZOOM_STEP_INCREASE;
bobMAP* myMap = MapObj->getMap();
myMap->updateVertexCoords();
CSurface::get_nodeVectors(*myMap);
callback::PleaseWait(WINDOW_QUIT_MESSAGE);
}
tryZoom(Position(ZOOM_STEP_WIDTH, ZOOM_STEP_HEIGHT), ZOOM_STEP_INCREASE, MapObj.get());
break;
case SDLK_F6:
{
if(MapObj->getMap())
{
callback::PleaseWait(INITIALIZING_CALL);
triangleHeight = TRIANGLE_HEIGHT_DEFAULT;
triangleWidth = TRIANGLE_WIDTH_DEFAULT;
triangleIncrease = TRIANGLE_INCREASE_DEFAULT;
bobMAP* myMap = MapObj->getMap();
myMap->updateVertexCoords();
CSurface::get_nodeVectors(*myMap);
callback::PleaseWait(WINDOW_QUIT_MESSAGE);
}
}
break;
case SDLK_F6: resetZoom(MapObj.get()); break;
case SDLK_F7:
if(triangleIncrease > ZOOM_INCREASE_MIN && MapObj->getMap())
{
callback::PleaseWait(INITIALIZING_CALL);
triangleHeight -= ZOOM_STEP_HEIGHT;
triangleWidth -= ZOOM_STEP_WIDTH;
triangleIncrease -= ZOOM_STEP_INCREASE;
bobMAP* myMap = MapObj->getMap();
myMap->updateVertexCoords();
CSurface::get_nodeVectors(*myMap);
callback::PleaseWait(WINDOW_QUIT_MESSAGE);
}
tryZoom(Position(-ZOOM_STEP_WIDTH, -ZOOM_STEP_HEIGHT), -ZOOM_STEP_INCREASE, MapObj.get());
break;

default: break;
Expand All @@ -142,17 +143,17 @@ void CGame::EventHandling(SDL_Event* Event)

case SDL_MOUSEMOTION:
{
// setup mouse cursor data
if(MapObj && MapObj->isActive())
// Avoid duplicate events especially when warping the mouse back during panning
{
if((Event->motion.state & SDL_BUTTON(SDL_BUTTON_RIGHT)) == 0)
{
Cursor.pos = Position(Event->motion.x, Event->motion.y);
}
} else
{
Cursor.pos = Position(Event->motion.x, Event->motion.y);
static Position lastMousePos = Position::Invalid();
const Position newPos(Event->motion.x, Event->motion.y);
if(newPos == lastMousePos)
break;
lastMousePos = newPos;
}

// setup mouse cursor data
Cursor.pos = Position(Event->motion.x, Event->motion.y);
/*
//NOTE: we will now deliver the data to menus, windows, map etc., sometimes we have to break the
switch and stop
Expand Down Expand Up @@ -384,6 +385,40 @@ void CGame::EventHandling(SDL_Event* Event)
break;
}

case SDL_MOUSEWHEEL:
{
if(!MapObj)
break;

int y = Event->wheel.y;
if(Event->wheel.direction == SDL_MOUSEWHEEL_FLIPPED)
y = -y;
if(y == 0)
break;

const Position deltaTri = (y > 0) ? Position(11, 5) : Position(-11, -5);
const int deltaInc = (y > 0) ? 1 : -1;

// Save values before zoom for cursor-centered adjustment
const PointF oldTriSize(triangleWidth, triangleHeight);
const PointF mousePos(Cursor.pos);

if(!tryZoom(deltaTri, deltaInc, MapObj.get()))
break;

// Keep the map point under the cursor stable after zoom
DisplayRectangle dr = MapObj->getDisplayRect();
const PointF ratio(triangleWidth / oldTriSize.x, triangleHeight / oldTriSize.y);
const auto size = dr.getSize();
const PointF newOrigin = (PointF(dr.getOrigin()) + mousePos) * ratio - mousePos;
dr.left = static_cast<Sint32>(newOrigin.x);
dr.top = static_cast<Sint32>(newOrigin.y);
dr.right = dr.left + size.x;
dr.bottom = dr.top + size.y;
MapObj->setDisplayRect(dr);
break;
}

case SDL_WINDOWEVENT:
{
if(Event->window.event == SDL_WINDOWEVENT_RESIZED)
Expand Down
44 changes: 18 additions & 26 deletions CMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ void CMap::moveMap(Position offset)
void CMap::setMouseData(const SDL_MouseMotionEvent& motion)
{
// following code important for blitting the right field of the map
// Are we scrolling?
// Are we scrolling? (right mouse button held)
if(startScrollPos)
{
assert(motion.state & SDL_BUTTON(3));
Expand All @@ -504,10 +504,9 @@ void CMap::setMouseData(const SDL_MouseMotionEvent& motion)
offset.y = 0;
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);

@Flamefire Flamefire Jun 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@morganchristiansson morganchristiansson Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Flamefire Flamefire Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not a good reason to me.

@morganchristiansson morganchristiansson Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well they're clearly not needed.

@morganchristiansson morganchristiansson Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: removing SDL_EventState(SDL_MOUSEMOTION, SDL_IGNORE)

This change is part of what fixes mouse panning on Linux in this PR. The old SDL_EventState/SDL_IGNORE/SDL_ENABLE dance is a global filter that temporarily drops all motion events — on > Linux (X11/Wayland) the warp-generated event can arrive asynchronously after SDL_ENABLE is called, creating a feedback loop where each motion event warps the mouse, which generates > another event, which warps again, etc. That matches the symptom of panning that stutters and becomes unusable.

The replacement filters by position identity: a motion event with the same coordinates as the previous one is a warp echo and gets skipped. It's strictly more targeted, and it converges > immediately instead of looping.

I know "s25client does it" isn't a strong argument on its own — I mention it more as validation that this approach is well-tested in production, not as the reason for the change. The actual reason is that it fixes a real bug with a more precise filter, replacing a global side-effect that interacts poorly with asynchronous event delivery on Linux.

@morganchristiansson morganchristiansson Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the exact source of frustration causing mouse scrolling to sputter and fail after just a few frames in every PR I work on.

// Warp mouse back to start position so we can keep scrolling
// Duplicate motion events generated by this warp are filtered in CGame::EventHandling
SDL_WarpMouseInWindow(nullptr, startScrollPos->x, startScrollPos->y);
SDL_EventState(SDL_MOUSEMOTION, SDL_ENABLE);
}

storeVerticesFromMouse(Position(motion.x, motion.y), motion.state);
Expand Down Expand Up @@ -984,6 +983,14 @@ void CMap::setKeyboardData(const SDL_KeyboardEvent& key)
}
}

namespace {
constexpr int positiveModulo(int value, int divisor)
{
// C++ % can be negative for negative inputs; this always returns [0, divisor-1]
return ((value % divisor) + divisor) % divisor;
}
} // namespace

void CMap::storeVerticesFromMouse(Position mousePos, Uint8 /*MouseState*/)
{
// if user raises or reduces the height of a vertex, don't let the cursor jump to another vertex
Expand All @@ -995,26 +1002,12 @@ void CMap::storeVerticesFromMouse(Position mousePos, Uint8 /*MouseState*/)

// get X
// following out commented lines are the correct ones, but for tolerance (to prevent to early jumps of the cursor)
// we subtract "triangleWidth/2" Xeven = (MouseX + displayRect.left) / triangleWidth;
Xeven = (mousePos.x + displayRect.left - triangleWidth / 2) / triangleWidth;
if(Xeven < 0)
Xeven += (map->width);
else if(Xeven > map->width - 1)
Xeven -= (map->width - 1);
// we subtract "triangleWidth/2" Xeven = (mousePos.x + displayRect.left) / triangleWidth;
Xeven = positiveModulo((mousePos.x + displayRect.left - triangleWidth / 2) / triangleWidth, map->width);
// Add rows are already shifted by triangleWidth / 2
Xodd = (mousePos.x + displayRect.left) / triangleWidth;
// Xodd = (mousePos.x + displayRect.left) / triangleWidth;
if(Xodd < 0)
Xodd += (map->width - 1);
else if(Xodd > map->width - 1)
Xodd -= (map->width);

MousePosY = mousePos.y + displayRect.top;
// correct mouse position Y if displayRect is outside map edges
if(MousePosY < 0)
MousePosY += map->height_pixel;
else if(MousePosY > map->height_pixel)
MousePosY = mousePos.y - (map->height_pixel - displayRect.top);
Xodd = positiveModulo((mousePos.x + displayRect.left) / triangleWidth, map->width - 1);

MousePosY = positiveModulo(mousePos.y + displayRect.top, map->height_pixel);

// get Y
for(int j = 0; j < map->height; j++)
Expand Down Expand Up @@ -1088,9 +1081,8 @@ void CMap::render()
else
Surf_Map = makeRGBSurface(displayRect.getSize().x, displayRect.getSize().y);
}
// else
// 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));
Comment thread
Flamefire marked this conversation as resolved.

// touch vertex data if user modifies it
if(modify)
Expand Down
10 changes: 5 additions & 5 deletions callbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2819,7 +2819,6 @@ void callback::MinimapMenu(int Param)
{
static CWindow* WNDMinimap = nullptr;
static CMap* MapObj = nullptr;
static SDL_Surface* WndSurface = nullptr;
static int scaleNum = 1;
// only in case INITIALIZING_CALL needed to create the window
int width;
Expand Down Expand Up @@ -2855,13 +2854,15 @@ void callback::MinimapMenu(int Param)
std::make_unique<CWindow>(MinimapMenu, WINDOWQUIT, WindowPos::Center, Extent(width + 12, height + 30),
"Overview", WINDOW_NOTHING, WINDOW_CLOSE | WINDOW_MOVE));
global::s2->RegisterCallback(MinimapMenu);
WndSurface = WNDMinimap->getSurface();
}
break;

case CALL_FROM_GAMELOOP:
if(MapObj && WndSurface)
MapObj->drawMinimap(WndSurface);
if(MapObj && WNDMinimap)
{
if(SDL_Surface* surf = WNDMinimap->getSurface())
MapObj->drawMinimap(surf);
}
break;

case WINDOW_CLICKED_CALL:
Expand Down Expand Up @@ -2893,7 +2894,6 @@ void callback::MinimapMenu(int Param)
WNDMinimap = nullptr;
}
MapObj = nullptr;
WndSurface = nullptr;
global::s2->UnregisterCallback(MinimapMenu);
break;

Expand Down