Change x,y->Point w,h->Extent by morganchristiansson · Pull Request #40 · Return-To-The-Roots/s25edit

morganchristiansson · 2026-06-18T12:46:51Z

Based on this comment #37 (review)

Replace raw x,y and w,h parameters with Position and Extent types across
CMap, CSurface, CControlContainer, CButton, CPicture, CTextfield, and
CFont to make X/Y vs W/H semantics explicit and prevent copy-paste errors.

CMap/CSurface: vertex/modification methods take Position pos instead
of separate int x, int y / int VertexX, VertexY / Uint16 MouseX, MouseY
CControlContainer: addButton/addText/addPicture/addStaticPicture/addTextfield
take Point16 pos / Extent16 size (consistent with addSelectBox)
CButton/CPicture/CTextfield/CFont: constructors take Point16 pos /
Extent16 size with internal storage renamed to pos_/size_
All call sites in callbacks.cpp and CDebug.cpp updated

Flow86 · 2026-06-18T13:03:05Z

+    this->pos_ = pos;
+    this->size_ = size;


please put those directly into the constructors initializer list for new code.

In this case it doesnt change any relevant order.

Flow86 · 2026-06-18T13:03:46Z

 {
    // cursor is on the button (and mouse button not pressed while moving on the button)
-    if((motion.x >= x_) && (motion.x < x_ + w) && (motion.y >= y_) && (motion.y < y_ + h))
+    if((motion.x >= pos_.x) && (motion.x < pos_.x + size_.x) && (motion.y >= pos_.y) && (motion.y < pos_.y + size_.y))


I thought we also had something like this in the code IsPointInExtent or so

IsPointInExtent is now used

Flow86 · 2026-06-18T13:04:40Z

        }
    } else if(button_text)
-        CFont::writeText(Surf_Button, button_text, (int)w / 2, (int)((h - 11) / 2), FontSize::Medium, button_text_color,
+        CFont::writeText(Surf_Button, button_text, Position((int)size_.x / 2, (int)((size_.y - 11) / 2)), FontSize::Medium, button_text_color,


C cast still necessary? if so, replace by static_cast instead

cast removed.

Flow86 · 2026-06-18T13:06:42Z

 #pragma once

 #include "SdlSurface.h"
+#include "defines.h"


not needed here?

It's needed because CPicture.h uses Point16 and Extent16 types for the new pos_ and size_ member
variables:

Point16 pos_; Extent16 size_;

These typedefs are defined in defines.h:

using Point16 = Point<Sint16>; using Extent16 = Point<Uint16>;

Can now be removed again, can't it?

Flow86 · 2026-06-18T13:08:00Z

this can't be rendered on github - what the heck happened?

It works for me. But it's very large diff :-D

Flamefire · 2026-06-18T14:08:31Z

        }
    } else if(button_text)
-        CFont::writeText(Surf_Button, button_text, Position((int)size_.x / 2, (int)((size_.y - 11) / 2)),
+        CFont::writeText(Surf_Button, button_text, Position(static_cast<int>(size_.x) / 2, static_cast<int>(size_.y - 11) / 2),


Why is the cast required?

cast removed.
AI loves adding casts unfortunately.

Flamefire · 2026-06-18T14:14:26Z

 {
    // cursor is on the button (and mouse button not pressed while moving on the button)
-    if((motion.x >= x_) && (motion.x < x_ + w) && (motion.y >= y_) && (motion.y < y_ + h))
+    if(IsPointInRect(Position(motion.x, motion.y), Rect(Position(pos_), Extent(size_))))


Why the cast (Position(pos_))? Should work directly, doesn't it?
Similar below?

If it cannot convert the Point16 to Position automatically then I'd say we should store them as Position/Extent. I mentioned that earlier that in almost all cases the default is fine instead of the sized-variants. As this seems to be an automatic conversion there is no real need to change that too, but if it causes issues like here(?) then it makes sense to change it at least for this class

CButton.cpp: size_ is Extent16 = Point. Uint16 implicitly promotes to int for arithmetic (standard
integral promotion), and Position(int, int) accepts the promoted values directly. So Position(size_.x / 2,
(size_.y - 11) / 2) works fine.

Do you want to go ahead and change ALL Point16 to Position in the codebase and remove Point16 type?

At least where it requires extra work like here.
I'm not sure if there is any use of the sized Point types, maybe in some file operations. But generally the ~4 extra bytes per control/window don't matter (anymore), so yes it makes sense to replace them to make them easier to handle and reason about

Ok went all the way and pushed usage of Point/Extent in UI mainly all the way to the C SDL functions.

morganchristiansson · 2026-06-22T23:40:25Z

Can we get this one out of the way?

It's a pure refactor that doesn't make any changes and there's no reason for it to sit in queue.

I stupidly based another branch on this thinking it would be quick to merge.

Flamefire · 2026-06-23T09:08:10Z

There is an open question

Flamefire

Better, thanks. 2 things:

Offsets should be signed. It could use a typedef like using Offset = Point<int> for clarity.
Lot's of introduced casts. Double-check where they are actually required. Many look uneccessary to me.

Flamefire · 2026-06-23T12:45:29Z

    // read start addresses
-    for(int y = 0; y < shadowArray->h; y++)
-        CHECK_READ(libendian::le_read_us(&starts[y], fp));
+    for(Position pos{0, 0}; pos.y < shadowArray->h; pos.y++)


You don't like Position in for loop?
Was originally going to leave for loops alone, but there's a lot of x, y usage in them and this is actually nice I think.

But hmm this is CFile, not UI. Maybe should not convert CFile. There was bug with unitialized pos from changes here.
And why is CFile in same folder as all UI classes anyway.

In this loop you loop over y only, so that's not a position

ok reverted the y-only loop. Kept Positio for x,y loops.

Flamefire · 2026-06-23T12:46:18Z

    // position in the Surface 'Surf_Button'
-    Uint16 pos_x = 0;
-    Uint16 pos_y = 0;
+    unsigned pos_x = 0;


Why not convert those too?

Flamefire · 2026-06-23T12:47:27Z

 #pragma once

 #include "SdlSurface.h"
+#include "defines.h"


Can now be removed again, can't it?

Flamefire · 2026-06-23T12:50:35Z

    // position in the Surface 'Surf_Button'
-    Uint16 pos_x = 0;
-    Uint16 pos_y = 0;
+    unsigned pos_x = 0;


Flamefire · 2026-06-23T12:51:30Z

@@ -340,45 +340,45 @@ bool CTextfield::render()
                // black frame is left and up
                // draw vertical line
                pos_x = 0;


Those variable values can be inlined (Position(0, y))

inlined to expressions

- pos_x = size_.x - 1; for(unsigned y = 0; y < size_.y; y++) - CSurface::DrawPixel_RGB(Surf_Text, Position(pos_x, y), 0, 0, 0); + CSurface::DrawPixel_RGB(Surf_Text, Position(size_.x - 1, y), 0, 0, 0);

morganchristiansson · 2026-06-23T14:00:25Z

(force pushed to apply clang-format to previously pushed commit)

morganchristiansson · 2026-06-23T21:27:47Z

Please can we keep pushing this one as it will cause conflicts with other PRs

morganchristiansson force-pushed the xy-point-extent branch 4 times, most recently from a2e78ad to 276f709 Compare June 18, 2026 13:02

Use Position/Extent for x,y/w,h values

a47e263

morganchristiansson force-pushed the xy-point-extent branch from 276f709 to a47e263 Compare June 18, 2026 13:06

Flow86 reviewed Jun 18, 2026

View reviewed changes

Your Name added 3 commits June 18, 2026 15:26

Merge branch 'master' into xy-point-extent

001edc8

Fix GH actions failure

442eeec

Apply PR feedback: init lists, IsPointInRect, static_cast, cleanup

fecdae7

Flamefire reviewed Jun 18, 2026

View reviewed changes

Comment thread callbacks.cpp Outdated

Flamefire reviewed Jun 18, 2026

View reviewed changes

Your Name added 3 commits June 18, 2026 16:18

Remove unnecessary static_cast<int> casts

be4e71f

Fix GH actions failure

8a46597

Merge remote-tracking branch 'upstream/master' into xy-point-extent

e94fd30

morganchristiansson mentioned this pull request Jun 18, 2026

s25edit backports from s25client #37

Open

morganchristiansson requested review from Flamefire and Flow86 June 22, 2026 23:36

morganchristiansson mentioned this pull request Jun 22, 2026

Align USD texture offset on even rows with s25client #48

Closed

Flamefire requested changes Jun 23, 2026

View reviewed changes

morganchristiansson and others added 2 commits June 23, 2026 15:59

Push Position/Extent all the way to SDL boundary

7e38250

Address PR feedback: Position usage, signed offset, loop cleanup

89c0f88

morganchristiansson force-pushed the xy-point-extent branch from f179a30 to 89c0f88 Compare June 23, 2026 13:59

morganchristiansson added 2 commits June 23, 2026 17:26

Fix duplicate pic variable

d8647dd

Merge remote-tracking branch 'upstream/master' into xy-point-extent

384b53d

morganchristiansson force-pushed the xy-point-extent branch from 9583ae4 to 384b53d Compare June 23, 2026 15:29

Flamefire merged commit 2ef8ed5 into Return-To-The-Roots:master Jun 25, 2026
1 check passed

Uh oh!

Conversation

morganchristiansson commented Jun 18, 2026

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 Jun 18, 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!

Uh oh!

Flamefire Jun 18, 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!

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

morganchristiansson commented Jun 22, 2026

Uh oh!

Flamefire commented Jun 23, 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!

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 Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

morganchristiansson Jun 18, 2026 •

edited

Loading

Flamefire Jun 18, 2026 •

edited

Loading

morganchristiansson Jun 23, 2026 •

edited

Loading

morganchristiansson Jun 23, 2026 •

edited

Loading