StrangeLoopGames / EcoModKit

Eco Modkit
https://docs.play.eco
69 stars 25 forks source link

Favor instance based registration over types. #45

Open SerratedSharp opened 2 years ago

SerratedSharp commented 2 years ago

It seems like using a data driven approach for various things like Recipes isn't possible since so many internal collections are indexed on types.

Currently it is analogous to if I wanted to declare two mailing addresses in a contact list application, I couldn't just new()/add() a new instance of an AddressClass and set its properties, I would have to declare a BobsAddress and KarensAddress class for each address. Structure wise they'd be identical, and just have different values for properties, which would make it more appropriate for instances. There's no need for multiple class declarations.

RecipeFamily/Recipe's for example, have almost no behavior. Almost all of them are just building a data structure relating the recipe to its input/output ingredients, settings property values, etc.. They are POCOs for the most part. If you allowed someone to just new RecipeFamily() and set all the appropriate properties, then all the information needed for the recipe is there without declaring a new type. There's no functionality in my opinion that requires a separate class declaration.

If mod devs wanted compile time safety on the index strings, they could still use nameof(BlahRecipe) or .GetType().Name instead of typeof, but devs who are using a data driven approach to register multiple recipes would no longer need to create class declarations for each and have the flexibility to customize string identifiers.

If there was something like a IRegisterRecipes interface, then you could still retrieve all types declared with a particular attribute similar to the approach of InstancesOfCreatableTypesParallel, but also find all implementations of IRegisterRecipes and call IRegisterRecipes.Initiailize, which would be how we provide multiple recipe instances. The rest of the initialization code would be identical, you would just have an extra method that calls the IRegisterRecipe interfaces and adds them to the collection from InstanceOfCreatable... before beginning initialization. You could also support priorities on IRegisterRecipe so if someone needed their recipes to be registered last it would be possible.

This would allow more extensibility by allowing programmatic/data-driven implementations to extend functionality, in contrast to the current declarative only option. There'd also be maintainability approaches that I could employ which wouldn't be possible with the declarative approach.

It would also make it easier for SLG to make future API changes without introducing immediately breaking changes. If you are the implementer of RecipeFamily for example, and everyone is just new'ing up instances of it in their code(instead of being required to subclass), it's easier to maintain the existing methods/properties, mark them as deprecated, add/change new properties/methods, and have both the old/new interact with internal fields in a way that supports both APIs for some period of time. This way you can phase in breaking changes without them immediately breaking mods/(and servers using those mods) giving mod devs ample time to address those changes. If everyone is instead required to subclass RecipeFamily, then it's alot more difficult to predict all the ways they are interacting with the class, nor create backwards compatible proxy members, and create a backwards compatible behavior while phasing in changes across multiple versions without breaking mods immediately.

In my case, since I'm trying to add Recipes that are variations of existing recipes, it's not feasible to create a class declarations since I need to iterate over an admin supplied data structure and add a recipe for each. I would be looking at something more akin to .Emit which I can't guarantee would execute soon enough for it to be picked up by the reflection done with InstancesOfCreatableTypesParallel.

As another litmus test, there's almost nothing in declaring a recipe that actually follows a declaritive pattern. A typical implementation is 90% setting properties. If I were to implement by creating an instance rather than declaring a class, the code would still be almost identical, the only difference being I'm new'ing a RecipeFamily and using that variable rather than this.

        var recipe = new Recipe();
        recipe.Init("MyUID",  Localizer.DoStr("Janky Hewn Logs"),
            new List<IngredientElement> {
                    new IngredientElement("Wood", 2, typeof(BasicEngineeringSkill)), //noloc
            },
            new List<CraftingElement> {
                    new CraftingElement<HewnLogItem>(1),
            });

        var family = new RecipeFamily();
        family.Recipes = new List<Recipe> { recipe };
        family.ExperienceOnCraft = 0.5f;
        family.LaborInCalories = CreateLaborInCaloriesValue(20, typeof(BasicEngineeringSkill));
        family.CraftMinutes = CreateCraftTimeValue("MyUID", 0.16f, typeof(BasicEngineeringSkill));
        family.ModsPreInitialize();
        family.Initialize(Localizer.DoStr("Hewn Logs"), "MyUID");
        family.ModsPostInitialize();
        CraftingComponent.AddRecipe(typeof(WorkbenchObject), family);
D3nnis3n commented 2 years ago

Transferred to ModKit.