Skip to content

Conversation

@esbudylin
Copy link
Contributor

Previously, the code assumed that every terraform had a terrain improvement associated with it. As a result, it caused NullReferenceException when executing terraforms for chopping forest and clearing wetlands.

This commit fixes the logic for replacement checking and separates it into a helper method in the TileOverlays class.

Previously, the code assumed that every terraform had a terrain
improvement associated with it. As a result, it caused
NullReferenceException when executing terraforms for chopping forest
and clearing wetlands.

This commit fixes the logic for replacement checking and separates it
into a helper method in the TileOverlays class.
@stavrosfa
Copy link
Contributor

I thought I had fixed that with #841. Does it still throw a NPE? Can you give me some steps to reproduce this on the current Development branch? I tried and I couldn't trigger it.

@esbudylin
Copy link
Contributor Author

Yes, my bad, I missed your PR and tested this on an outdated version of Development branch (I tripped over this bug while playing the release build). I tested it again, and it doesn't reproduce now. I still think we should merge this PR, since it gives slight readability improvement, but it's not a big deal.

@stavrosfa
Copy link
Contributor

I agree it reads better, I just wanted to make sure there wasn't an issue with the previous PR.

My only comment would be to rename the method from GetReplacementTarget to something like GetTerrainImprovement, and also maybe rename ImprovementAtLayer to the same name. The method I had had the same name, but that maybe is not that accurate and GetReplacementTarget seems to be very specific. It would be easier to find and use the API in the future.

I don't know what you think, but otherwise it looks fine.

@esbudylin
Copy link
Contributor Author

My only comment would be to rename the method from GetReplacementTarget to something like GetTerrainImprovement, and also maybe rename ImprovementAtLayer to the same name

I don't think GetTerrainImprovement is a better name for this method. The method specifically checks for an improvement that will be replaced by a terraform, not just retrieves an improvement. I also don't think that it should be an overload of ImprovementAtLayer, since these two methods do two essentially different things.

@esbudylin esbudylin changed the title Fix terrain improvement replacement logic Refactor: terrain improvement replacement logic Dec 14, 2025
@esbudylin esbudylin merged commit 54ed904 into Development Dec 14, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants