[Dev] 0.1 "Babylon" Progress Thread

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.

 

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?

 

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.

 

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:

Code:

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.

 

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.

 

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:

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.

 

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.

 

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.