Topic: [BUG] scr_size overflow

[Ranran]

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.

[Freahk]

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?

[Ranran]

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.

[jamespetts]

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?

[Freahk]

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.

[jamespetts]

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.

[Freahk]

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

[Freahk]

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.

[freddyhayward]

Oh dear, that makes at least three patches requiring a new savegame version. James - maybe they could all be merged at once?

[Freahk]

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

[Ranran]

it seems to be conflicting with ranrans latest minimap changes

Has minimap been updated since the first post?
Does it mean contrasting with standards?

[Freahk]

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.

[jamespetts]

Now incorporated - thank you for that.

[Freahk]

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

[jamespetts]

Thank you for that: now incorporated.

[Vladki]

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?

[jamespetts]

Could this be the reason why the nightly server build is one day older than nightly client?

Yes, indeed.

[Freahk]

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.