-
Notifications
You must be signed in to change notification settings - Fork 29
800 - Add world sizes #850
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
Open
zack-the-coding-actuary
wants to merge
7
commits into
C7-Game:Development
Choose a base branch
from
zack-the-coding-actuary:Add-World-Sizes
base: Development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6a61656
World Size Options for New Games Added
zack-the-coding-actuary e2b0d63
Fixed feedback from maintainer and ran whitespace formatting
zack-the-coding-actuary 0ecd6cb
Merge branch 'C7-Game:Development' into Add-World-Sizes
zack-the-coding-actuary c51cff5
Merge branch 'C7-Game:Development' into Add-World-Sizes
zack-the-coding-actuary 729eedc
Merge branch 'C7-Game:Development' into Add-World-Sizes
zack-the-coding-actuary 938ddc1
Merge branch 'C7-Game:Development' into Add-World-Sizes
zack-the-coding-actuary bcf1bb9
Merge branch 'C7-Game:Development' into Add-World-Sizes
zack-the-coding-actuary File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these values for the WorldSize variants coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the Civ 3 wiki + hardcoded game values: https://civilization.fandom.com/wiki/Map_(Civ3)
Albeit, they are indeed coded as "magic numbers" right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if @TomWerner had something specific in mind when he opened the issue, besides making easier for modders to play around with the sizes, etc, and that's why he said that this should be saved in the json save file.
The other thing would be dynamic names for the buttons (Tiny, Small, etc) for all the World Sizes, vanilla & custom.
I would prefer if we wrote that to the json as well, but the current code doesn't break anything I tested so far with .biq, .sav and .json files.
If this were to be accepted, a TODO would have to go in there for a future refactor so that it can go in the json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can point me to the right JSON file to save the configs in (or the directory where I should make a new config file) I'll happily refactor it. I agree that direction is better modular design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The json file is actually two files c7-static-map-save.json and c7-static-map-save-standalone.json
These files are used to launch a new game (for now, today Yegor opened a PR that merges them into one actually, but still valid), the first for the original version and the other one for the standalone with seperate graphics. I can't explain the whole thing here, but I would suggest you take some time, and try to understand how the creating/loading works.
There is a class called CreateGame.cs that you can have a look at and start to unravel the whole thing. Most of the code's variables/methods/classes are well named, so if you can't find something through code, you can search the text for what would be the most likely name, and chances are you are going to find it.
I would also suggest to use a debugger when you want to examine things in detail, and also have a look at the existing unit tests and even write your own if you want.
If you have any questions, discord is better for this kind of thing I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't want to be hardcoding these.
This information should be present in the WSIZ or WCHR (world size/world characteristic) fields. We grab some data from them in ImportCiv3 right now:
Prototype/C7Engine/C7GameData/ImportCiv3.cs
Lines 1720 to 1736 in 9ecbc5a
If we pull from there (and save it in the JSON files) then it's available to modders, existing civ3 mods that change the world sizes and such will work correctly, and it'll generally work nicely.
In case you're a printf debugging guy, you can use
dotnet test ../EngineTests/EngineTests.csproj -v nto run the tests and see print statements you add, otherwise a debugger works great too. The WSIZ/WCHR classes should map to the civ3 editor pretty closely if you want to inspect the values from vanilla civ3.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other printf debugging note, there's this nice DumpObject function in
ImportCiv3.csthat I've used with some of the civ3 structs before.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
In Rider, when running in Debug, you can right click on a variable and select Evaluate Expression, where you can run something like
System.Text.Json.JsonSerializer.Serialize(biq.Prto[4])and it will turn the object into a json, without having to modify the code, or if you have forgotten something, you can always run this as long as the var is in the current scope.
This probably doesn't cover private fields like DumpObject, but still quite helpful for larger objects