News:

SimuTranslator
Make Simutrans speak your language.

6828 koord/scr_coord tidy

Started by kierongreen, October 19, 2013, 04:10:45 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

kierongreen

Patch adds new functions
koord.h: set_min( koord k_min ), set_max( koord k_max )
display/scr_coord.h: set_min( scr_coord scr_min ), set_max( scr_coord scr_max )

These check the koord or scr_coord and apply a minimum or maximum value if they are currently outside this. This tidies a few sections of code.

Markohs

#1
 Good idea.

I'd suggest anyway koord clamp(koord min, kood max) , just one function, that does the same.

But I don't think set_min and set_max are a bad idea.


inline void clamp( koord k_min, koord k_max )
{
    if ( x < k_min.x  ||  x > k_max.x ) {
        x = min(max(k_min.x,x ), k.max.x);
    }
    if ( y < k_min.y  || y > k_max.y ) {
        y = min(max(k_min.y,y ), k.max.y);
    }
}


I just typed it in notepad, might be wrong code.

kierongreen

In most cases there's code like this:

         lo.set_max(gb_pos);
         ur.set_min(gb_pos);

Hence the max and min are being applied to different coordinates so a clamp function wouldn't be useful.

There's only actually one occasion both min and max are needed:

   outside_pos.set_min(koord(0,0));
   outside_pos.set_max(koord(get_size().x-1,get_size().y-1));

using the existing clamp(int, int_min, int_max) this could also be written as

   clamp(outside_pos.x, 0, get_size().x-1);
   clamp(outside_pos.y, 0, get_size().y-1);

Creating a clamp(koord, koord_min, koord_max) which is only used once seems a bit overkill.

Markohs


prissi

perhaps rather use clip_lefttop or clip_rightbottom if you want to keep the way it was introduced in scr_coord. Or may clip_lt and clip_rb, or clip_under, clip_over, or (my least choice) clip_min, clip_max because there is no order to koord.

But the idea is good. As soone as you decide on the name you like best (whatever it will be), submit it.

Ters

The intuitive behaviour (to me at least) for set_min and set_max is to specify what get_min and get_max will subsequently return. That's not the case here. Even if get_min and get_max existed, the behaviour would be unexpected of something looking like a setter. clamp... or clip... would be better names.

Markohs

how about .clamp_max and .clamp_min?

Ters is right, set_max sounds like you are setting a maximum value for that structure, that could persist all subsequent assignments.

Max-Max

I think clamp or limit would be a more proper term here. Usually there is both a min and max value (a range) to clamp/limit against.
- My code doesn't have bugs. It develops random features...

kierongreen

Ok - for the final time, in all but one case the code is only wanting to limit to a max or a min. So checking both is just going to slow things down.

prissi

That was why there was the suggestion to functions like clip_min or clip_max ...

kierongreen

Yes - I've implemented these for scr_coord here (where it makes sense). Just trying to think of equivalents for koord maybe just use clip for that too.

isidoro

Quote from: Markohs on October 19, 2013, 05:01:18 PM
[...]

inline void clamp( koord k_min, koord k_max )
{
    if ( x < k_min.x  ||  x > k_max.x ) {
        x = min(max(k_min.x,x ), k.max.x);
    }
    if ( y < k_min.y  || y > k_max.y ) {
        y = min(max(k_min.y,y ), k.max.y);
    }
}

[...]

Why not just:

inline void clamp( koord k_min, koord k_max )
{
    x = min(max(k_min.x,x ), k.max.x);
    y = min(max(k_min.y,y ), k.max.y);
}
?

Sometimes with modern deep pipelined processors, it is better to do just the calculation than to jump.  Nevertheless, current trend is to avoid optimizations and let the compiler do its job, so that the code is high level and easy to understand...

kierongreen

Quote from: isidoro on October 20, 2013, 12:19:50 AM
Why not just:

inline void clamp( koord k_min, koord k_max )
{
    x = min(max(k_min.x,x ), k.max.x);
    y = min(max(k_min.y,y ), k.max.y);
}
?

Sometimes with modern deep pipelined processors, it is better to do just the calculation than to jump.  Nevertheless, current trend is to avoid optimizations and let the compiler do its job, so that the code is high level and easy to understand...
Particularly when you then just end up repeating testing conditions. Anyway I'm not planning to use min and max I'm just going to use conditions - code in the patch is (remembering that function names are likely to change):

inline void set_min( koord k_min )
{
if (x < k_min.x) {
x = k_min.x;
}
if (y < k_min.y) {
y = k_min.y;
}
}

inline void set_max( koord k_max )
{
if (x > k_max.x) {
x = k_max.x;
}
if (y > k_max.y) {
y = k_max.y;
}
}

This is really as simple as you can make it.

Markohs

To make it short: clip_max or clamp_max, whatever, but not set_max. :)

kierongreen

Added koord clip_min and clip_max to trunk in 6839. Have not added scr_coord clip_lefttop and clip_rightbottom yet as this will require a much more extensive patch.