-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix units not entering cities upon capture #4839
Fix units not entering cities upon capture #4839
Conversation
Moved the exception that players (but not AI) can always pass through city-states from TileInfo.canCivPassThrough to CivilizationInfo.canPassThroughTiles (which is called by the former). This makes the behavior of the latter function accurate to its name, and makes the code more understandable overall. A side-effect of this change is that functions that used the lower-level CivilizationInfo.canPassThroughTiles function directly now consider the aforementioned exception. For example, if a city-state expands its border, it should now no longer push out units of a player civilization. It may make even more sense to move the exception to the "canMoveTo" logic, since AI should probably be able to pass through city-state tiles even when they don't want to end on them. That might however affect the AI more significantly.
Before, capturing a city with a civilian unit in it would cause the capturing military unit to enter the city but also cause the civilian unit to leave the city. Now, the civilian unit stays in the city as well.
I want through the code and it looks good, but since this deals with the fundamentals of unit movement, I want us to check this seriously. |
Auto-running the game with a large Exception stack trace
Apparently, the auto-simulation sometimes results in the Instead of auto-simulating, I manually clicked "Next turn" up to turn 400 for two games with three AI players and a spectator, and no errors occured. I did not notice any movement irregularities either, so I believe the changes of this PR are safe. |
Nice! |
This fixes the bug where units would not enter a city upon capturing it, as listed in #4697.
Only the changes in
Battle.attack
andUnitMovementAlgorithms.canMoveTo
are necessary to fix the bug. The other changes are for different reasons.I renamed
CivilizationInfo.canEnterTiles
tocanPassThroughTiles
andTileInfo.canCivEnter
tocanCivPassThrough
to more accurately describe what they do. Almost everywhere in the code and the comments, "entering a tile" is used to refer to ending movement on that tile instead of merely passing through it. The renamed functions test whether certain tiles can be passed through, not whether they can be "entered" in this sense, hence the renaming. I also added some documenting comments to both of these functions and toUnitMovementAlgorithms.canPassThrough
to make it more clear what exactly they check.Furthermore, the function that is now called
TileInfo.canCivPassThrough
used to contain an exception to always allow player civilization (but not AI) units to pass through city state tiles. I moved that exception to the slightly lower-levelCivilizationInfo.canPassThroughTiles
, which is called byTileInfo.canCivPassThrough
. This makes the behavior ofCivilizationInfo.canPassThroughTiles
more accurate: player units can indeed pass through tiles of city-states.A side-effect of this change is that functions that used the lower-level
CivilizationInfo.canPassThroughTiles
function directly now consider this exception, while they previously didn't. For example, if a city-state expands its border, it should now no longer push out units of a player civilization. I do not know whether these side-effect changes are accurate the original Civ V, but the behavior ofCivilizationInfo.canPassThroughTiles
now certainly makes more sense. If we need a function that checks if a tile can be passed through without considering the city-state exception, I think that a separate function with a distinctive name should be added.It may make even more sense to move the exception to the "can enter" logic, since AI units should probably be able to pass through city-state tiles even when they don't want to end on them. That might however affect the AI more significantly. I went for the safer change.