search

beyond-all-reason / spring

A powerful free cross-platform RTS game engine
https://beyond-all-reason.github.io/spring/
Other
178 stars 95 forks source link

Look up water level instead of hard coding 0 #1484

Open Jordan-Cottle opened 2 months ago

Jordan-Cottle commented 2 months ago

Summary

The goal of this PR is to contribute somewhat to #1445. I only spent a couple hours on this (and a good bit of that just getting a dev environment set up to compile and test the changes).

I'm not familiar enough with the project to be anywhere near confident I got all the places I needed, so my goal isn't to be 100% comprehensive here. However, if there's any spots reviews know of that this change is needed that I missed I'd be more than happy to add those in here too.

Test Evidence

None yet, but it does compile 😅

If anyone has any ideas on good ways to test these things, I'm all ears. I was thinking of setting the constant to something else that would create a noticeable change in the game. It might be hard to tell if a broken thing is broken from something I added but shouldn't have, or a place I missed.

marcushutchings commented 2 months ago

As for the testing question. Having the water set to a very noticeably different value, like -200 and try out all unit types and try to place buuildings over the areas that should now be dry. Should also check that the AI like SimpleAI, and BARB are functioning correcting in those areas.

sprunk commented 2 months ago

I fully expect any testing with a modified waterlevel to reveal that most mechanics don't obey the new waterlevel consistently, and I think that is fine. After all this change replaces like 10 lines, surely there's more areas that deal with water. We can start testing different waterlevels when the replacement closer to completion. As long as there are no visible changes when waterlevel is kept at 0 it's fine imo.

Jordan-Cottle commented 2 months ago

That was my thought as well for testing. I'm definitely not confident that this PR handles all cases, but doing a full test like that would potentially point out new cases that need to be addressed. Seeing the missed case in game would certainly help me track down where to look for things in the codebase.