Criticisms and Ideas

[CptMoore]

Hey there,

I went through your code, and have some comments on your ModTek idea.

First and foremost, only concentrate on the manifest stuff asap, and only the most simple of things (not even partial loading). That seems to be the most important thing modders need. Once that is done and stable, move to dlls loading, partial loading, dependency management etc.

Topics

Manifest Additions

• (good) ability to load additions for the manifest and trying to do it automatically
• (bad) why not fully automated? just require a certain directory structure and load up stuff automatically based on where everything is. everything under modname/data/chassis load as chassis def etc.
• (bad) IgnoreDirectories and IgnoreFiles should not be necessary, if it can be solved by moving files out of a directory, no need to reimplement that functionality.

Partial JSON Loading

I would suggest postponing the partial json loading topic until a proper json manipulation language can be integrated (e.g. JSONata)

• (good) only change what you need
• (bad) how does that work for removing and adding stuff from/to lists? most of stuff I want to change are currently in lists (hardpoint stuff)
• (bad) one should probably use some kind of json query language to find data (JSONiq/JMESPath/JSONata/), and then replace it with native (string/int) or json.
• (bad) rather lots of code required and only useful for simplest mods (most mods that simple usually change the same thing.. all dmg).
• (good) a description/lore mod could benefit from this type of change and be used besides all those hundreds of upcoming dmg mods.

mod.json File

• (good) central file to describe your mod, and define it as a modtek mod
• (bad) would call it modtek.json though, so its unique across games
• (bad) why doesn't it just load up a fixed named modtek.dll similar to how modtek.json works? Makes a template project also easier, as people wouldnt need to know how to rename a dll in VS. Otherwise you solution is better than loading up all dlls. Oh, make it optional, default dll name to look for is modtek.dll .
• (bad) Newtonsoft.Json should not be necessary, use the JSONSerializationUtility to serialize from and to json directly into a C# class.

DLL loading

• (good) loading of the mod dll using modtek style Init
• (bad) I don't want to receive a dictionary as my settings. Instead of giving the directory or config, just pass in the modDef instasnce, that can already return the mod directory. Allow it to load up a config with a class as parameter, basically a wrapper around JSONSerializationUtility. e.g. InjuryConfig config = modtekDef.LoadConfig();

Dependency Management

I would suggest postponing the hole dependency topic and use a simple mod load order list, that also reflects the manifest load order mechanism used by battletech.

• (good) dependency management is someting that can be useful, larger mod communities usually build their own master lists to solve dependency issues
• (bad) all big games don't use this as a core loading mechanism, they use a simple mod order list which is a index with mods, its simple, doesn't require lots of thinking from anybody who has to debug the list. A dynamic dependency graph, especially if circles are introduces or bad versions are used can be problematic to debug.
• (good) ConflictsWith, but only if you can show conflicts after someone clicks battletech.exe
• (good) LoadAfter, very simple way to say, i need that other mod, so load me after it. "LoadAfter" : [ ["InjuryIgnore", "v1.0"] ]
• (bad) why LoadBefore and LoadAfter and DependsOn ? DependsOn is not necessary?
• (bad) why LoadBefore? Seems overly complicated, that assumes another mod overwrites stuff afterwards (like saying one wants to exchange a base class before someone subclasses it... )?

Suggested Directory Structure for Mods

• BATTLETECH\mods\mybattletechmod\source*.sln
  • I would like the source to be part of the mod directly
• BATTLETECH\mods\mybattletechmod\modtek.json
• BATTLETECH\mods\mybattletechmod\modtek.dll
• BATTLETECH\mods\modtek.log
  • one logfile for all mods, so bugs due to other mods interference can be more easily spotted
• BATTLETECH\mods\mybattletechmod\data\chassis\mybadass-150t-mech.json
  • the structure under data\ should be a replica of the streamingassets\data folder

Code Style

• always use curly bracets, yea might look like more code but its easier to have just one style
• use early exits, no if.. if.. if.. nested stuff, negate each if and return early to make everything flat.
• instead of == null and != "" use string.IsNullOrEmpty .
• if you have the money, buy Resharper from JetBrains, allows lots of automatic refactoring of code, supports linq code.
• for each class create its own .cs file, use folder to group together stuff. Only do this once you are out of prototyping though and you know your folder structure is kinda set.

My Contributions

If you want I can try to address any of the points I rased above.

As a first small step I would like to provide the following things in a PR:

• Introduction of JSONSerializationUtility for mod config loading
• Logger class that can be reused for further dll mods. should probably be added to a modDef instance that should be given in Init
  • Have a single logger file so once different mods load up, its clear which ones are crashing or not working.
• Share traverse code (I call them adapters) to allow quick access to private fields and properties for various classes, usually its related to patches.

Further ideas for ModTek

• Dynamic compilation of code, allows creating dll mods without having VS corresponding knowhow (easy modding for small stuff).
• Full description on how to use dnspy

[mpstark]

Thanks for taking a look -- I've read through your suggestions and they're extremely helpful. Let me take a bit to digest, look through the code, and get a proper reply written up.

[mpstark]

mod.json specification

• I see your point about the name, but I dislike all mods including a modtek.json, it seems sort of ambiguous, which seems to be the point of moving away from the mod.json name; what do you think about MyModName.modtek.json?
• Definitely agree about implicit loading of specific directories and and removing IgnoreFiles/IgnoreDirectories, adding a DontLoadImplicit bool to the ModDef would allow mods to do own handling if need be.
• Regarding Newtonsoft.Json, it's included with the game as it's one of the libraries that they use, I've matched the NuGet package version with the included library -- as far as I know, there is no advantage to not using it

DLL Mods

• "Shipping" a first version without any .dll loading is a mistake
• Mod .dlls should never have to include a reference to ModTek.ModDef
• Dictionary<string, string> is bad, I agree -- I'm now thinking about just passing the JSON for the settings as a string, that way the mod can do it's own parsing of the settings file
• I very much dislike the idea of implicit loading of .dll files. Also, having mods ship modTek.dll is a recipe for disaster, as many people might drag the file over the actual ModTek.dll or simply put it alongside.

Partial JSON Loading

• "Shipping" a first version without this any type of partial JSON is a mistake
• The large constants files like SimGameConstants.json that make up a bulk of user tweaks are a giant usecase that will make up a great percentage of the "simple" mods
• Newtonsoft.Json includes a handy simple json merge function that happily does a lot of the work for us
• There are a fair amount of JSON errors in the game JSONs, which might be somewhat of a challenge to overcome

Dependancies

• First version will likely not include anything but simple dependency support (i.e. load things with no dependencies, then loop loading dependents as their dependencies are satisfied, stopping when nothing is loading)
• DependsOn is an indicator that this mod should be loaded after that mod, and if that mod isn't installed, then don't load this mod at all.
• I agree that LoadBefore/LoadAfter are probably redundant, and that if this sort of thing, when it needs to be specified, a loadorder file or something would be better.

Logging

• I do not believe that loaded mods should be able to write to ModTek.log
• It's easy enough to roll your own log file in your own directory

As to your coding style suggestions, I'll be sure to start using them! I think sometimes no curly brace statements can be a net-benefit for code clarity, but I know that I wasn't consistent about it.

[mpstark]

I would be very happy to see any Harmony-specific tips and tricks if you have them. Traversing to find private members sounds very, very useful, although I think the next Harmony version will include some additional tools to do that.

[CptMoore]

mod.json specification

MyModName.modtek.json

I would want to avoid having to write the mod name everywhere.. ideally I only need to change it at one place for everything (code, files, ..). Every file name, every variable that includes a name, that I have to set myself, is just another source of error when creating new mods.

I rather have mod.json than mymodname.modtek.json . What about settings.modtek.json ?

adding a DontLoadImplicit bool to the ModDef would allow mods to do own handling if need be.

Why not create an autoimport directory? call it autoimport.. ? then there would be no need for the flag and its clear whats happening to the files inside there

Newtonsoft.Json [...] as far as I know, there is no advantage to not using it

Yea no difference here, I usually prefer using the utilities the ecosystem is using, in this case Battletech seems to be using JSONSerializationUtility for their data files. Newtonsoft is better documented though and should work fine.

DLL Mods + Logging

Mod .dlls should never have to include a reference to ModTek.ModDef

I'm now thinking about just passing the JSON

It's easy enough to roll your own log file in your own directory

I believe you don't want ModTek internals be exposed to a mod right? There should be a modtek-modding library that both ModTek and the Mod can share, so ModTek can fill up a context (similar to ModDef) and let the mod have that.

I really would want an easy to use utility (easy logging, easy file access, easy configuration loading, additional dynamic manifest loads) to jumpstart mods, so such a shared lib would allows that easily. What do you say?

Or are you afraid of compatibility issues over time? Thats why that helper lib should be bare bones only right?

having mods ship modTek.dll is a recipe for disaster

call it different then, like mod.dll or mod.modtek.dll

look at how factorio does it https://wiki.factorio.com/Modding

info.json control.lua data.luia

they also have a nice info.json

{
  "name": "EvoGUI",
  "version": "0.4.302",
  "factorio_version": "0.16",
  "title": "EvoGUI - Evolution Factor Indicator and more",
  "author": "Octav \"narc\" Sandulescu",
  "contact": "factorio-mods@narc.ro",
  "homepage": "https://github.com/narc0tiq/evoGUI/",
  "description": "Places some indicators on your UI for various little statistics that are nice to have.",
  "dependencies": ["base >= 0.16.1", "? moweather"]
}

? = optional ! = required I believe

In order to avoid wrong handling of everything, mods are zipped.

See Assembly.Load(byte[]) to load an assembly from memory, so read it from the zip into memory then load the bytes. Of course this would now definatly require a helper class to handle loading files from a zip.

Partial JSON Loading

The large constants files like SimGameConstants.json that make

Yea ok, especially like changing the starting mech becomes very easy, or adding in the tutorial skip (well the devs will fix that but I get your point).

Newtonsoft.Json includes a handy simple json merge function that happily does a lot of the work for us

The problem is the setting, sometimes I want to replace an array, sometimes I want to add an element to the end, sometimes I want to remove something but not everything. That requires more precise modification abilities. Question would be if that's necessary for the first ModTek release or not or even ever. And what should be the standard, Union, Merge or Replace? I would suggest replace, in worse case one has to replicate data, but at least one can always remove unwanted data without creating a DLL mod.

There are a fair amount of JSON errors

Really? Would explain why they use their own JSON utility?!

[CptMoore]

I would be very happy to see any Harmony-specific tips and tricks if you have them.

Well its very simple to use, mostly just annoying to find and expose the private members every time you find them.

I usually create a class I name something-Adapter to signify that its just an access point to an existing class.

    internal class MechLabLocationWidgetAdapter
    {
        private readonly MechLabLocationWidget _instance;
        private readonly Traverse _traverse;

        internal MechLabLocationWidgetAdapter(MechLabLocationWidget instance)
        {
            _instance = instance;
            _traverse = Traverse.Create(instance);
        }

        internal MechLabPanel MechLab => _traverse.Field("mechLab").GetValue() as MechLabPanel;
        internal TextMeshProUGUI LocationName => _traverse.Field("locationName").GetValue() as TextMeshProUGUI;
        internal List<MechLabItemSlotElement> LocalInventory => _traverse.Field("localInventory").GetValue() as List<MechLabItemSlotElement>;
        internal LocationLoadoutDef Loadout => _instance.loadout;
    }

Just remember to use Property instead of Field is its an actual Property you are accessing. Thats it already..

[CptMoore]

btw my new suggestion for a mod structure

Suggested Structure for Mods

• BATTLETECH\mods\mybattletechmod\source*.sln
  • I would like the source to be part of the mod directly
• BATTLETECH\mods\mybattletechmod\mod.json
• BATTLETECH\mods\mybattletechmod\mod.dll
• BATTLETECH\mods\modtek.log
  • one logfile for all mods, so bugs due to other mods interference can be more easily spotted
• BATTLETECH\mods\mybattletechmod\data\chassis\mybadass-150t-mech.json
  • the structure under data\ should be a replica of the streamingassets\data folder
• BATTLETECH\mods\mybattletechmod\patches\chassis\chassiDef-atlas.json
  • patches\ should be used for partial jsons, lets call em patch jsons? or make them in the same data\ folder as full jsons but call them .patch.json ?

[mpstark]

• We'll go with mod.json -- it's simple, short, and uncomplicated
• I don't want to load a .dll that isn't explicitly named, which is one of the reasons that BTML's implicit loading is limited to the root directory -- I dislike the idea of a default name for the much the same reason.

Implicit Loading of Manifest Entries

The way I see this feature working is that we'll define two different implicitly-loaded directory structures that don't need to be included in the mod.json file.

The first (and most presently pressing) would be the data directory that mirrors the streaming assets one -- the purpose behind this directory is to modify/replace existing game files. In this regard, supporting JSON merge operations on JSON with specific types, with whatever MergeArrayHandling value we choose as the default.

The second would be directories with pluralized type names like ModFolder\MechDefs\. This would generally be for new additions with no partial JSON files that expect to be merged.

Any behavior beyond this is specified in mod.json's manifest. These implicitly-loaded directories need to be able to be configured by the mod in-case the modder wants the behavior to change. First of all, specifying "LoadImplicitManifest": false, will disable it completely. Providing a ManifestEntry in the manifest that matches an implicit one will also take precedence. Full control should always be possible.

Regarding Utilities/Logging

I want ModTek to stay dependent on only the game and BTML. I don't believe that other mods should ever rely on ModTek's assembly itself. I do not want centralized logging into ModTek's log.

These are design decisions and ultimately rely on personal value judgements.

[CptMoore]

The second would be directories with pluralized type

any reason for that? why not mimic streamingAssets\data again?

Any behavior beyond this is specified in mod.json's manifest

do we still need manual manifest loading if the automatic one already works? whats the point of having two ways of doing things? in the automatic case the mod author still has full control, one can still decide what files to put in a directory.

is the automatic way not working with everything and therefore manual manifest entries are still required?

I want ModTek to stay dependent on only the game and BTML

I am a bit confused, why not just integrate BTML as part of ModTek? ModTek should become the new BTML, maybe even provide a own startup exe for in-memory injection for battletech, so no need to modify any files anymore, just start modtek.exe .

I'm thinking for the user to have the simplest experience possible.

I don't believe that other mods should ever rely on ModTek's assembly itself

Sure maybe not the main ModTek assembly, but what about a helper library?

[CptMoore]

@Mpstark what do you think about the dynamic compilation feature? like adding a .cs file to a modtek based mod and that will be compiled dynamically? I'm right now looking at making it feasible.

[mpstark]

Could be a feature later on, but right now, looking at nuts and bolts of the first release.

[umbravi]

Great thread thus far, with a lot of great points. On:

instead of == null and != "" use string.IsNullOrEmpty .

This is rather small, but:

• instead of string.IsNullOrEmpty use string.IsNullOrWhiteSpace

[CptMoore]

I'm currently splitten up all my points I made into separate tickets and add more information to it. @Mpstark please feel free to re-add your comments on the topics to those tickets.

[mpstark]

I'm leaving issue open so that others can chime in. Thanks for breaking it up though, makes it a bit easier to handle.