Radius overlay by morganchristiansson · Pull Request #1946 · Return-To-The-Roots/s25client

morganchristiansson · 2026-06-15T08:54:08Z

Add tooltip and overlay for building ranges

Flamefire

Good idea, makes it a bit S4-like.

@Spikeone Should this be an addon too?

Flamefire · 2026-06-15T09:24:27Z

+constexpr unsigned HUNTER_SEARCH_RADIUS = 19;  ///< Hunter searches for animals in a square of this half-side length
+constexpr unsigned FARMER_RADIUS = 2;         ///< Farmer plants and harvests crops within this radius
+constexpr unsigned CHARBURNER_RADIUS = 3;     ///< Charburner places and harvests charcoal piles within this radius
+constexpr unsigned CATAPULT_RANGE = 12;       ///< Catapult attack range


I see a if(distance < 14) check there. So why 12 here?

Google said 12. :-( I started with my own numbers.
This is fixed.

Also catapult was not available as constant, I extracted it to constant.

Extracting constants is always good. You just have to make sure you actually use them after defining it.

@Spikeone I guess we should move to actually using radius for all jobs, not rectangles.

Probably yes - can check in the original but pretty sure thats it's circular

I see a if(distance < 14) check there. So why 12 here?

Isn't it the same reason our building range is +1? So the target node is included or something like that?

Which building range? if(distance < 14) means the building on the 13th node away can still be hit, so there are 12 nodes in-between. So maybe that is what is meant by google. And maybe the other part you are referring to has a corner case which required that. But would need to check

morganchristiansson · 2026-06-15T15:06:01Z

+        // Draw at all 9 toroidal copies (canonical ± 1 map dimension).
+        // Using all copies guarantees the ring is continuous across the seam
+        // regardless of viewport position — the renderer clips off-screen pixels.
+        for(int dw : {-w, 0, w})
+        {
+            for(int dh : {-h, 0, h})
+            {
+                const MapPoint copyPt = MakeMapPoint(Position(basePt.x + dw, basePt.y + dh), mapSize);
+                const auto alt = world.GetNode(copyPt).altitude;
+                const DrawPoint scr = GetNodePos(copyPt) - DrawPoint(0, HEIGHT_FACTOR * alt) - offset;
+                Window::DrawRectangle(Rect(scr - DrawPoint(2, 2), Extent(5, 5)), BORDER_COLOR);
+            }
+        }


Noone commented on this yet.
The drawn radius was being clipped where map repeats and this is a workaround. Did not find any cleaner way advice appreciated.

That is indeed hard. You can use the GetNodePos overload taking the height as the base point. Then use const Position mapDrawSize = world->GetSize() * Position(TR_W, TR_H); as the offsets

I don't think it gets better than that: You have to draw possibly all copies when the zoom factor is large which it seems is also not yet taken into account. Possibly even that is not enough.
Maybe rather use a (flat_)set to store the radius points and move drawing to DrawGUI where you can check with contains whether to draw the mark

I found that s25edit draws a brush under the cursor which is resizeable by numpad +/- and based the radius outline drawing on that!

Flamefire · 2026-06-16T06:33:04Z

Given #129 was a request for an addon, can you add one and guard the code changing the tooltip and setting the draw-circle by the addon setting? We usually keep the default behavior to be the same as in S2

BTW: Unless you just applied a suggested code snippet directly please don't resolve conversations as then we miss your reply

Spikeone · 2026-06-16T09:01:06Z

@Spikeone Should this be an addon too?

Yes, I think so

morganchristiansson · 2026-06-16T09:36:35Z

I think it should be player toggleable during game, and per player in multiplayer.
It's just a fancy measuring tool so I think this should be fine.
Also I think the list of addons is getting large.

But most ppl said it should be addon so maybe best to proceed with that.

But should tooltip also be guarded by addon?

BTW: Unless you just applied a suggested code snippet directly please don't resolve conversations as then we miss your reply

Ok I will leave resolving to original commenter from now on. I just did it to track what I resolved.

stefson · 2026-06-16T19:13:40Z

nice idea, but please make it an optional addon.

also, the red isn't very much in the style of S2,

morganchristiansson · 2026-06-16T20:21:33Z

nice idea, but please make it an optional addon.

it is now an addon

also, the red isn't very much in the style of S2,

not sure what graphics to use, please provide/suggest.

morganchristiansson · 2026-06-18T20:33:47Z

Ok all issues solved.
There's been calls for better graphics. I'm not a good artist and noone has stepped up. I think for measuring tool/hud the red rectangles are fine.

And made the addon enabled by default - mainly so I could use my existing save for testing.

morganchristiansson · 2026-06-19T16:08:40Z

Found an edge case: it hovers game world buildings thru windows and will draw outline

morganchristiansson · 2026-06-22T14:20:23Z

I have played many games with this enabled and it works perfectly. Hard to play without it.

Only remaining issue is to use custom graphics instead of red rectangles.

Flamefire · 2026-06-30T11:56:45Z

+                return nofFarmhand::GetWorkRadius(*job);
+            return 0;
+        }
+        // All other building types have no relevant radius overlay


Suggested change

// All other building types have no relevant radius overlay

// All other building types have no relevant work radius (e.g. only work inside the building)

Flamefire · 2026-06-30T11:58:08Z

+                      "when hovering over a building icon or selecting a building."),
+                    1) // Enabled by default


Suggested change

"when hovering over a building icon or selecting a building."),

1) // Enabled by default

"when hovering over a building icon or selecting a building."), 0)

The convention is that the default is S2-like behavior, so you can "disable" all addons and get vanilla S2

Yeah can revert this. Making it default enabled allowed me to continue using my save game for testing tho so it's been nice. Also playing campaign with it enabled is lovely.

I'm addicted I can't play without it.

Let's revert to default disable last thing before merge ok?

Flamefire · 2026-06-30T12:03:22Z

+    auto* bldGroup = buildTabCtrl ? buildTabCtrl->GetGroup(buildTabCtrl->GetCurrentTab()) : nullptr;
+
+    bool hasHoveredIcon = false;
+    if(bldGroup && gwv.GetWorld().GetGGS().isEnabled(AddonId::BUILDING_RADIUS))


This looks inefficient doing on every single draw call. What was wrong with the MouseMove-hovering?

There's an issue with icon->SetOnHoverChanged()
When hovering icons in forward direction, the leave event fires after enter event.

Maybe there should be 2 passes, first send leave event, then send enter.

Flamefire · 2026-06-30T12:04:01Z

+        for(auto* icon : bldGroup->GetCtrls<ctrlBuildingIcon>())
+        {
+            const unsigned radius = GetBuildingRadius(icon->GetType());
+            if(icon->IsMouseOver() && radius > 0)


Shouldn't you break once you found ANY mouseover?

Flamefire · 2026-06-30T12:08:35Z


+// -----------------------------------------------------------------------------
+// Snap a point to the nearest toroidal copy relative to a reference position.
+// Same formula as s25edit's correctMouseBlit():


Please don't include such references. The client should be self-contained and the code correct on its own. So rather explain why this specific code is correct as-is.
Also the arguments are not clear to me. Why pass 2 by const-ref? Should all be values.
Can mapPxSize ever be zero? When and why?
Without that it gets much easier to read as you can use e.g. pt -ref etc.

Flamefire · 2026-06-30T12:11:16Z

+    constexpr unsigned BORDER_COLOR = 0xFFFF0000;
+    const DrawPoint mapPxSize(mapSize.x * TR_W, mapSize.y * TR_H);
+
+    // Reference: screen position of the center vertex (with seam offset).


Why "reference"? Isn't this simply a position in screen coordinates? I.e. a DrawPoint from a MapPoint?

Flamefire · 2026-06-30T12:12:13Z

+    const auto centerAlt = world.GetNode(center).altitude;
+    const DrawPoint ref = GetNodePos(center) - DrawPoint(0, HEIGHT_FACTOR * centerAlt) + selPtOffset;


Use the overload:

Suggested change

const auto centerAlt = world.GetNode(center).altitude;

const DrawPoint ref = GetNodePos(center) - DrawPoint(0, HEIGHT_FACTOR * centerAlt) + selPtOffset;

const DrawPoint ref = GetNodePos(center, world.GetNode(center).altitude) + selPtOffset;

Also where does selPtOffset come from? Would need a comment

Flamefire · 2026-06-30T12:15:02Z

+    const auto centerAlt = world.GetNode(center).altitude;
+    const DrawPoint ref = GetNodePos(center) - DrawPoint(0, HEIGHT_FACTOR * centerAlt) + selPtOffset;
+
+    for(const auto& [basePt, dist] : pts)


Suggested change

for(const auto& [basePt, dist] : pts)

for(const auto& [pt, dist] : pts)

Or mapPt or similar, I suspected a function parameter or similar especially with scr not containing "Point" which I guess it should. Maybe mapPt and screenPt?

Flamefire · 2026-06-30T12:17:43Z

 bool GlobalGameSettings::isEnabled(AddonId id) const
 {
    const auto* addon = getAddon(id);
-    return addon && addon->status != addon->addon->getDefaultStatus();


This doesn't seem correct. Please revert

This is to allow default enabled addons.
Ok I find it very useful to have it default enabled for testing. Can we revert this last thing before merge.

Flamefire · 2026-06-30T12:22:18Z

+
+    // Auto-detect radius for the building under the cursor.
+    // Do not trigger hover when the mouse is over an ingame window.
+    if(!radiusPreview_ && GetWorld().GetGGS().isEnabled(AddonId::BUILDING_RADIUS)


Shouldn't that be rather on the outside, e.g. in dskGameInterface where it could be more efficiently in a MouseMove handler. This way it is opt-in and e.g. doesn't enable for observation windows (suspicion only, haven't checked)
And whereever this is done I'd say it makes sense to cache GetWorld().GetGGS().isEnabled(AddonId::BUILDING_RADIUS) (effectively const) instead of getting this on every draw call.

The original logic checks whether status differs from the addon's default (status != getDefaultStatus()). This is the convention used throughout the codebase: an addon is considered 'enabled' when its status has been changed from the default (S2-like) behavior. The previous change to 'status != 0' would break addons with default != 0.

This reverts commit 0ed0c34.

morganchristiansson · 2026-06-30T15:27:19Z

Addressed all feedback.
Ran clang-format.
24 commits so far getting a bit messy maybe could squash it.
Or I would've liked to split it but it's only usable and testable as a whole.

Flamefire · 2026-06-30T15:37:20Z


 bool dskGameInterface::Msg_MouseMove(const MouseCoords& mc)
 {
+    // Update radius preview for buildings under the cursor (event-driven, not in draw loop)


Make sure to clean such "intermediate" comments from AI

Suggested change

// Update radius preview for buildings under the cursor (event-driven, not in draw loop)

// Update radius preview for buildings under the cursor

Flamefire · 2026-06-30T15:38:01Z

                std::stringstream tooltip;
                tooltip << _(BUILDING_NAMES[bld]);

+                // Radius anzeigen falls vorhanden


English comments only (at least for new/changed ones)

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

+                unsigned radius = 0;
+                if(showBuildingRadius)
+                    radius = GetBuildingRadius(bld);


Prefer const variables:

Suggested change

unsigned radius = 0;

if(showBuildingRadius)

radius = GetBuildingRadius(bld);

const unsigned radius = showBuildingRadius ? GetBuildingRadius(bld) : 0;

Flamefire · 2026-06-30T15:39:38Z

+                // Store hover callback; activeHoveredIcon_ guards against stale leaves from
+                // reversed child iteration order in Msg_MouseMove dispatch


Similar AI comment unless I'm missing something this is completely redundant

Suggested change

// Store hover callback; activeHoveredIcon_ guards against stale leaves from

// reversed child iteration order in Msg_MouseMove dispatch

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

    if(show_names || show_productivity)
        DrawNameProductivityOverlay(terrainRenderer);

+    // Draw radius preview outline (set by icon-hover or map-hover)


this comment is about the caller and hence can easily be outdated. Avoid please

Suggested change

// Draw radius preview outline (set by icon-hover or map-hover)

// Draw radius preview outline if enabled

Flamefire · 2026-06-30T15:59:53Z

+
+void GameWorldView::UpdateRadiusPreviewForMousePos(const Position& mousePos)
+{
+    if(!isBuildingRadiusEnabled_ || WINDOWMANAGER.FindWindowAtPos(mousePos))


Hm, this looks like this check should rather be handled by the caller: You update by selPt not mousePos and that is only set/updated in the Draw call so likely to be stale by 1 frame anyway.
Passing a value into this class just to check it using a different layer (world vs windows) feels wrong

Maybe we can turn it around: Add a method that gets the building type at the selection which we can reuse in dskGameInterface::ContextClick too, see existing code.
Do we really want to handle non-owned buildings? Feels like a cheat... If we do the code will be different so then I'd say a method getSelectedBuildingType here and handling the mouse pos/window-manager and buildings-enabled in the caller is cleaner.

Flamefire · 2026-06-30T16:13:45Z

 }

+// Snap a point to the nearest toroidal copy (map wrapping) relative to a reference.
+// k = round((reference - pt) / mapSize), pt += k * mapSize.


You can (pretty much) directly express this below like k = PointF(ref - pt) / mapPxSize (maybe a better name than k) then pt.x += DrawPoint(round(k.x), round(k.y)) * mapPxSize

And a short comment why this formula is correct/used would be good. I.e. what is this k and what does it mean. This way would keep us wondering later.

morganchristiansson added 5 commits June 14, 2026 18:24

Radius overlay & tooltips

fa453a0

Fix clipping, mine radius

3175b05

Cleaner DrawRadiusOutline

d489b4c

FoW and cleaner DrawRadiusOutline

5728636

Tidy

d6c53b8

Flamefire requested changes Jun 15, 2026

View reviewed changes

morganchristiansson added 3 commits June 15, 2026 16:25

Improve radius constants

11df35a

Replace boost::optional and boost::none

0572598

Simpify UI/mouse changes

cfebdce

morganchristiansson commented Jun 15, 2026

View reviewed changes

Fix map wrap clipping again

795a110

morganchristiansson commented Jun 15, 2026

View reviewed changes

Comment thread libs/s25main/figures/nofHunter.cpp

This was referenced Jun 15, 2026

Hunter: Use GetPointsInRadius() instead of square #1948

Open

[addon] Visible workarea for buildings #129

Open

Fix "Invalid job id" from GetWorkRadius()

bdfd9b1

morganchristiansson added 3 commits June 16, 2026 13:12

Toggle via addon

7957d83

Ensure hover fires after leave

1f70192

Fix formatting

4f4fa6e

morganchristiansson added 2 commits June 18, 2026 22:29

Better radius ouline drawing

7ec4ce5

Enable addon by default

ab91acd

morganchristiansson marked this pull request as ready for review June 18, 2026 20:30

Ignore game buildings when mouse over window

16840a8

Flamefire requested changes Jun 30, 2026

View reviewed changes

morganchristiansson and others added 8 commits June 30, 2026 12:58

Revert "Revert isEnabled change: restore original semantics"

e4df373

This reverts commit 0ed0c34.

Move OnHover() callsite to Msg_MouseMove

675e474

UpdateRadiusPreviewForMousePos

0f408f0

SnapToNearestCopy

caef896

DrawRadiusOutline

ae81945

GetBuildingRadius comment wording

93246c4

clang-format

83752d2

Flamefire requested changes Jun 30, 2026

View reviewed changes

	// All other building types have no relevant radius overlay
	// All other building types have no relevant work radius (e.g. only work inside the building)

		"when hovering over a building icon or selecting a building."),
		1) // Enabled by default

		const auto centerAlt = world.GetNode(center).altitude;
		const DrawPoint ref = GetNodePos(center) - DrawPoint(0, HEIGHT_FACTOR * centerAlt) + selPtOffset;

	const auto centerAlt = world.GetNode(center).altitude;
	const DrawPoint ref = GetNodePos(center) - DrawPoint(0, HEIGHT_FACTOR * centerAlt) + selPtOffset;
	const DrawPoint ref = GetNodePos(center, world.GetNode(center).altitude) + selPtOffset;

	for(const auto& [basePt, dist] : pts)
	for(const auto& [pt, dist] : pts)

	// Update radius preview for buildings under the cursor (event-driven, not in draw loop)
	// Update radius preview for buildings under the cursor

		// Store hover callback; activeHoveredIcon_ guards against stale leaves from
		// reversed child iteration order in Msg_MouseMove dispatch

	// Draw radius preview outline (set by icon-hover or map-hover)
	// Draw radius preview outline if enabled

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Flamefire left a comment

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!

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!

Uh oh!

Uh oh!

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

Uh oh!

Flamefire commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Spikeone commented Jun 16, 2026

Uh oh!

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

Uh oh!

Uh oh!

stefson commented Jun 16, 2026

Uh oh!

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

Uh oh!

Uh oh!

morganchristiansson commented Jun 18, 2026

Uh oh!

morganchristiansson commented Jun 19, 2026

Uh oh!

morganchristiansson commented Jun 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

morganchristiansson commented Jun 15, 2026 •

edited

Loading

morganchristiansson Jun 15, 2026 •

edited

Loading

Flamefire commented Jun 16, 2026 •

edited

Loading

morganchristiansson commented Jun 16, 2026 •

edited

Loading

morganchristiansson commented Jun 16, 2026 •

edited

Loading

morganchristiansson Jun 30, 2026 •

edited

Loading

morganchristiansson Jun 30, 2026 •

edited

Loading