Skip to content
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

Add sfall CitiesLimitFix to fix Resurrection startup #361

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

roginvs
Copy link
Contributor

@roginvs roginvs commented Apr 18, 2024

This PR adds cities limit patch from sfall. It allows Resurrection to start instead of crashing on startup.

According to sfall code here https://github.com/sfall-team/sfall/blob/a19f9d0b83e5ef8456d351728c08b08e98076534/sfall/Modules/Worldmap.cpp#L389 this patch is enabled by default, and it was so from the first git commit (actually second https://github.com/sfall-team/sfall/blob/b8813ee6c74830ccd7a31ddcc1cbe00d319ae059/sfall/main.cpp#L1134) when code was copied from sourceforge.

This PR enables cities limit patch (similar to sfall) but only if ddraw.ini was read. This way non-sfall instances will work as they worked before, and sfall instances will work as sfall do

@roginvs roginvs changed the title Add sFall CitiesLimitFix to fix Resurrection startup Add sfall CitiesLimitFix to fix Resurrection startup Apr 18, 2024
@phobos2077
Copy link
Contributor

Checking for the existence of ini file to decide on default value is a bit unorthodox, don't you think?

@roginvs
Copy link
Contributor Author

roginvs commented Apr 19, 2024

Checking for the existence of ini file to decide on default value is a bit unorthodox, don't you think?

I agree that it looks slightly weird, but what other options do we have?

  • If we make it default=true then it will be inconsistent with vanilla game
  • If we make it default=false then it will be inconsistent with sfall games and they will not work until this option is added into config. Furthermore, there is no even such option in sfall because this part of code is commented in sfall code.

I might agree that using default=true might be a good option, especially keeping in mind that it will not break anything. On the other hand it is always good to have health-check for worldmap.txt parsing - if cities amount do not match then definitely something went wrong and it is better to crash.

This way using existence of ini file is kind of a tradeoff: it is consistent with original game behavior and also consistent with sfall game behavior where is enough to have ddraw.dll to enable this patch.

@phobos2077
Copy link
Contributor

phobos2077 commented Apr 19, 2024

CE has already deviated from vanilla game, it's a Community Edition after all. In this case I think the default should be the same as in sfall, since it can't break anything. The "health check" you mention doesn't seem so useful to me because the number of cities differ from game to game. This engine is not just for vanilla Fallout 2.

I don't like this tradeoff because it's unusual behavior - people WILL trip over this "gotcha" feature.

@roginvs
Copy link
Contributor Author

roginvs commented Apr 20, 2024

@phobos2077 How about now?

@phobos2077
Copy link
Contributor

Looks nice.

@JanSimek
Copy link
Contributor

JanSimek commented Jun 4, 2024

I would argue that this config option is not useful. No one is going to manually turn it off.

src/worldmap.cc Outdated
debugPrint("WorldMap Error: Cities limit fix is enabled, "
"but the number of cities in the save file is different from "
"the number of cities in the worldmap.txt file.");
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the game has more cities than save file. For example, the mod was updated but it just added new cities. Is it possible to change this condition to numCities > wmMaxAreaNum to support this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I removed this part

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen now if numCities > wmMaxAreaNum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested, but I have a gut feeling that it will be fine. SFall have this patch enabled by default and nothing happened.
Let's maybe keep a warning there

@phobos2077
Copy link
Contributor

I would argue that this config option is not useful. No one is going to manually turn it off.

I tend to agree. Maybe we need to search for any possible issue with high city limit and just make it always on. I see there's already a hard coded limit of 5000 in code, maybe that's enough?

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.

None yet

3 participants