-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase maximum migration distance #2447
Comments
I don't think there's gonna be any error, but the performance impact should be kept in mind: a large migration distance would mean walking through the whole map for every city (effectively quadratic complexity in the map size). |
That's true. However, we don't know exactly what the performance impact would be without testing it. It's also something that could be balanced in other ways, higher citymindist, smaller map etc. I think people should have the freedom to do stupid things if they want to, and our role should be to provide appropriate warnings so they know it's their own fault.
This is a value that gets squared, so at least make sure it doesn't cause an overflow. |
I don't see it getting squared... The loop goes outward and stops once the distance becomes is too large, so it's capped by the map size. |
You're right, I was thinking about the city map max radius. That algorithm looks pretty inefficient in general. |
If we did need to improve the efficiency, this seems like low-hanging frut: Line 4217 in a9b77fd
There are only a handful of possible values for the squared city radius, so we could get those from a lookup table rather than calculating a costly square root. This is probably premature optimisation though. |
Is your feature request related to a problem? Please describe.
Currently, the maximum migration distance for a server is arbitrarily limited by the maximum city working radius. This is currently blocking: longturn/rules-of-chaos#17
Describe the solution you'd like
Increase the maximum migration distance to the highest value it can be set to without causing errors.
Describe alternatives you've considered
https://youtu.be/ubPWaDWcOLU?feature=shared
Additional context
freeciv21/common/game.h
Line 492 in a9b77fd
The text was updated successfully, but these errors were encountered: