OrderOfThePorcupine / ProjectPorcupine

Project Porcupine: A Base-Building Game...in Space!
GNU General Public License v3.0
294 stars 91 forks source link

[Discussion] JSON Formatting for files #46

Closed BraedonWooding closed 6 years ago

BraedonWooding commented 7 years ago

Issue by BraedonWooding Thursday Apr 06, 2017 at 05:59 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT Originally opened as https://github.com/TeamPorcupine/ProjectPorcupine/issues/1794


Information

This is purely design, and doesn't contain the full document. Some of the names are capitalized (maybe incorrectly but again this is more the layout then specifics).

Structure?

The structure follows a more modern and cleaner json style, instead of an array of objects we just define it as a single object with the top level property names often being the 'type' or 'name' of the object, i.e. { "power_cable": {...} }. This means that iterating through every property which while is a little more work on our side, makes the modders job easier and makes documents actually readable. Every document is valid JSON also (obviously).

Examples

This

<?xml version="1.0" encoding="utf-8" ?>
<Inventories>
    <Inventory type="raw_iron" maxStackSize="50" basePrice="0.8" category="inv_cat_raw" localizationName="inv_raw_iron" localizationDesc="inv_raw_iron_desc"/>
    <Inventory type="ice" maxStackSize="10" basePrice="1.2" category="inv_cat_raw" localizationName="inv_ice" localizationDesc="inv_ice_desc"/>
    <Inventory type="raw_copper" maxStackSize="50" basePrice="0.8" category="inv_cat_raw" localizationName="inv_raw_copper" localizationDesc="inv_raw_copper_desc"/>
</Inventories>

To

{
    "raw_iron": {
        "MaxStackSize": 50,
        "BasePrice": 0.8,
        "Category": "inv_cat_raw",
        "LocalizationName": "inv_raw_iron",
        "LocalizationDesc": "inv_raw_iron_desc"
    },
    "ice": {
        "MaxStackSize": 10,
        "BasePrice": 1.2,
        "Category": "inv_cat_raw",
        "LocalizationName": "inv_ice",
        "LocalizationDesc": "inv_ice_desc"
    },
    "raw_copper": {
        "MaxStackSize": 50,
        "BasePrice": 0.8,
        "Category": "inv_cat_raw",
        "LocalizationName": "inv_raw_copper",
        "LocalizationDesc": "inv_raw_copper_desc"
    }
}

This

<?xml version="1.0" encoding="utf-8"?>
<Overlays>
    <Overlay type="oxygen" min="0" max="1" colorMap="Jet">oxygenValueAt</Overlay>
    <Overlay type="room" min="0" max="1" colorMap="Random">roomNumberValueAt</Overlay>
</Overlays>

To

{
    "oxygen": {
        "min": 0,
        "max": 1,
        "colorMap": "Jet",
        "functionName": "oxygenValueAt"
    },
    "room": {
        "min": 0,
        "max": 1,
        "colorMap": "Random",
        "functionName": "roomNumberAtValueAt"
    }
}

Files

Below are all the files. They are in .txt cause that is the only format that github allows for text. Headlines.txt Inventory.txt Need.txt Overlay.txt PerformanceHUDTemplate.txt Quest.txt RoomBehavior.txt SettingsTemplate.txt SheduledEvents.txt Ships.txt Stats.txt Tiles.txt Trader.txt Utility.txt ConsoleCommands.txt Currency.txt Furniture.txt GameEvents.txt

BraedonWooding commented 7 years ago

Comment by koosemose Thursday Apr 06, 2017 at 06:33 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


At a glance it looks good. (will look into it further at a later point, as I get time).

As Furniture is the first I looked at, I will make a few comments on that for the time being (that may be applicable to others).

The most notable concern (perhaps the only one that jumps out to me at the moment.) is that some things seem to have more nesting than strictly needed.

For Job, if Time is the only descendant, it seems that it could be simpler (and no less readable, possibly more readable) as simply a JobTime property. Similarly with the Inventory amounts, having the type as property name, and the amount as the value (though perhaps this loses some amount of readability). Others that could potentially benefit from the same treatment are: DefaultSpriteName and Usename, and Provides and Rate in PowerConnection.

So a proposed alternate version for Furniture:

{
    "steel_wall" : {
        "TypeTag": "Wall",
        "Width": 1,
        "Height": 1,
        "Health": 1000,
        "LinksToNeighbours": "wall",
        "EnclosesRooms": true,
        "DragType": "path",

        "OrderActions": {
            "Build": {
                "JobTime": 1,
                "Inventory": {
                    "steel_plate": 5
                }
            },

            "Deconstruct": {
                "JobTime": 1,
                "Inventory": {
                    "steel_plate": 3
                }
            }
        },

        "Parameters": {
            "thermal_diffusivity": 0.2
        },

        "Components": {
            "Visuals": {
                "DefaultSpriteName" : "steel_wall_"
            }
        },

        "LocalisationCode": "furn_steel_wall",
        "UnlocalizedDescription": "furn_steel_wall_desc"
    },

    "rtg" : {
        "TypeTag": "Temperature",
        "MovementCost": 2,
        "PathfindingModifier": 0,
        "PathfindingWeight": 2,
        "Width": 1,
        "Height": 1,

        "OrderActions": {
            "Build": {
                "JobTime": 2,
                "Inventory": {
                    "steel_plate": 2,
                    "raw_uranium": 2
                }
            },

            "Deconstruct": {
                "JobTime": 1,
                "Inventory": {
                    "raw_uranium": 1
                }
            },

            "Uninstall": {
                "JobTime": 1
            }
        },

        "Actions": {
            "OnInstall": "Rtg_InstallAction",
            "OnUninstall": "Rtg_UninstallAction",
            "OnUpdateTemperature": "Rtg_UpdateTemperature"
        },

        "Parameters": {
            "base_heating": 500,
            "pressure_threshold": 0.2
        },

        "Components": {
            "PowerConnection": {
                "Provides" : 2
            }
        },

        "LocalisationCode": "furn_rtg",
        "UnlocalizedDescription": "furn_rtg_desc"
    }
}
BraedonWooding commented 7 years ago

Comment by BraedonWooding Thursday Apr 06, 2017 at 09:03 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


Sadly I wish that could be carried out, the only problem is all of those have multiple other children parameters, such as Visuals.DefaultSpriteName having a function way of getting the name. Same deal with job which as Time and FromFunction. Note: though this does allow for in the future those things to occur if we remove the FromFunction since in what way will a job ever take a variable amount of time?

BraedonWooding commented 7 years ago

Comment by koosemose Thursday Apr 06, 2017 at 09:42 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


Ahh... I had assumed you'd chosen examples so that all (or most) variations would be present.

One possible way for a job to take variable time would be skill playing a part, or status of Crew (i.e. 1 armed Crew taking longer to build a thing). Though it could also be handled a different way as well (for example anything that could vary the time being built into the Order Action... which would make sense, since, as far as I can think of, if a build job, for example, can take a variable amount of time, all build jobs should have the same sort of variability, rather than having a per furniture way of handling it.

Perhaps having multiple options so instead of the nested version, you have options of using JobTime, or JobTimeFromFunction (or alternatively, it's smart about it, and if it can be converted to a number it uses that directly, and if it can't be converted it's treated as a function name).

It might be useful for discussion sake, rather than having 1 or 2 examples of each, having an "uberexample" have one object with every possible property (rather or not it would actually make sense in an actual object of that type). Minimize the conversational round trip time from a suggestion based on assuming something has fewer options than it actually does.

BraedonWooding commented 7 years ago

Comment by BraedonWooding Thursday Apr 06, 2017 at 10:32 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


Note: I did choose examples that show most variations, just there were none there with jobs so I kinda forgot to makeup one, especially since you will have EITHER Time or FromFunction, not both at any one time (for jobs and the same goes with the other ones). So really it would be 2-3 uberexamples containing every possible variant, still can do it though.

BraedonWooding commented 7 years ago

Comment by koosemose Thursday Apr 06, 2017 at 17:22 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


You could use some kind of marking to designate that it's one or the other... Perhaps an @ in front... since it looks sort of like a radio button, as in only one choice can be made..?

BraedonWooding commented 7 years ago

Comment by BraedonWooding Friday Apr 07, 2017 at 06:12 GMT # Sample: Friday Sep 13, 2013 at 22:58 GMT


I still want to keep it as perfectly valid json, but I have an idea to make the document more clear and nice.

BraedonWooding commented 7 years ago

New files remove some of the clutter

koosemose commented 7 years ago

Ok, I have my converter working, even have it able to figure out the types from the file (so far it's been reliable) with some tricks that may or may not have something to do with reflection. Currently I am doing it simplistically which is converting the list of objects to json, which means it has an extra layer to it. I'll look into a more complex way of doing that so that it removes the extra layer. And I can't quite match the formatting we've discussed. And some of the more complex types (Furniture being a prime example) still need some of their subelements dummied up to handle conversion to json properly.

As an example, here's probably the simplest one there is, just to give an idea of how the base level conversion looks:

[
  {
    "Type": "Strength",
    "Name": "Strength"
  },
  {
    "Type": "Dexterity",
    "Name": "Dexterity"
  },
  {
    "Type": "Constitution",
    "Name": "Constitution"
  },
  {
    "Type": "Intelligence",
    "Name": "Intelligence"
  },
  {
    "Type": "Wisdom",
    "Name": "Wisdom"
  },
  {
    "Type": "Charisma",
    "Name": "Charisma"
  }
]

This one is actually probably the most different from your prototype version, as it's such a simple file, it may actually need a lot more special casing to make it come out that simple... though I think we may want to have it not quite that simple, so it doesn't require completely changing it should we add more to it. (for example with stats, perhaps we might want to allow it to contain a max and min, in case we want them to vary, or a "difficulty" or some other concept that causes them to go up from different rates.

koosemose commented 7 years ago

With a touch more massaging (adding an ignore attribute to Type, and creating a GetDictionary method in PrototypeMap) gets a version much closer (and for the reasons I stated in the previous comment, I think that while currently this is somewhat redundant looking, it is both clearer (name should probably be changed to LocalizationName, which would make it a bit less redundant looking) and should be more sturdy for future changes.)

Current version:

{
  "Strength": {
    "Name": "Strength"
  },
  "Dexterity": {
    "Name": "Dexterity"
  },
  "Constitution": {
    "Name": "Constitution"
  },
  "Intelligence": {
    "Name": "Intelligence"
  },
  "Wisdom": {
    "Name": "Wisdom"
  },
  "Charisma": {
    "Name": "Charisma"
  }
}
BraedonWooding commented 7 years ago

Yes, that is fair; I quite like that and we definitely will at some point want to add more 'attributes' such as code handlers for managing what '+1' means in terms of stats or min/max limits or whatever.

koosemose commented 7 years ago

Small change to GameEvents, Preconditions and ExecutionsActions, as designed, can have multiples of each, so is is now an array of string values.

As of present I have confirmed smooth conversion of 4 of the listed files. Most of the others should be easy, aside from the more complex ones, such as Furniture.

koosemose commented 7 years ago

And Headlines doesn't work as a prototype as you've listed it, but that's partially because it should never have been turned into a prototype, because it's not a prototype, just a list of data... I'll see if I can't massage it into converting into a decent result for now, and possibly later unprototype it (since it really isn't one)...

However, this does bring to mind one concern for my converter (and its alternative function as an editor) related to the format of the data, which may have an impact on modding in the future as well. With the current format we're going for for the json, there is no equivalent to the xml's listTag (as referred to in code), while currently in PP codebase it serves little function other than just junk metadata really, lack of it (or an equivalent to the element tag) there is no way to figure out from that json data what type of object it is (is it a Furniture or Utility for example). This means we have to explicitly tell it what files contain what. This works perfectly fine for our current setup, but for modding this means the modder either has to use standardized name files, or have a way to tell PP what files are what. And additionally it means that if they have a small mod that only adds one or two items of various types (like 1 furniture, a couple GameEvents, and a Tile), they have to have multiple very small files. But wouldn't it be nice (and also maybe make processing things and implementing modding fully easier) if instead of having to tell it what files a given type of prototypes are interested in, we instead can tell it "Any objects in a certain tag, pass to me to let me create prototypes from". This would both allow a modder (or us) to group up different data (say for example you can turn off and on ability to land on planets, so we have a set of prototypes including furnitures, tiles, and gameEvents all conveniently packed into one data file), or to split up a single data file (say down the line we end up with hundreds of furnitures, and for ease of viewing/editing the furniture files, we split it up into all variations of walls in one file, all production machines in another, and a third for decoration, and so on.)

I mention this for two reasons, first, at this stage I can easily include an identifying tag (put all furniture into a "furniture" (or "furnitures" array)), and since once I'm done converting I will need to implement a new version of the prototype mapping system and may be able to include this functionality that may be useful for the future with a minimum of extra effort. (It also has the added benefit for me of making it simpler to transition my converter/editor from reading xml (and saving json) to reading json (and still saving json), without requiring even more steps to figure out what sort of object it is reading.

To summarize, the two proposals are adding a simple indicative array around all prototype data (furniture is all under "furniture", the singular matching the class name being my preference purely for selfish reason of usage in converter/editor), and and altering the prototype loading system so that rather than parsing a given file for each type of prototype, it reads in all data files, and passes each array to be read into/added to the appropriate prototype map.

BraedonWooding commented 7 years ago

With headlines, I do think we should remove it as a prototype and just have a list of data in a raw format like .txt. It'll simplify it and just cut on each line?

I may have a third solution to the second thing. Packaging, this is an idea that I want to introduce in more detail once I do more research. But essentially you have an xml file (or json) that tells the system where each file lies and well I guess there would be no limits on how many for a specific type. Again this would just be read by your converted. And it means that you can just submit a zipped folder with extension .package or something idk, wraps everything up nicely. Though I do actually (really) like the idea of your first one, but I would say maybe the package xml/json stuff is still required for code files cause currently as with the data files the system needs the code files to be called a static term. So perhaps we could mix behaviour, a JSON/XML file for telling what each file correlates to, along with each file requiring a surrounding Furniture: {...} which not only allows you to have multiple 'files' into one but also allows any parsers to read the data easier.

koosemose commented 7 years ago

I may not be explaining it well, but at least with respect to data, it shouldn't need package info (it may need package info to tell where data lives in general, along with other file types such as lua or c# code, but not what data is in what files).

Basically imagine our data file processing taking each data file within the data folder (or maybe with a package info to tell it what is prototype data, if there are other data that uses json format), going through it and seeing a "Furniture" array, it then passes that array to the prototypeMap that declares for "Furniture" and it process it as normal, it then sees a "Utility" array, and does the same for the "Utility" array. It does this for each file in turn, of course, the prototype map and prototype creation code would have to be adapted to ensure it works with being called multiple times (not overwriting prototypes from previous files, unless explicitly told to do so by something having the same name. Once the data reading phase is done, the PrototypeManager still has the same PrototypeMaps that contain data identical to what it is in its current form when created from xml.

Essentially what I'm suggesting isn't mutually exclusive with package info, but also doesn't require it in our current setup. And it would (or should at least, according to how I envision it) also allow package info to be simpler with regards to prototypes, rather than having to say "This file contains these kinds of prototypes", it would only have to say "These files are prototypes" or "All files in this directory are prototypes", and the formatting suggested would allow our system to be intelligent enough to figure out what's what.

BraedonWooding commented 7 years ago

Yeh that's what I meant, I also explained it badly xD. Basically you hit the nail on the head. Just a package that says these files are data these are code files (which we may have a header to tell what function hooks up where) but yeh.

koosemose commented 7 years ago

And here's what I've got for Utility (should mostly apply to Furniture, except furniture also of course has components... which I dread doing)

Most significant difference is in ContextMenuActions, simply because I'm not entirely sure if LuaFunction name is quite analogous to a key such as Type usually is. I can easily see a situation in which we would want the ability to set parameters in ContextMenuActions so that 1 lua function could handle two different (but similar) context options (particularly a boolean parameter), and it also stays more analogous to the structure used in the relevant objects, since ContextMenuActions are always (or at least are in Furniture and Utility) stored in Lists. I think the effective loss of 1 line per ContextMenuAction is worth the gains of being more analogous to the in code data structures, and not potentially getting in the way of doing things that the code side data structures wouldn't prevent us from doing.

{
  "power_cable": {
    "LocalizationCode": "util_power_cable",
    "UnlocalizedDescription": "util_power_cable_desc",
    "TypeTags": [
      "Power"
    ],
    "ContextMenuActions": [
      {
        "LuaFunction": "UtilityContextTest1",
        "LocalizationKey": "Test Context 1",
        "RequireCharacterSelected": false,
        "DevModeOnly": false
      },
      {
        "LuaFunction": "UtilityContextTest2",
        "LocalizationKey": "Test Context 2",
        "RequireCharacterSelected": false,
        "DevModeOnly": false
      }
    ],
    "OrderActions": {
      "Build": {
        "JobTime": 1,
        "Inventory": {
          "copper_wire": 2
        }
      },
      "Deconstruct": {
        "JobTime": 1,
        "Inventory": {
          "copper_wire": 1
        }
      }
    }
  },
  "fluid_pipe": {
    "LocalizationCode": "util_fluid_pipe",
    "UnlocalizedDescription": "util_fluid_pipe_desc",
    "TypeTags": [
      "Fluid"
    ],
    "OrderActions": {
      "Build": {
        "JobTime": 1,
        "Inventory": {
          "steel_plate": 2
        }
      },
      "Deconstruct": {
        "JobTime": 1,
        "Inventory": {
          "steel_plate": 1
        }
      }
    }
  }
}
BraedonWooding commented 7 years ago

The functionality is at minimum enough to warrant it looking ever so slightly uglier for context actions. Good job so far!

koosemose commented 7 years ago

Just realized why I've been struggling to get PerformanceHudTemplate to match your sample... it's invalid duplicate keys.

The closest I can get is:

{
  "Basic": {
    "Components": [
      {
        "Type": "FPSPerformanceComponent"
      }
    ]
  },
  "Extended": {
    "Components": [
      {
        "Type": "FPSPerformanceComponent"
      },
      {
        "Type": "FPSRangePerformanceComponent"
      },
      {
        "Type": "FPSPerformanceComponent",
        "Parameters": {
          "MeasurePeriod": "5",
          "DisplayText": "AVG: ",
          "DisplayColor": "false"
        }
      }
    ]
  },
  "Verbose": {
    "Components": [
      {
        "Type": "FPSPerformanceComponent"
      },
      {
        "Type": "FPSRangePerformanceComponent"
      },
      {
        "Type": "FPSPerformanceComponent",
        "Parameters": {
          "MeasurePeriod": "5",
          "DisplayText": "AVG: ",
          "DisplayColor": "false"
        }
      },
      {
        "Type": "MemoryPerformanceComponent"
      }
    ]
  }
}

Also I just realized you never assign the local variable disableUI to the field disableUI (the setting is read and then dumped, not being used as far as I can see).

If disableUI is no longer needed, and there's not going to be any need of any other ComponentGroup level settings (such as disableUI, or anything else that could conceivably be added in the future) then I could get rid of the Components Property and go straight into an array.

BraedonWooding commented 7 years ago

Yeh I'll remove it when I get time. It was made obsolete and that was an accident I already made changes in local which I'll push sometime soon.

Kjarrigan commented 7 years ago

Just my 2 ct - I'm in favor of using JSON over XML :heart: but I want to suggest to create one file per "thing" instead of 1 XML -> 1 JSON file. So if the stuff grows over time we have thousands of lines. With a file based approch we might get hundreds of files BUT its really easy to find and create a new one.

Pros

Cons

We can workaround the cons in two ways - first would be to not load all files on start at all. Instead load just the files needed. The second way would be to all the files on Compilation to one single file - so we would get the clear maintainable structure of many files and the speed of one big file.

So finally something to look at:

wall/steel_wall.json then just contains its own properties. We could even remove the "steel_wall" and get that by file_name.

{
        "Width": 1,
        "Height": 1,
        "Health": 1000,
        "LinksToNeighbours": "wall",
        "EnclosesRooms": true,
        "DragType": "path"

        "OrderActions": {
                "Build": {
                        "JobTime": 1,
                        "Inventory": {
                                "steel_plate": 5
                        }
                },

                "Deconstruct": {
                        "JobTime": 1,
                        "Inventory": {
                                "steel_plate": 5
                        }
                }
        },

        "Parameters": {
                "thermal_diffusivity": 0.2
        },

        "Components": {
                "Visuals": {
                        "DefaultSpriteName" : {
                                "UseName": "steel_wall_"
                        }
                }
        },

        "LocalisationCode": "furn_steel_wall",
        "UnlocalizedDescription": "furn_steel_wall_desc"
}

Update. We should use the "base-TypeTag" as folder and then we might remove them as well from the json file. Edited my examples appropriately.

koosemose commented 7 years ago

@Kjarrigan A couple more cons to multiple small files:

If the format changes (adding or removing something, particularly something that is required by all or at least likely to be used by most), it is drastically easier to edit one file in multiple places than it is to edit multiple files in a single place.

Similarly it is a lot easier to review multiple objects of a certain prototype if they're all in the same file.

On the base-TypeTag, the problem with that is in some cases (though not so much at present) it may not be obvious what the base typeTag would be, and additionally it would make reading prototypes more complicated, in addition to having to be passed the data file, they would also need to be passed some part of the path, and rather than just parsing the json, would now also have to parse the path, or somewhere along the line it would have to be parsed, which could get very messy since not all prototypes use typeTags so either the ones that do or the ones that don't would have to be special cased. Further, separating where typeTags are defined would be very messy.

However, one upgrade I intend to make (assuming there's not something unforeseen which gets in the way), is to have the json nested in a tag of the prototype type (similarly to what the xml is now), so, for example furniture would be:

[
    {"Furniture": {
        ....
     }
  }
]

or something to that effect. But rather than having that simply be extra organizational cruft that doesn't actually mean anything to the actual data, it would actually signal what type of prototypes are contained within. Then all files within the data folder would be read in (rather than how it currently is where each prototype has to specifically be told what file to read from, and would error if given the wrong file), and the contents of each prototype tag would be passed to the appropriate prototypeMap to have each of those prototypes added in.

This would allow us to expand in both directions, either splitting up a prototype's file when it becomes too large and unwieldy (and perhaps separating them into some sort of logical categories, but only as a matter of convenience, rather than anything relevant to the data), or to pack multiple types of prototypes into a single file (for example, either if we or a mod wished to introduce another "tier" to the game, we could have all the prototypes using some new unobtanium resource, along with the resource itself, added in in a single file). Or if a mod really wanted to break things down it would allow said mod to have individual items per file (though each of those would be a layer deeper than they would be without the "prototype type tag".

And finally on the loading on the fly, it wouldn't really be feasible (or at least not improve things), as a fair bit of our prototypes (roombehaviors, tileTypes, furniture, and utilities, and those being likely to be the largest files) would all need to be read when the construction menu opens. And further we'd still need to do some sort of partial load to know what items are available to be loaded. All in all, aside from that issue, I'd rather have all of our slow loading happen at one time, when we can make the user aware long loading is happening, and the user will be more likely to expect and accept loading times.

koosemose commented 7 years ago

@BraedonWooding Ok, wasn't sure if that was the case or if it was an oversight, I'll go ahead and remove it from the converted files. However, having thought about it, I think, barring you having a convincing argument that we would never have any reason whatsoever to have further properties at the ComponentGroup level, I will go ahead and leave the "Components" container, since it's only one extra layer of depth and adds a little more future-proof sturdiness.

Kjarrigan commented 7 years ago

If the format changes (adding or removing something, particularly something that is required by all or at least likely to be used by most), it is drastically easier to edit one file in multiple places than it is to edit multiple files in a single place.

Thats no real con for me. Every mediocre editor/ide today has the ability to search & replace across multiple files, the better ones even with regular expressions.

But the other ones are valid objections and if you already plan some kind of splitting mechanism I guess I'm fine although I got another idea while cutting the grass in my yard. I could write a (more or less) simple command line tool which splits the json to something I can work with (many directories and a single file per thing) and also can reassemble the whole stuff, because I still think the ability to easily compare two objects and get an overview of what things exist is really helpfull in development. This way we both could get what we want :smile: I'll sleep over it and see what I can come up with. If anything usefull arises I'll share it with you :+1:

BraedonWooding commented 7 years ago

Also another problem is that you have to call the file the 'type' of the furniture or whatever, it just adds more ambiguity and confusion just for the pro of one less 'variable'. Though your solution may fix any problems so we'll see :D.

koosemose commented 7 years ago

Yeah, assuming I am able to implement everything I intend to, my solution should allow a structure as Kjarrigan suggested, while not requiring it. The key aspect is divorcing the data from anything relating to file metainformation (even further than it is currently, as presently the file name effectively determines what sort of prototypes it contains), while a modder will be free to have directory structures broken into categories, and filenames matching the prototype, or whatever else they may choose, this will be null information, meaningless to the system, only mattering to a modder who is investigating the files, which does of course mean that should a modder wish to maintain a directory structure such as this, they will be required to ensure the data matches up, as it is the data itself which matters.

While I like to trim excessively long things where possible (if possible any one "thing" should fit into one reasonably sized screen at a reasonable font size), there is a point of balance, at some place this complexity of data needs to exist, and the more different places the data can exist, the more complexity we have to have elsewhere to deal with it, and at a certain point there is the risk of it taking a greater amount of complexity in one place to create simplicity elsewhere.

I will also note to @Kjarrigan since you're new here and not familiar with my ways, my comments and what not tend towards long and meandering, for various reasons, among them being that I am often typing them as I ponder a related (or sometimes completely unrelated) problem/solution, so new ideas develop or unrelated thoughts sometimes affect things, leading to the wandering nature. Also, as I haven't seen you in there, feel free to drop by the official Discord chat (linked in the Readme), a lot of informal discussions on things will happen there and they can sometimes benefit from the faster nature of chat, over the slower nature of post and reply.

koosemose commented 7 years ago

Ok, so here's what I have for various components, I think maybe some of them could be made a little more succinct, and perhaps a little more clear while we're at it... but I'm not completely sure on all of them. Of particular note is ProductionChain in Workshop, I think it could benefit from having the chain Name as a key, and have the Inventory in Input match more closely with inventory elsewhere (though it will actually make things a bit longer, but I like the consistency). Another area of note are the Parameter Definitions, which feels overly long to me, but I'm not entirely sure how else to format it.

    "Visuals": {
        "DefaultSpriteName": {
          "Value": "steel_wall_"
        }
      }
      "Visuals": {
        "DefaultSpriteName": {
          "Value": "door_horizontal_0"
        },
        "SpriteName": {
          "FromFunction": "Door_GetSpriteName"
        }
      }
      "Visuals": {
        "UseAnimation": [
          {
            "Name": "idle",
            "RunConditions": {
              "ParamConditions": [
                {
                  "ParameterName": "pow_is_running",
                  "Condition": "IsFalse"
                }
              ]
            }
          },
          {
            "Name": "running",
            "RunConditions": {
              "ParamConditions": [
                {
                  "ParameterName": "pow_is_running",
                  "Condition": "IsTrue"
                }
              ]
            }
          }
        ]
      }
    "PowerConnection": {
        "ParameterDefinitions": {
          "CurrentAcumulatorCharge": {
            "ParameterName": "pow_accumulator_charge"
          },
          "CurrentAcumulatorChargeIndex": {
            "ParameterName": "pow_accumulator_index"
          },
          "IsRunning": {
            "ParameterName": "pow_is_running"
          },
          "IsConnected": {
            "ParameterName": "pow_is_connected"
          },
          "Efficiency": {
            "ParameterName": "pow_efficiency"
          },
          "OutputNeeded": {
            "ParameterName": "pow_out_needed"
          }
        },
        "Requires": {
          "Rate": 1.0,
          "CanUseVariableEfficiency": true
        }
      }
      "FluidConnection": {
        "ParameterDefinitions": {
          "CurrentStoredAmount": {
            "ParameterName": "fluid_stored_amount"
          },
          "CurrentStorageIndex": {
            "ParameterName": "fluid_storage_index"
          },
          "IsRunning": {
            "ParameterName": "water_is_running"
          }
        },
        "Requires": {
          "Rate": 1.0
        },
        "FluidType": "water"
      }
      "GasConnection": {
        "Provides": [
          {
            "Gas": "O2",
            "Rate": 0.032,
            "MaxLimit": 0.2
          },
          {
            "Gas": "N2",
            "Rate": 0.128,
            "MaxLimit": 0.8
          }
        ],
        "Efficiency": {
          "FromParameter": "pow_efficiency"
        }
      }
    "Components": {
      "PowerConnection": {
        "Requires": {
          "Rate": 1.0
        }
      }
"Workshop": {
        "ParameterDefinitions": {
          "CurrentProcessingTime": {
            "ParameterName": "cur_processing_time"
          },
          "MaxProcessingTime": {
            "ParameterName": "max_processing_time"
          },
          "InputProcessed": {
            "ParameterName": "cur_processed_inv"
          },
          "IsRunning": {
            "ParameterName": "workshop_is_running"
          },
          "CurrentProductionChainName": {
            "ParameterName": "cur_production_chain"
          },
          "InputHaulingJobsCount": {
            "ParameterName": "input_hauling_job_count"
          },
          "HasAllNeededInputInventory": {
            "ParameterName": "has_all_needed_input_inv"
          }
        },
        "ProductionChain": [
          {
            "Name": "Ice melting",
            "ProcessingTime": 60.0,
            "Input": [
              {
                "ObjectType": "ice",
                "Amount": 1,
                "HasHopper": true
              }
            ]
          }
        ]
      }

Proposed change for Production chain (done by hand, so it may contain errors), along with needless depth removed resulting from class structure:

{
    "ProductionChain": {
         "Ice melting":  {
            "ProcessingTime": 60.0,
            "Input": 
            {
                "HasHopper": true,
                "Inventory": {
                    "ice": 1
                }
            }
      }
    }
}
BraedonWooding commented 7 years ago

They look all good, I think that while parameter definitions is long it isn't messy and is quite readable, though if it can be shortened to just something like CurrentAcumulatorCharge": "pow_accumulator_charge". Then that could possibly increase its readability and shorten it? That is of course if it doesn't require more attributes (which it may).

I do like the new change for production chain, but it seems from the original that for each input it has the opportunity to be a hopper, as in you could have two inputs one of ice and the other cryofuel and have the ice 'HasHopper' set to true where as the cryofuel set the 'HasHopper' to false.

koosemose commented 7 years ago

Yeah as far as I can tell from looking at the code ParameterDefinitions basically functions like a dictionary, X parameter corresponds to Y field.

And unfortunately you're right on the Input, I forgot the example of the power cell press, which requires both uranium and steel, and was blindly cutting depth that worked for the particular example I pulled out... I'll give it a think and see if I can come up with a way to restructure it... Of course I dislike the whole hopper concept in general, mostly because it's a side effect of the whole "sit the inventory in an actual tile to feed it into the machine" thing, which I feel is just ugly and makes a need to have absurdly large machines. I'd like to restructure the whole workshop to just have all inventory be internal rather than sitting on it, but that would require it having a UI to show what its current inventory is.

koosemose commented 7 years ago

Alright, I've got Parameter Definitions cleaned up. Was pondering if ParameterConditions could benefit from the same general treatment, without losing clarity (though I think these generally will need documentation anyways, as it isn't entirely clear what exactly is going on even in the longer form).

Currently it is:

          "ParamConditions": [
            {
              "ParameterName": "workshop_is_running",
              "Condition": "IsTrue"
            }
          ]

Alternative:

          "ParamConditions": {
              "workshop_is_running": "IsTrue"
            }
BraedonWooding commented 7 years ago

I think that is fine since I will be posting documentation on all the JSON documents anyway (and we can have comments due to json.net).

SidneyArmitage commented 7 years ago

can we have a method in IPrototype called: public void ReadJSONPrototype(JProperty token)

BraedonWooding commented 7 years ago

Yes, yes, yes (haha).

BraedonWooding commented 6 years ago

Closing because done