The International Simutrans Forum

 

Author Topic: 6828 koord/scr_coord tidy  (Read 3340 times)

0 Members and 1 Guest are viewing this topic.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
6828 koord/scr_coord tidy
« on: October 19, 2013, 04:10:45 PM »
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.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: 6828 koord/scr_coord tidy
« Reply #1 on: October 19, 2013, 05:01:18 PM »
 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.

Code: [Select]
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.
« Last Edit: October 19, 2013, 05:07:55 PM by Markohs »

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: 6828 koord/scr_coord tidy
« Reply #2 on: October 19, 2013, 05:13:42 PM »
In most cases there's code like this:
Code: [Select]
         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:
Code: [Select]
   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
Code: [Select]
   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.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: 6828 koord/scr_coord tidy
« Reply #3 on: October 19, 2013, 05:53:43 PM »
okay then ;)

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10636
  • Languages: De,EN,JP
Re: 6828 koord/scr_coord tidy
« Reply #4 on: October 19, 2013, 07:03:20 PM »
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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5691
  • Languages: EN, NO
Re: 6828 koord/scr_coord tidy
« Reply #5 on: October 19, 2013, 08:19:27 PM »
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.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: 6828 koord/scr_coord tidy
« Reply #6 on: October 19, 2013, 08:42:17 PM »
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.

Offline Max-Max

  • Coder/patcher
  • *
  • Posts: 670
    • MK Development
  • Languages: SV,EN
Re: 6828 koord/scr_coord tidy
« Reply #7 on: October 19, 2013, 10:18:51 PM »
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.

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: 6828 koord/scr_coord tidy
« Reply #8 on: October 19, 2013, 10:20:35 PM »
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.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 10636
  • Languages: De,EN,JP
Re: 6828 koord/scr_coord tidy
« Reply #9 on: October 19, 2013, 10:22:23 PM »
That was why there was the suggestion to functions like clip_min or clip_max ...

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: 6828 koord/scr_coord tidy
« Reply #10 on: October 19, 2013, 10:23:55 PM »
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.

Offline isidoro

  • Devotee
  • *
  • Posts: 1140
Re: 6828 koord/scr_coord tidy
« Reply #11 on: October 20, 2013, 12:19:50 AM »
[...]
Code: [Select]
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:
Code: [Select]
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...

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: 6828 koord/scr_coord tidy
« Reply #12 on: October 20, 2013, 12:51:49 AM »
Why not just:
Code: [Select]
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):
Code: [Select]
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.

Offline Markohs

  • DevTeam, Coder/patcher
  • Devotees (Inactive)
  • *
  • Posts: 1559
  • Languages: EN,ES,CAT
Re: 6828 koord/scr_coord tidy
« Reply #13 on: October 20, 2013, 02:38:29 AM »
To make it short: clip_max or clamp_max, whatever, but not set_max. :)

Offline kierongreen

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 2346
Re: 6828 koord/scr_coord tidy
« Reply #14 on: October 20, 2013, 11:34:05 AM »
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.