Code review comment for lp:~widelands-dev/widelands/terrain_affinity_as_int

Revision history for this message
Arty (artydent) wrote :

I reviewed the code and also tested somewhat. Still has a few issues, most of them minor though. See diff comments.

The file data\world\immovables\bush1\init.lua still has a few comments (lines 86-94) referring to the floating point values, this should also be changed in this branch.

Overall it should be much less likely now to get different results based on platform/compiler dependent precision choices, but they might still happen. There are still things based on floating point calculations (like the main probability calculation in calculate_probability_to_grow) but they might be difficult to replace with pure integer based stuff.
That said, considering that the ratios of the final probabilities of the six best immovables already change significantly with this branch, we could possibly also change the basic probability calculation to somewhat that uses only integer operations but is not too far off from the current approach (which seems to have been based roughly on multivariate normal distributions).

Btw, where did those three weight constants in calculate_probability_to_grow originally come from? Their precision is too high to be from a simple "try and error until it feels right" approach. They still feel quite arbitrary though.

review: Needs Fixing

« Back to merge proposal