Credits harvested capped at 65000 #177

Closed
rofl0r opened this Issue Jan 18, 2013 · 5 comments

Projects

None yet

4 participants

@rofl0r
Contributor
rofl0r commented Jan 18, 2013

Original title: artifical limit of 65000 obfuscated how much spice was really collected
Original body:

The amount of credits harvested, as shown in the End Game screen, is capped at 65000, which is easily reached in the last few campaigns.

@rofl0r rofl0r added a commit to rofl0r/OpenDUNE that referenced this issue Jan 18, 2013
@rofl0r rofl0r -Fix (#177): remove artificial limit of 65000 for harvested spice
closes #177
0e8b220
@TrueBrain
Member

The limit is not "artificial", in the sense it is limited to an uint16. They reduced it by a tiny bit, most likely for aesthetic reasons.

Your patch is flawed (ignoring the fact it changes a lot of unrelated lines, and that using "uint" is a no-no in OpenDUNE coding style), as it will not survive a save/load. It will require adding a new chunk to resolve that issue.

@rofl0r
Contributor
rofl0r commented Jan 19, 2013

using uint is the only sane thing for stack variables, the 16bit age is long gone and using uint16 instead of uint only leads to more and slower generated code, integer overflow and truncation etc.

compile this test code here http://sprunge.us/XfJE with -S or -c and typedef short mint vs typedef int mint to see that the resulting 16 bit code is about 30 % bigger (and much slower, since cpu's are optimized for the common 32bit operations). on RISC cpus dealing with 16bit has even higher cost.

where is the g_scenario.harvested data stored to file ?

@Ericson2314

TrueBrain is right. Certainly you don't want your save file containing machine-dependant integer sizes. I see no reason not to bump it up to uint32 however.

@TrueBrain
Member

Sadly, bumping it to uint32 is not as trivial as it sounds. It requires a new savegame-chunk to store it correctly, and all the code that comes with that.

For me this enhancement is at the bottom of my priority list, as personally I don't mind the 65k limit; personally I think it fits the game as a whole, limited and everything.

That said, I have nothing against such patch, but someone will have to write one that complies with OpenDUNE coding style, and is correct (in terms of also saving the information correctly). So if you feel up for that task, I eagerly await the PR for it :)

@miniupnp miniupnp added the wontfix label Jan 19, 2017
@miniupnp
Contributor

I don't think it is worth fixing this.

@miniupnp miniupnp closed this Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment