Valheim-Modding / Jotunn

Jötunn, the Valheim Library.
https://valheim-modding.github.io/Jotunn/
MIT License
120 stars 40 forks source link

[REFACTOR] The CustomRecipe dilemma #95

Closed paddywaan closed 3 years ago

paddywaan commented 3 years ago

During documentation, there was an inconsistency which has jumped out at me, and we must resolve before we release to the public as this will be a "now or never" change which will affect all documentation, examples, and dependant plugins going forward.

I will preface this issue by detailing the vanilla game's implementation for two similar object interfaces.

First, we have two individual component systems within valheim, that live in the ObjectDB. We have the m_recipes collection, and the m_items collection. Recipe is comprised of two notable attributes: public ItemDrop m_item;, and public Piece.Requirement[] m_resources = new Piece.Requirement[0];. Meanwhile, the m_items collection is of generic GameObjects, which may have an ItemDrop component, the same ItemDrop which can be referenced from a recipe.

Secondly, we have Pieces, that short of their piecetables do not have a objectDB style collection. Pieces, have a subclass named Requirement, which is a basic structure that defines an public ItemDrop m_resItem; and a quantity as an ingredient.

At this point, its clear to see some similarities in these implementations. Now lets look forward at how we interface with these libraries within our JVL abstractions:

Item

private void CreateBlueprintRune()
{
    // Create and add a custom item
    // CustomItem can be instantiated with an AssetBundle and will load the prefab from there
    CustomItem rune = new CustomItem(BlueprintRuneBundle, "BlueprintRune", false);
    ItemManager.Instance.AddItem(rune);

    // Create and add a recipe for the custom item
    CustomRecipe runeRecipe = new CustomRecipe(new RecipeConfig()
    {
        Item = "BlueprintRune",
        Amount = 1,
        Requirements = new PieceRequirementConfig[]
        {
            new PieceRequirementConfig {Item = "Stone", Amount = 1}
        }
    });
    ItemManager.Instance.AddRecipe(runeRecipe);
}

In the above, we create a CustomItem, and a CustomRecipe which can take a RecipeConfig as a parameter, that also requires a PieceRequirementConfig property to define its ingredient list.

Piece

private void CreateRunePieces()
{
    // Create and add custom pieces
    GameObject makebp_prefab = BlueprintRuneBundle.LoadAsset<GameObject>("make_blueprint");
    CustomPiece makebp = new CustomPiece(makebp_prefab, new PieceConfig
    {
        PieceTable = "_BlueprintPieceTable",
        AllowedInDungeons = false
    });
    PieceManager.Instance.AddPiece(makebp);
    GameObject placebp_prefab = BlueprintRuneBundle.LoadAsset<GameObject>("piece_blueprint");
    CustomPiece placebp = new CustomPiece(placebp_prefab, new PieceConfig
    {
        PieceTable = "_BlueprintPieceTable",
        AllowedInDungeons = true,
        Requirements = new PieceRequirementConfig[]
        {
            new PieceRequirementConfig {Item = "Wood", Amount = 2}
        }
    });
    blueprintRuneLocalizations();
    PieceManager.Instance.AddPiece(placebp);
}

Inconsistency

In the above, we create a CustomPiece, which takes PieceConfig as a parameter, that also requires a PieceRequirementConfig property to define its ingredient list.

If we compare this to the item example:

In the above, we create a CustomItem, and a CustomRecipe which can take a RecipeConfig as a parameter, that also requires a PieceRequirementConfig property to define its ingredient list.

The problem:

The problem here is inconsistency. We are obfuscating native implementations via our abstractions using inconsistent naming schemas to accomplish the same operations, not to mention potentially having some redundant nesting of tightly coupled objects/types when we could just abstract them away with parameters, but that's a problem for another time.

Bellow, I will note our equivalent implementation names, and what i suggest they should become ImplementationName Item Piece
CustomObject CustomItem CustomPiece
Recipe/Conf CustomRecipe PieceConfig
Ingredient PieceRequirementConfig PieceRequirementConfig

We can see that if we were to apply consistent naming schema, then the CustomItem should have an ItemConfig, not a CustomRecipe as a component parameter.

Furthermore, PieceRequirementConfig is a misnaming within the vanilla libraries, and just further adds to the confusing as now items have piece requirements, which makes 0 sense at all either.

We are better than this, there is no need to make this mistake and we can correct it before we release. I suggest that we do as such.

sirskunkalot commented 3 years ago

My proposal would be to get rid of the CustomRecipe and add the recipe generation into the CustomItem.

Item with a config class: new CustomItem(prefab, new ItemConfig())

Item with mocked Recipe: new CustomItem(prefab, RecipeInstance)

Piece with config class new CustomPiece(prefab, new PieceConfig())

PieceRequirements can be just Requirements