Crashes When Updating Game -- Progress Bar

[ChromePoptart]

Description

I've encountered two fatal errors that crash the launcher when it's in the middle of the game updating process. These errors seemed to occur around the time the "saves_backup" directory was being analyzed or copied/moved.

CDDA Game Launcher version: 1.6.8
OS: Windows-10-10.0.19044-SP0 (64-bit)
Type: <class 'OverflowError'>
Value: argument 2 overflowed: value must be in the range -2147483648 to 2147483647
Traceback:
File "C:\Program Files -- Games\CDDA Game Launcher [DazedNConfused]\cddagl\ui\views\main.py", line 3716, in step

progress_bar.setRange(0, self.total_copy_size)

CDDA Game Launcher version: 1.6.8
OS: Windows-10-10.0.19044-SP0 (64-bit)
Type: <class 'OverflowError'>
Value: argument 1 overflowed: value must be in the range -2147483648 to 2147483647
Traceback:
File "C:\Program Files -- Games\CDDA Game Launcher [DazedNConfused]\cddagl\ui\views\main.py", line 3772, in step

self.progress_bar.setValue(self.copied_size)

I will note that I initially encountered the first crash when using v1.5.6 of the original remyroy launcher. I looked into other launchers, installed this variant/fork in order to minimize necessary changes on my end, and then encountered the first crash again using this launcher. So, the first crash at least doesn't seem to be related to any changes since that fork.

To Reproduce

For me, the errors seemed to occur whenever I tried to update the game, and seemed to pop up around the time the "saves_backup" directory was being analyzed or copied/moved.

Expected behaviour

I'd expect the launcher to be able to successfully complete the game updating process.

Screenshots

No response

Attempted Fixes

I was able to prevent the errors/crashes from occurring by editing the "cddagl\ui\views\main.py" file noted in the error messages. Assuming that the progress bar is just a user convenience and not necessary for the updating process itself, I deleted the indicated lines 3716 and 3772. Upon trying to update the game again, the process completed successfully (albeit with the progress bar not being helpful whatsoever).

In initial troubleshooting, I tried selecting different builds to update to, in case the errors lie with something related to the builds themselves. After making the above fix, I was able to see via the status bar that the crashes were occurring around the time the "saves_backup" directory was being analyzed or copied/moved. This leads me to think that the errors might stem from the fact that I have what is (probably) a large "saves_backup" directory. It's currently sitting at about 2.2GB, meaning that storing that size of the directory in bytes would cause a signed integer to overflow (which tracks with the error messages shown). I haven't done Python development, but perhaps a fix would be to switch to using 64-bit integers instead of 32-bit integers.

System Details

CDDA Game Launcher v1.6.8

CDDA itself:
- OS: Windows
    - OS Version: 10.0.19044.2251 (21H2)
- Game Version: c804856 [64-bit]
- Graphics Version: Tiles
- Game Language: System language []
- Mods loaded: [
    Dark Days Ahead [dda],
    Disable NPC Needs [no_npc_food],
    No Fungal Growth [no_fungal_growth],
    Bionic Professions [package_bionic_professions],
    Blaze Industries [blazeindustries],
    Bionic Slots [cbm_slots]
]

Additional context

No response

[DazedNConfused-]

Coming from a Java background, data type length would have been a prime suspect for me as well. However, I am under the impression that, barring some very specific NumPy types, integers in Python 3 have unlimited size.

This is gonna need some digging into. Extra comments from people experiencing the same problem are welcome.

[langp11]

I've done some more research on this and it seems clear (to me...) that the underlying cause of the problem is not python but the Qt widget library.

Checking the Qt docs (https://doc.qt.io/qt-5/qprogressbar.html#setRange) the progress bar only accepts int parameters. Since this is a C++ library this will be a hard limit.

Possible solutions:

1. Comment out the offending lines as per @ChromePoptart's suggestion.
2. Add some cunning scaling process that adjusts the parameters in python to avoid the overflow.
3. Change those lines to limit the maximum value passed in (see below).

Option 1 makes the progress bar 'unhelpful'. Option 2 is theoretically best, but computationally intensive for a fairly minor (imho) feature. Option 3 only requires changing the two lines in question to include a maximum: like this

3716:                           progress_bar.setRange(0, min(self.total_copy_size, 2000000000)) 
3772:                   self.progress_bar.setValue(min(self.copied_size, 2000000000)) 

I used 2000000000 rather than the 2^32 -1 (a) because I'm lazy, and (b) if the intended audience is normal humans, it seems easier to understand a decimal limit. But whatever... Note that the text progress indicator continues to update even once the bar has maxed out.

I have tried the update with this change and a large save directory (5GB) and it works. I had previously had the same overflow problem. I can make this a real PR if you like.

[ChromePoptart]

Option 3 seems like a reasonable solution, assuming that having such large of save directories is fairly rare (not sure whether this is true or not). A further suggestion could be to just hide the progress bar, disable it, or do something else if the arguments to it would cause an overflow -- as @langp11 pointed out, the text progress indicator would still show actual progress, and the progress bar wouldn't be displaying false information.

Regarding Option 2, it seems to me like it would be fairly simple to do something along the lines of (excuse my C++) if (size > 2^32 - 1) { size = size / scaling_factor; } It seems to me that it wouldn't be necessary to care that much about rounding errors or anything of the sort (does it matter if the progress bar is off by 1%?), and, regarding computational intensity, if the concern is about the division operation itself happening with each recalculation, I would assume that the scaling factor could be set to a power of 2 (or bitwise-right-shifting could be used explicitly) to reduce the overhead due to division. Just my two cents -- I of course could be missing something!

[langp11]

Option 2: Yes, it's the division that was bothering me, but my head is stuck in floating point-world. I have tried a fix with the scaling factor and right-shifting and it works just fine. I can't notice any performance impact, even though I'm doing the shift all the time (hoping that "current_size >> scaling_factor" is cheaper than "if scaling_factor != 0").

[DazedNConfused-]

PR has been merged and is available in latest release https://github.com/DazedNConfused-/CDDA-Game-Launcher/releases/tag/v1.6.9

Thanks @langp11!

[ChromePoptart]

@DazedNConfused- Should I go ahead and close this issue (referencing #79)?

[DazedNConfused-]

We can leave it around some more, see if anyone has any feedback. Afterwards yup, I will be closing the issue~