Skip to content

Conversation

@pcen
Copy link
Contributor

@pcen pcen commented Feb 22, 2023

does not directly address the discussion in #377, but adds the necessary signals to use the save game button in the game menu, and saves/loads games using c# references

it also ~halves the size of saves by not writing common boolean fields when they are false, since upon serialising the save the default value if not present will be false.

@pcen pcen changed the base branch from Development to pcen/godot-4 February 22, 2023 01:39
@pcen pcen mentioned this pull request Feb 22, 2023
@pcen
Copy link
Contributor Author

pcen commented Feb 23, 2023

When serialising interfaces or inheritance hierarchies, you need to annotate the interface / base class with the types that implement / inherit from it, along with a type discriminator to store in the JSON (.NET 7 doc). The basic pattern is:

[JsonDerivedType(typeof(FooImplementsISomeInterface), typeDiscriminator: "iAmFoo")]
interface ISomeInterface {...}

This isn't possible to do for the StrategicPriority type with the current C7Engine / C7GameData structure. StrategicPriority is defined in the C7GameData project, in the namespace C7Engine.AI.StrategicAI since Player must hold a list of StrategicPriority instances, but the type would be inaccessible if declared in C7Engine since C7Engine includes C7GameData, and dotnet does not allow circular includes. All of the types implementing StrategicPriority are declared within C7Engine, since the implementation of some of them require access to EngineStorage, so they cannot be implemented in C7GameData along with the class they inherit from. The issue is that StrategicPriority cannot be annotated with typeof(ClassInheritingFromStrategicPriority), since these classes are in C7Engine, and thus inaccessible.

If we keep the separation, I think fundamentally the solution is to make everything in AIData (and every class that implements it) strictly plain old data with no methods (beyond maybe those which access or mutate only internal members), but this seems like an non beneficial restriction for a game that relies heavily upon state and mutating it.

I think we should revisit combining the two because issues like these require refactors that provide no benefit other than working around the dependency issue, and the only benefit we get from separating the game data and engine is ease of writing tools, but we can also get this separation by having two distinct namespaces within a single project containing engine code and game data code, if such a separation is necessary. @WildWeazel @QuintillusCFC @maxpetul

@QuintillusCFC
Copy link
Member

When serialising interfaces or inheritance hierarchies, you need to annotate the interface / base class with the types that implement / inherit from it, along with a type discriminator to store in the JSON (.NET 7 doc). The basic pattern is:

[JsonDerivedType(typeof(FooImplementsISomeInterface), typeDiscriminator: "iAmFoo")]
interface ISomeInterface {...}

This isn't possible to do for the StrategicPriority type with the current C7Engine / C7GameData structure. StrategicPriority is defined in the C7GameData project, in the namespace C7Engine.AI.StrategicAI since Player must hold a list of StrategicPriority instances, but the type would be inaccessible if declared in C7Engine since C7Engine includes C7GameData, and dotnet does not allow circular includes. All of the types implementing StrategicPriority are declared within C7Engine, since the implementation of some of them require access to EngineStorage, so they cannot be implemented in C7GameData along with the class they inherit from. The issue is that StrategicPriority cannot be annotated with typeof(ClassInheritingFromStrategicPriority), since these classes are in C7Engine, and thus inaccessible.

If we keep the separation, I think fundamentally the solution is to make everything in AIData (and every class that implements it) strictly plain old data with no methods (beyond maybe those which access or mutate only internal members), but this seems like an non beneficial restriction for a game that relies heavily upon state and mutating it.

I think we should revisit combining the two because issues like these require refactors that provide no benefit other than working around the dependency issue, and the only benefit we get from separating the game data and engine is ease of writing tools, but we can also get this separation by having two distinct namespaces within a single project containing engine code and game data code, if such a separation is necessary. @WildWeazel @QuintillusCFC @maxpetul

Interesting, so... basically my take is .NET 7 missed it with polymorphic serialization, coming from a Java background where there are libraries like GSON that make it super easy. They do mention at the end of the article that you linked:

For use cases where attribute annotations are impractical or impossible (such as large domain models, cross-assembly hierarchies, or hierarchies in third-party dependencies), to configure polymorphism use the contract model. The contract model is a set of APIs that can be used to configure polymorphism in a type hierarchy by creating a custom DefaultJsonTypeInfoResolver subclass that dynamically provides polymorphic configuration per type

But the implementation of that is a lot more complex than it would be for the same thing in Java.

As for the AI packages not all being in one area, the EngineStorage is part of it, but the larger-scale picture is extensibility. Everyone's always complaining about the Civ3 AI, so the idea was you could plug your own AI classes in for mods, from your own C# packages. As long as they conformed to the interface for that type of AI (combat AI, city building AI, strategic priority AI, etc.), you could customize them. I think this is similar to how Civ4 did it, although I never really got into Civ4 modding.

For that to work, the implementing classes must be able to be outside of C7GameData.

One option would be to give up on that customization option and say sorry, everything has to be in C7GameData.

I see two viable ways to achieve the original goal though:

  1. The "plain old data" approach you mentioned, basically everything is stored in a map and the storage for AI objects is a key-value lookup. This is the "lowest common denominator" for me - it works, but for the more complex parts of the AI it's likely to be a bit awkward compared to being able to have traditional nested data structures.
  2. Use Newtonsoft JSON. I'm 98.2% sure Newtonsoft would work for polymorphic inheritance, and was starting to look into that until we realized Godot 4/.NET 7 were more imminent than expected, and might also solve the problem well. I think we have a use case where "the built-in solution isn't good enough, so we're using a third-party library" is justified. There's nothing wrong with that at a fundamental level, either; in Java almost nobody uses the build-in logging library, the third-party ones are simply better so they're what almost everyone uses.

@pcen
Copy link
Contributor Author

pcen commented Jul 31, 2023

closing, this is stale experiment code

@pcen pcen closed this Jul 31, 2023
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.

3 participants