[Dev] 0.1 "Babylon" Progress Thread

[Quintillus]

I've been going off in random directions adding things today. One small things is the "Z" key setting zoom to 100%, or 50% if it's already at 100%, which was bantered around a few pages ago. A small quality-of-life improvement.

More interesting has been the barbarian work. I made it so that there is a 7% chance of each barbarian camp spawning a new barbarian each turn (7% being chosen because I couldn't decide between 5% or 10% as the best option; I envision this changing with the barbarian aggression setting). I then also made a basic AI, which is now refactored to follow the basic structure Puppeteer outlined in the SimpleGame repo, that moves those barbarians around randomly, while ensuring each camp is always guarded by at least one barbarian. There isn't any combat yet, so they don't really pose any threat. But it got me thinking about a lot of possibilities for how the barbarian AI in particular could behave. I wrote them up in this commit's message.

The tl;dr is that it consists of potentially each barbarian camp choosing from one of several barbarian AIs, thus presenting a varied threat (pillaging/looting/preventing expansion/etc.). I also thought some about the possibilities of barbarians evolving (in the not-sticking-purely-to-Civ3-mechanics mode), and the pace of that potentially being tied to their success in pillaging, looting, and defeating player units.

 

[Puppeteer]

Huh. Started thinking more about map visibility. It was and is trivial to read the visibility flag from the save file, and it's trivial to just not draw non-visible tiles, but it's stored in Civ3 as a 32-bit flag which we're tossing out for C7 to allow more civs. Not a huge blocker, but there are quite a few civ bitflags that will need to be represented differently in C7.

An array or collection of bools feels inefficient; a 64-bit int only doubles the civ limit. Actually I guess a hashtable of bools will be efficient enough, have no limit to speak of, and are easy to read in code.

As for the clean filter, I got a test repo working as I hoped, but it's not cross-platform, and git-scm on Windows has the necessary tool (sed) included, but it doesn't magically make them available to filters. I may try having two clean filters, one for Windows and one for unix-like.

Edit: Oh, it *does* work to have the git-scm-included run filters on Windows! It's just that the config to add the filter needs to be manually added. I think this will work as we can target the "don't change" bits with specific filters and only have to add the filter once per cloned repo.

 

[Puppeteer]

Let's refactor all path finder calls to pass Global.Civ3Home - It's some parameter repetition, but putting pass-through convenience functions in GlobalSingleton is just too icky and opens the door for multiple code paths to exist to get a file path which is bound to cause issues

ARGH! Civ3MediaPath alone has 22 references across 11 files. Which means I'd have to add GlobalSingleton reference loading to all those files as well as the home reference to all the references. It's doable but feels utterly wrong for now and for going forward.

Well, I'm going to set that aside for the moment. Maybe something else will occur to me or to one of you.

The problem:

• Need a global-ish store for civ3 home for reading media
• GlobalSingleton isn't in a global scope in itself, but it can always be loaded by a Node in the tree
• The utility function is not in the tree

I could move the utility to GlobalSingleton, but that doesn't really improve things much as we'd still have to load a reference to GlobalSingleton in every file. It also puts the function out of reach of non-Godot code; that shouldn't need to be done, but I kind of hate to just take that possibility away completely.

While you can use reflection to get info on the calling method, I don't see that it's possible to call a method on the calling object instance, else I'd just reach back to the caller and do a GetNode<Node> from the utility function.

Meanwhile I think I'll give up on it for a bit and look at something else. Not that I've been working on this for days; I just have been doing non-C7 stuff.

Edit: On second thought, I'll just chain the load as a parameter. It's still ugly, but it's not spread across each file. GetNode<GlobalSingleton>("/root/GlobalSingleton").Civ3Home as a parameter. Pretty. On third though, no, I don't want to query the node tree every time we need a texture or audio.

 

[Quintillus]

I count 23 references across 11 files. The 23rd might be one I've added in a branch over the past couple days.

I guess I don't see the problem with the current Civ3MediaPath implementation. From an API standpoint, it's super convenient right now, which is probably why it's used in 22 or 23 places. It takes one parameter, which is exactly what I'd expect to need to pass, and goes and figure it out, with the help of the GetCiv3Path function, and returns the path we really need to open the file.

You mentioned looking at refactoring this, "to lower attack vectors as I want to expose those classes to user scripts." What exactly is the attack vector/what is the need for this functionality to be exposed to user scripts? This is an area where I'm not super well versed in what Lua scripts can and can't do. Can they dig around and modify the file system? If so, how does Civ3MediaPath make things worse? If a script can write to the file system, I suppose a malicious script could mess around with Civ3 install files, and this might make it easier to figure out where Civ3 is located, but couldn't it do a lot worse than that if it wanted to? Like placing links to malicious websites on the user's desktop?

 

[Puppeteer]

I guess I don't see the problem with the current Civ3MediaPath implementation.

It only works if Civ3 home is detected from registry or env var. If we want the user to be able to browse for it or provide it in an in-game prompt, we need to store and access that user input. Or else prompt them every time we want to load a media file.

I was trying to add a directory selector dialog. Well, I did, but plumbing it in is flummoxing me.

You mentioned looking at refactoring this, "to lower attack vectors as I want to expose those classes to user scripts." What exactly is the attack vector/what is the need for this functionality to be exposed to user scripts? This is an area where I'm not super well versed in what Lua scripts can and can't do. Can they dig around and modify the file system?

My paranoid production support background. There is no obvious attack vector, but any class object we might expose to Lua scripts I'd like to sanitize as much as possible. In this particular case I'm removing all methods that may call the filesystem in case we accidentally add a vulnerability later.

I can keep Lua out of the filesystem directly, but if I expose a class to it that can access the filesystem, that's where my security paranoia kicks in.

Maybe we won't need in the end to expose QueryCiv3 to Lua, but I'm doing so in the short term for research, and I want to in the longer term for my other projects, although I can always re-fork that if C7's needs and my needs diverge. At one point I had thought the import from Civ3 might actually be in Lua as MoonSharp includes a JSON converter module, but C# does JSON really easily, too, and as long as C7's save format is serializing GameData it makes all the sense in the world to go straight from Civ3 to GameData and then serialize to save.

There are issues with serializing GameData, and you're running into them already. It doesn't handle any discrepancies well as we add features or refactor. But I can't think of any better near-term better alternatives. Well, I mean if you want to just not use the json save for the "new game" it's fine with me if we just auto-import a Civ3 save instead. But we're close to being able to have a meaningful game state which has the code to be saved and loaded, even if it is tied really really strongly to our class definitions which are changing quickly.

 

[Quintillus]

For saving the directory selection, would there be an issue with saving it in a static variable in Util.cs, which GetCiv3Path could then access? Or a static variable in a Settings/Preferences class? We're going to need a Settings/Preferences concept regardless.

Yeah, I suppose from the standpoint of protecting ourselves from accidental vulnerabilities, it comes down to (in this case) whether QueryCiv3 (and other areas that may access the file system) should be exposed to Lua. I'm trying to think of what we might want to expose them to Lua for. Currently, the only thing coming to mind is allowing scripts to examine Civ3 resources (say, diplo texts or "scripts" that ship with Civ3), and make decisions based off of the contents of those. But wouldn't it make more sense to give the scripts access to the classes we will eventually have that have already processed the contents of those files, and have them nicely in C#, MoonSharp-compatible classes? I can see how in the short term it may be handy to have direct access to prove that concepts work, since it would be additional work to process the files up front.

I was thinking the other day about how it could be tough to maintain compatibility with the JSON as we add new things, which is happening quickly these days. I haven't come up with any better alternatives, either. At this point I am thinking that for the early days of being able to load from a C7 JSON file, we should simply say that if you want to load a save, user the same version you used to save it. Trying to maintain compatibility across versions at this early stage would be too much overhead given the rate of changes. Longer-term, we can put some effort into stability between milestone releases. That would be more or less what Paradox does; sometimes you can load across milestone versions, but they generally recommend sticking to the same milestone to play a save, and that is often required; bugfix releases for a milestone do maintain compatibility.

 

[Puppeteer]

For saving the directory selection, would there be an issue with saving it in a static variable in Util.cs, which GetCiv3Path could then access? Or a static variable in a Settings/Preferences class? We're going to need a Settings/Preferences concept regardless.

To my understanding, there really isn't such a thing as a variable without an instance. So if the utility functions are part of an instance and not static, then we have to find a way to get a reference to that from 11 or so files.

I was going to say that other settings & preferences will be of interest to the game engine, but no...there will be quite a few UI-only preferences, and we do have to figure that out.

For Lua, yeah, the access to the Civ3-native data wouldn't be part of the game play. I had vaguely imagined a modder being able to pull custom info out of the save, but I guess that doesn't really make sense. I guess at this point it's mainly because *I* want to access QueryCiv3 with Lua at dev time or for other projects. It will probably never make sense to use Lua as an intermediary from Civ3 to JSON for C7. My personal needs don't override C7's needs for the repo.

 

[Quintillus]

To my understanding, there really isn't such a thing as a variable without an instance. So if the utility functions are part of an instance and not static, then we have to find a way to get a reference to that from 11 or so files.

We might be using different terminology. As I understand it, there are variables that are not tied to instances. Java and C# use the term "static" to refer to such variables and methods, and I believe C++ does as well. Checking some terminology reference, it looks like "class variable" is a more general term for the concept. Tied to a class, rather than an instance of a class.

We already have some of these, such as MapUnit.NONE:

public static MapUnit NONE = new MapUnit();

It looks like all of our utility functions are static, which is what I would expect in a utility class.

Depending on the structure of eventual multiplayer code, some settings may need to be on instances, in case an engine is handling multiple player's preferences for things such as "Always build previously built unit" that may be of interest to the engine. But for UI preferences, I'm feeling reasonably confident that there only needs to be one class-level variable for them.

 

[Puppeteer]

Ok, if that works, that would be perfect. I didn't think that was possible. My recollection is that I always got an error trying to do something like that, but I'll give it a shot, and if it's already in our code then it must be possible. I don't understand the scoping of that, but I guess I'll find out.

Wait, maybe I do get the scoping. Because it's all namespaced, if not with an explicit namespace then at least with a class namespace. And I'm far enough out of school that I've never had a formal education on any language newer than C++, and C# is still very new to me. And coding has never been my primary job role.

Edit: Well, I mean this is very much like setting a global scope variable in other languages, but I didn't think C# allowed such.

 

[Quintillus]

I thought we might be coming at it from different language backgrounds. It is somewhat like a global, but not like a global in JavaScript as it is still tied to that class. We can have both MapUnit.NONE and Tile.NONE, and they're separate (although in Java, there is the concept of a static import; if you static imported the MapUnit class, you could refer to MapUnit.NONE with simply NONE - but if you static imported both MapUnit and Tile, you'd have to specify, as the compiler wouldn't know which NONE you meant otherwise. I don't know whether C# also has a static import of a class concept; it's niche in Java and best ignored if it doesn't make sense right away).

C also has a static keyword, but IIRC is works slightly differently than the C#, Java, or C++ static keyword. It's been a couple years since I wrote C, though, so I couldn't say what the differences are off the top of my head. (It's also been even longer since I wrote C++, and what C++ I did write was based on standards from around when Civ3 came out, not today's standards... so it might not be quite the same in C++ as Java/C#, either)

---------------

I've been writing Java today, on my editor. But also baking and visiting people. Probably not going to write any C7 code today.

 

[Puppeteer]

So...there are two pull requests sitting out there, and we don't seem to have a procedure beyond not wanting to just merge our own PRs right away. I looked at each, not sure what to look for. I fetched Quintillus' branch and it works and barbs move around occasionally, but I really don't know what everyone has in mind here.

I didn't quite go through the trouble of cloning Lucien's repo or otherwise testing out the code, but I saw that this is the second PR for BIQ data and Quintillus had some input on the other one, so I just added him as a reviewer which kind of feels like pushing unsolicited work onto someone else.

 

[WildWeazel]

You can check out a PR directly from the target repo

 

[Quintillus]

I was wondering if there was a way to check out a forked repo without cloning it too. Thanks for sharing that!

As soon as we start talking PRs, my mind turns to what teams I've worked with have done. The most common workflow I've seen is:

1. Quintillus opens PR
2. Puppeteer makes suggestions.
3. Quintillus makes improvements
4. Puppeteer says looks good, or makes more suggestions (skip this step if in step 2, Puppeteer said these are all minor, no need for me to re-review changes)
5. Quintillus merges

But I've seen some teams where if someone approves, they merge.

I tend to be slow and methodical when reviewing PRs, but I know other developers who are much faster and more surface-level. I tend to get comments about noticing details and making useful suggestions, but it also takes longer. But my general philosophy has been if you're going to go to the trouble of a code review, it might as well review the code. If the goal is just feature testing, that might be a lighter-level check out the branch and see if it does what it advertises (and doesn't need to be done by a developer).

Which one we want is debatable. Code reviews do the most for quality, but take the longest, and can feel like work if there are a lot of them. Even when they were work, it could be energy-sapping when there would be a few days in a row with a lot of them. Writing code is fundamentally more fun than reviewing it.

WildWeazel wrote in the "Assembling the team" thread:

Let's at least keep doing work in branches, and try to get somebody to look at it before merging to development even if just a sanity check. It'll be more of a concern as the software gets bigger and more complex, but even now I'd like to (a) get in the habit of putting everything through a PR, and (b) have more eyes on the core code while it's still a manageable size.

The "try to get somebody to look at it before merging even if just a sanity check" suggests a higher-level review, maybe just a clone and test that it works. The "more eyes on the code" suggests at least looking through the code though.

My thoughts on trying to strike a balance:

- Minimum goal of someone checks out the code and runs it. To me, this is the "sanity check". It makes sure it compiles and that it is not broken, and is equivalent to what a manual tester would do.
- Mention in the PR if you think some or all of the code could use a detailed review
- For the first couple PRs from a new contributor, someone reviews the code regardless. The ones from Caro-Kann have been really good so far (and also quite large). But it does seem like a good minimum standard.



Maybe that's enough, maybe it's too little, maybe too much. I tend to subscribe to the school of try something and modify it, but want to be cautious not to err on the side of too much procedure too early.

 

[Quintillus]

Working on rivers, I've been adding a few fields to C7 Tile, and have noted there are three types of tiles:

C7 Tiles (Tile.cs in C7GameData)
Civ3 Tiles (TILE in Tile.cs in QueryCiv3/BiqSections)
MapTiles (MapTile in GameSections.cs in QueryCiv3)

In ImportCiv3, it's fairly clear that Civ3 Tiles (TILE) are the source. But the distinction between C7 Tiles and MapTiles has been harder to discern.

Looking at the Git history, it appears that MapTile and C7 Tile were added within a few days of each other in early November, and both have been added to in parallel. Both have a fair amount of overlap in attributes, although MapTile's structure is more BIQ/SAV focused, using byte offsets to get attributes.

As best I can tell, it makes sense to move away from MapTile and toward C7Tile, leaving Civ3 Tile for importing purposes. Still, I wanted to run this by the team in case I'm missing something about why we need MapTile. Both have received updates this month, so it's not 100% clear which should stick around.

(As a side note, the Git history is also cleaner for C7 Tile, since we don't have a bunch of classes in the same .cs file. That's part of why I'd prefer creating new CS files for each class)

Edit: Hmmm, I see now that MapTile is the one that gets populated by PopulateSections in the ImportCiv section; C7 tiles are not populated there. Too tired to figure out what the right thing to do is now.

 

[Caro-Kann]

I was working on refactoring the SAV import code to get rid of things like GameSections.cs, so that's a WIP. But I only got halfway done with getting all the SAV data before vacation season started, so I might wait to do a PR for the branch for a few days. Here's a sneak peak of the new Sav Tile logic: https://github.com/lucienmaloney/Prototype/blob/SavStructs/QueryCiv3/SavSections/Tile.cs

Sadly, I think we're still going to need 3 Tile sections. One for actual game logic (C7 Tile), one for importing from Biq data (Biq.TILE), and this last new one for importing from Sav data (Sav.TILE). The contents of Tile sections in Biq vs Sav files are too different to justify using the same QueryCiv3 classes, which is why I'll be giving them the same name but put them in different sub-namespaces.

 

[Quintillus]

Looking over the documentation and realizing that SAVs embed BIQ rules, but not the BIQ map, I think you are right that we'll need both a BIQ and a SAV import for tiles. Even if SAV did include a BIQ map, we'd need the SAV tile for updates to the tiles, visibility, etc.

Given that, I think I'm going to put the river branch on hold for now, not that it's been moving very quickly the past few days anyway.

 

[Flintlock]

The game stopped working for me a few days ago, I don't know the exact time since I haven't pulled changes in a bit. It crashes when I press new game with this annoyingly uninformative exception:

Unhandled Exception:
System.TypeLoadException: Failure has occurred while loading a type.
at C7Engine.CreateGame.createGame (System.String loadFilePath, System.String defaultBicPath) [0x0001e] in R:\proj\C7\Prototype\C7Engine\EntryPoints\CreateGame.cs:22
at Game._Ready () [0x00017] in R:\proj\C7\Prototype\C7\Game.cs:47

It points to the line calling C7SaveFormat.Load in createGame. I found more info in the log, here's the relevant part before the crash:

Loading reference 1 of R:/proj/C7/Prototype/C7/.mono/temp/bin/Debug/C7GameData.dll asmctx DEFAULT, looking for System.Text.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\mono\gac\System.Text.Json\6.0.0.0__cc7b13ffcd2ddd51\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:\Users\Auxiliary\Desktop\Godot_v3.4-stable_mono_win64\GodotSharp\Mono\lib\mono\4.5\/Facades\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\mono\gac\System.Text.Json\6.0.0.0__cc7b13ffcd2ddd51\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'C:\Users\Auxiliary\Desktop\Godot_v3.4-stable_mono_win64\GodotSharp\Mono\lib\mono\4.5\/Facades\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\mono\gac\System.Text.Json\6.0.0.0__cc7b13ffcd2ddd51\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'R:\proj\C7\Prototype\C7\.mono\temp\bin\Debug\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:\Users\Auxiliary\Desktop\Godot_v3.4-stable_mono_win64\GodotSharp\Mono\lib\mono\4.5\/Facades\System.Text.Json.dll'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\mono\gac\System.Text.Json\6.0.0.0__cc7b13ffcd2ddd51\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'R:\proj\C7\Prototype\C7\.mono\temp\bin\Debug\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'C:/Users/Auxiliary/Desktop/Godot_v3.4-stable_mono_win64/GodotSharp/Mono/lib\System.Text.Json.exe'. (in domain Mono, info)
Assembly Loader probing location: 'C:\Users\Auxiliary\Desktop\Godot_v3.4-stable_mono_win64\GodotSharp\Mono\lib\mono\4.5\/Facades\System.Text.Json.exe'. (in domain Mono, info)
The following assembly referenced from R:/proj/C7/Prototype/C7/.mono/temp/bin/Debug/C7GameData.dll could not be loaded:
Assembly: System.Text.Json (assemblyref_index=1)
Version: 6.0.0.0
Public Key: cc7b13ffcd2ddd51
The assembly was not found in the Global Assembly Cache, a path listed in the MONO_PATH environment variable, or in the location of the executing assembly (R:\proj\C7\Prototype\C7\.mono\temp\bin\Debug\).
(in domain Mono, warning)
Failed to load assembly C7GameData[0000000020e78bd0]. (in domain Mono, info)
Could not load signature of C7GameData.C7SaveFormat:get_JsonOptions due to: Could not load file or assembly 'System.Text.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. (in domain , warning)

I tried searching online but didn't find any easy way to fix this. Any ideas?

By the way, are you guys using Mono or .NET Core? I've gone back and forth a couple of times now, basically when one breaks I try switching to the other. Recently I've been using .NET Core. I tried this under Mono but that was more badly broken, I couldn't even get the game to compile.

 

[Puppeteer]

I'm not sure what you're asking about Mono vs .NET Core. You always build the game in Godot, right? Then you're always using Mono with the Godot SDK. It's just command line stuff where I in particular have confused things by making dotnetcore/dotnet console apps.

Oh, I think I see what's happening with "new game": It's crashing when trying to read in a native C7 json save file. I thought maybe the save didn't get updated with recent changes, but even after refreshing it, it still crashes on "new game" or manually loading the json.

I'm going to guess the game data are now relying on new structures not added to the native save file. It works fine if you load a Civ3 save.

Edit: Submitted PR #65 to switch _Console/BuildDevSave to use Mono instead of dotnet. This also fixes an error BuildDevSave having to do with text encodings as we're now apparently reading text in the import code and dotnet 5 kicked the encodings out to a nuget package which wasn't imported, but 472 still has it natively.

 

[Puppeteer]

Digging a little further, the json read seems to be resulting in "recursive_mutex lock failed: Invalid argument". A quick Google search suggests this might have to do with target data being released before the thread is done with it. Or maybe that data is referenced before it is constructed.

Wild guess is that it's the latter. I looked at the updated ImportCiv3 code and it seems to be needing to create several values explicitly which may make deserialization require some extra code. Saving the json data seems to work, so reading it back in seems to be missing something.

Meanwhile just load a Civ3 save and avoid the "new game" or loading a json save.

 

[Flintlock]

I didn't even think of trying to load a SAV, but yeah that works, thanks for the reminder.

I'm not sure what you're asking about Mono vs .NET Core. You always build the game in Godot, right? Then you're always using Mono with the Godot SDK. It's just command line stuff where I in particular have confused things by making dotnetcore/dotnet console apps.

I do build the game through Godot. Honestly I don't even know what the Godot SDK is. My understanding is the Mono version of Godot is capable of running .NET programs, because it includes the Mono runtime, but not building them. When you go to download Godot the website tells you it requires either the .NET SDK or the Mono SDK and if you don't install either it can't compile anything.

Digging a little further, the json read seems to be resulting in "recursive_mutex lock failed: Invalid argument".

Where are you seeing this? The log file says "Could not load file or assembly System.Text.Json [...] or one of its dependencies" which makes me think the problem is not with the JSON file itself.