C7-Game
Refactor unit de/spawning
Currently units are created by new MapUnit() or UnitPrototype.GetInstance() and then their contents are set manually. As units are fleshed out, this is becoming more awkward and error prone. For example, after adding experience levels to the game I had to edit four different spots where units are created to give them their starting experience levels. We should instead have a single unit-creating method that makes sure units are properly filled out.
The natural thing would be to use the MapUnit constructor for this purpose, except that conflicts with other aspects of the code. The first conflict is with the engine/data separation that I've complained about before. Since the constructor is part of the game data module it doesn't have access to the actual instances of the game data & rules. Initially, for experience levels, I wanted units to be created at the default experience level which then could then be replaced later as needed. That would have been nice since it would have kept unit creation simple and ensured all units always have a valid experience level. Unfortunately that's not possible since the experience levels are part of the scenario data, which is inaccessible to the game data module. The obvious solution would be to take the initial experience level as a parameter to the constructor, except that conflicts with the empty object pattern we're using. MapUnit.NONE couldn't be constructed with an experience level b/c, again, it's in the game data module that doesn't have access.
I'm planning to use UnitPrototype.GetInstance as the single unit-creating method, except I'll move it into the engine as an extension method so that way it can initialize units using the scenario data. This means the game data module won't have an easy way to create units but that's okay since it shouldn't need to. Right now the only time it creates units is to set up the dummy game, which should be done in the engine, but doesn't matter anyway since it's temporary. Also while I'm at it I'm going to separate out the concepts of disbanding & despawning units. Right now we disband units to get rid of them, but disbanding in a city is supposed to award shields, which is not what we want.
What was the reasoning / discussion behind making the game data separate? If we want some separation of game data vs. engine logic, why not move all that game data code into the engine project in its own folder?
The only place this was discussed, that I know of, was in the architecture thread starting with my post here: https://forums.civfanatics.com/threads/dev-architecture-thread.674254/page-2#post-16236236 You can read Quintillus' reasoning for the separation there, but the short story is that it's a carryover from how he designed his editor. In my opinion we should merge the game data and engine modules since that would simplify the job of writing the game logic. It might complicate other things instead but game logic should take priority. An editor would have little to no game logic so there the separation makes more sense.
The obvious solution would be to take the initial experience level as a parameter to the constructor, except that conflicts with the empty object pattern we're using.
MapUnit.NONEcouldn't be constructed with an experience level b/c, again, it's in the game data module that doesn't have access.
Maybe I'm missing something, but why wouldn't this work? City.cs is already using that pattern, including for City.NONE. ExperienceLevel exists in C7GameData just like MapUnit and UnitPrototype do. GameData is also part of C7GameData.
I agree that there should be a standard interface for this, but I'm not sure why it doesn't work with the structure we have.
What was the reasoning / discussion behind making the game data separate? If we want some separation of game data vs. engine logic, why not move all that game data code into the engine project in its own folder?
The link Flintlock linked to is relevant, but the short version is actually not that it's just a carryover for no reason, but because it makes it clear what the structure of the save format is. If you want to know the structure of the format, you can look at C7GameData and figure it out. This also makes it clear that any time you add a variable to a class in C7GameData, it will be added to the save (unless you add the [JsonIgnore] annotation to it).
It also has the side benefit of allowing potential side projects to use the data format without being tied to the whole engine. The most likely one at this point is the map generation exploration that @JimOfLeisure has mentioned. The last I saw, he was thinking of doing it in a completely separate/cloned repo, but it could be done in a branch of this repo, using C7GameData to import just the data.
But, we should start documenting these sorts of decisions, probably on GitHub discussions. Another one is the reasoning for the "Extension" methods - tonight I believe it was due to wanting to integrate animations with the C7GameData (where animations are definitely not part of the data), but the last time I looked at it I couldn't remember why we'd adopted that paradigm.
Maybe I'm missing something, but why wouldn't this work? City.cs is already using that pattern, including for City.NONE.
It could be made to work by passing null for the experience level or adding an ExperienceLevel.NONE object. I just don't want to have to do that. The best way to avoid null reference exceptions is to avoid using null whenever possible. It would be nice if we could guarantee that all units always have a valid experience level by enforcing that in the constructor and setter. That way no code ever needs to test for unit.experienceLevel != null. The problem with the NONE object, which really is a common problem with the whole empty object pattern, is that it's just null by a different name. The empty object often can't stand in for a real one so you end up with tests against it scattered everywhere just like tests against null would have been.
Experience levels are a simple enough feature that we could get away with a null or an empty object without causing any real trouble later on, probably. On the other hand, because they're a simple feature, I want to use them as a starting point for what I consider to be a better design. If we can first ensure that all units always have a valid experience level, we can next ensure they always have valid types and player owners, then ensure things about cities, etc.
Another one is the reasoning for the "Extension" methods - tonight I believe it was due to wanting to integrate animations with the C7GameData (where animations are definitely not part of the data), but the last time I looked at it I couldn't remember why we'd adopted that paradigm.
Extension methods are really just a syntactic thing. They allow you to write unit.animate(...) instead of the normal static method call which would be something like AnimationManager.animate(unit, ...). The issue is that almost all of the game logic will need to be written as static methods, extensions or not, inside the engine module so they can have access to the game data in EngineStorage. For example, anything that relies on units' effective strengths (including defensive bonuses) will need to be in the engine since defensive bonus values are specified in the scenario data. It works but it's awkward. The more natural thing would be to put all that code in class methods.