The International Simutrans Forum

 

Author Topic: [BUG] scr_size overflow  (Read 563 times)

0 Members and 1 Guest are viewing this topic.

Offline Ranran

  • Devotee
  • *
  • Posts: 997
  • Languages: ja
[BUG] scr_size overflow
« on: May 18, 2020, 06:32:40 PM »
In the Bridgewater-Brunel server, I noticed that the factory list's scrollbar is not visible.
I did some research on this and it seems that this is due to the size overflow due to too many factories in the map.
The Bridgewater-Brunel server has 3429 factories. The standard linespace of extended is 12 pixels, so the required vertical size is 3429 * 12 = 41148.

scr_coord is defined in sint16.
Code: [Select]
// Screen coordinate type
typedef sint16 scr_coord_val;
So the Bridgewater-Brunel server example would return a negative value.

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #1 on: May 18, 2020, 11:01:52 PM »
These scr_coord_val overflows are EVERYWHERE...

I was preparing a slightly different approach on mapping minimaps koords to screen koords, to allow for zooming in there.
In principle the same could be applied to the industry list and whatever, efectively the smallest posible scroll unit would then be a line, instead of a sceen pixel, but I guess it's best to reject this project in its early stage and instead change scr_coord_val to be an sint32.

However, changing this is not too simple either. It's used by very many classes and many of them rely on scr_coord_val being sint16.

Anyways, we'll need a fix as these overflows seem to appear in many places and I guess changing to sint32 is the easiest.


Is there any easier or better alternative suggestion on fixing the scr_coord_val overflows on the minimap, the industry list and wherever else they appear?

Offline Ranran

  • Devotee
  • *
  • Posts: 997
  • Languages: ja
Re: [BUG] scr_size overflow
« Reply #2 on: May 19, 2020, 09:10:30 AM »
In standard, it seems that it was extended to sint32 when GUI overhaul was executed in r8653.
phystam is in the process of incorporating changes from the standard but it would take a great deal of effort to incorporate the entire that commit...
I don't know if it can be partially included. That's has a huge change.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #3 on: May 19, 2020, 07:19:33 PM »
It does seem sensible to use 32-bit integers for screen co-ordinates, as there really is no sensible benefit to making them 16 bit. As to merging from Standard, this is always an extremely challenging task. However, could not the change to 32-bit be made independently of a merge, since, if done carefully, it would not produce an extra conflicts on merging the Standard changes?

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #4 on: May 19, 2020, 07:40:25 PM »
Well "Simply" changing so sint32 is also rather challenging here.
There are innumerable uses ob both display koord types and many of them need to be changed in order to work properly with sint32.
That's why my attempt to fix the minimap zoom was not to "simply" change to sint32, but to use a different koordinate system, as I expected this to be less effort to implement than "simply" changing to sint32.

Anyways, as the sint32 will also fix further overflow issues, I'd prefer that solution now. I might try to implement this change.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #5 on: May 19, 2020, 08:30:50 PM »
Well "Simply" changing so sint32 is also rather challenging here.
There are innumerable uses ob both display koord types and many of them need to be changed in order to work properly with sint32.
That's why my attempt to fix the minimap zoom was not to "simply" change to sint32, but to use a different koordinate system, as I expected this to be less effort to implement than "simply" changing to sint32.

Anyways, as the sint32 will also fix further overflow issues, I'd prefer that solution now. I might try to implement this change.

It would be very helpful if you could - thank you.

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #6 on: May 19, 2020, 11:44:35 PM »
There are very many rdwrs affected, so the savegame format/version will change.

How do I properly handle this?
Let's take that simple example:
Code: [Select]
xml_tag_t k( file, "koord" );
file->rdwr_short(x);
file->rdwr_short(y);
I guess I need an sint16 dummy to read the value from old savegames, but which version do I need to compare? And which versions do I need to increase?

I am a little confused of all these different versions in simversion.h

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #7 on: May 20, 2020, 06:25:02 PM »
It's making progress, but never did such an annoying refactoring.
216 variables are declared as scr_coord_val and all of these could potentially rely on actually being sint16. Whenver rdwr is involved, they actually rely on it.

Again, I'll need some help to properly handle the resulting new savegame revision.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 237
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #8 on: May 20, 2020, 09:57:29 PM »
Oh dear, that makes at least three patches requiring a new savegame version. James - maybe they could all be merged at once?

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #9 on: May 20, 2020, 11:00:11 PM »
Good news, I have to correct my previous statements:
1. It was quite a lot of potentially affected code, but in the end it was a rather small change spread across 15 files.
2. The savegame version is not affected. It's only the user preferences "save" that seems to be affected, so simply not handling this will only reset user preferences.
I had a look at standard and it seems they simply didn't handle it either. So either we don't handle it or anyone who knows what he's doing can add this to the code. The only affected location of that kind i

I hope I have really found all places in the code where manual changes are required, but it seems to be working and running stable.

Bad news, the current state cannot be seen on my github yet, as it broke after merging in latest master and it seems to be conflicting with ranrans latest minimap changes.

Well we'll have to wait another day.


Couldn't hesitate to fix this now.
Someone was a little slovenly and used sint16 directly instead of scr_coord_val in functions signature, so it resulted in a compile error.
Was a quick fix, now it's working again.

Ready for incorporation from my side.
If you want to handle the user preferences properly, feel free to adjust scr_size::rdwr and scr_koord:rdwr
https://github.com/jamespetts/simutrans-extended/pull/175

« Last Edit: May 21, 2020, 12:31:52 AM by Freahk »

Offline Ranran

  • Devotee
  • *
  • Posts: 997
  • Languages: ja
Re: [BUG] scr_size overflow
« Reply #10 on: May 21, 2020, 12:28:28 AM »
it seems to be conflicting with ranrans latest minimap changes
Has minimap been updated since the first post?
Does it mean contrasting with standards?

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #11 on: May 21, 2020, 12:34:05 AM »
Has minimap been updated since the first post?
I don't think so. I just started that work on the wrong, rather outdated branch, so when work was done i noticed that, and rebased the changes on latest master, which resulted in a compile error.
It was only a tiny conflict, it's working now.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #12 on: May 21, 2020, 01:18:12 AM »
Now incorporated - thank you for that.

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #13 on: May 21, 2020, 08:04:11 AM »
Sorry, I guess I broke the headless "COLOUR_DEPTH 0" build.
I had properly modified simsys16, but not simsys0.

Fixed in latest commit.
https://github.com/irgend42/simutrans-extended/commit/12660ceb78ec95cbd5fcb98e7af9e937c8c25dfd

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #14 on: May 21, 2020, 04:51:45 PM »
Thank you for that: now incorporated.

Offline Vladki

  • Devotee
  • *
  • Posts: 3334
    • My addons, mostly roadsigns, pak128.cs
  • Languages: EN, CS
Re: [BUG] scr_size overflow
« Reply #15 on: May 21, 2020, 05:30:33 PM »
Sorry, I guess I broke the headless "COLOUR_DEPTH 0" build.
Could this be the reason why the nightly server build is one day older than nightly client?

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 19823
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG] scr_size overflow
« Reply #16 on: May 21, 2020, 06:51:22 PM »
Could this be the reason why the nightly server build is one day older than nightly client?

Yes, indeed.

Offline Freahk

  • Devotee
  • *
  • Posts: 1069
  • Languages: DE, EN
Re: [BUG] scr_size overflow
« Reply #17 on: May 22, 2020, 11:25:46 AM »
I am aware that in it's current state ghost towns can appear on the minimap in high zoom levels, that means you will see towns name, cityborders, stops and lines of towns that are actually located elsewhere.
Towns will however never disappear.
I don't yet know why this happens, but I will find out and fix this.
« Last Edit: May 22, 2020, 11:37:22 AM by Freahk »