Prototype code cleanup #111
Conversation
Currently fluids and recipes for fluids are mixed together under the "fluid" prototype folder. This is part of a much wider problem that causes load order issues on several levels and likely leads to quite a few hacks in data-updates.lua - Which we really should try to avoid, as it makes understanding and extending the code base much harder (in my case, simply trying to update icons for a couple of recipes). We already have the structures that we need to make things better established, so all we need to do is adhere to them :) This patch cleans up the fluid prototype section by moving all recipes there into its own file in the "recipes" folder. These changes shouldn't effect existing mod behaviour.
Moving the recipes out of the fluid prototypes had the effect that we have now a whole bunch of files who each contain only the definition for a singular fluid. This is annoying to navigate, maintain and extend, so we move everything into a singular file to make everyones life a bit easier. Does not change mod behaviour.
The "buildings" prototype folder has the same problem as "fluids" had, expect worse. Items, recipes and entities are mixed together here. This is part of a much wider problem that causes load order issues on several levels and likely leads to quite a few hacks in data-updates.lua - We already have the structures that we need to make things better established, so all we need to do is adhere to them :) This patch cleans up the building prototype section by moving all items there into its own file in the "items" folder. These changes shouldn't effect existing mod behaviour. Attention: The item for "pulp-mill" was left in its place due to startup problems. It looks like the whole item/recipe/entity shouldn't exist anymore. I'm unsure how to proceed here, so i left it as it is for now.
This patch cleans up the building prototype section by moving all RECIPE entries there into its own files in the "recipes" folder. These changes shouldn't effect existing mod behaviour.
This change loads FLUID and ITEM prototypes first, this way we can refer to them correctly with functions in later prototype stages without constantly running into "nil value" errors.
|
The pipeline failed first. Was able to reproduce the error. Was able to fix it too. I was wondering about the alienlife buildings being in hightech, The problem only appeared because the ITEM prototypes for mk02/mk03/mk04 were now loaded due to being in a different place, referring an ENTITY that does not exist. But why weren't mk02/mk03/mk04 entities loaded in the first place? Could one of the maintainers please take a closer look at this? |
|
Hi! While I appreciate all this work, I'm going to have to deny these changes. The current system provides a file structure that's much easier to navigate than one large file. Additionally large refactors are very difficult to review, and revert if anything goes wrong. In the future I'd suggest sectioning large changes into smaller components so you can get feedback much sooner before putting in too much effort. Thank you for all your effort. |
|
Hmm, yeah, that is a bit much at once, maayybe i got a bit carried away... 😅 This is not a monolithic block of commits however, they work independent of each other (with one exception). So i can split them up into several merge requests to make things easier for you to review, that is no issue at all 😄 And the file system structure is the whole reason for my efforts here! Currently ITEM, FLUID and RECIPE entries are strewn all over the place in the prototype folder, RECIPE's are parked in the fluids folder, ITEMS and RECIPES in the buildings folder and so on. This makes it impossible to navigate the prototype folders without an IDE with a folder-wide search function, as half of the stuff isn't where it is supposed to be 😑 It also makes it impossible to create better icons using py.composite_icon(), And i absolutely do not want to solve that by adding further recipes to that 3600+ line recipes.lua monster of a file (and i didn't, i created additional files). I could try to sort and categorize that one into smaller files in the recipe folder too in the near future. |
|
There is steady work to merge the mk01-mk04s into single files while preserving the git history (example), but this kind of mass re-sort isn't really compatible with the current codebase and workflow (this latter part being my opinion, other maintainers are welcome to chime in). I also want to mention that you will continue to run into load-order issues as many things are conditional on which py mods are present. A good example of this is your experience with the moondrops, which have mk02-mk04 tiers conditional on pyalienlife being present. While it isn't consistent (and this is a pain point), the bulk of the inter-mod compatibility work is done by the later mod updating the prototypes of the earlier one. |
|
I've added a small but important pull request (#112) that contains a part of this pull request. If that one gets a green light, i'll strip this pull request here down to the FLUID changes only and remove the Draft status to try again - So please don't close it right now, i'd like to keep the comments here for context as for why i did these changes. |
|
@oorzkws: Hmmm. I think there are different viewpoints on the prototype folder structure. Combining RECIPE/ITEM/FLUID together in a file goes - in my understanding/my viewpoint! - against the prototype folder structure - Which has one folder for ITEMs, one folder for FLUIDs, one for RECIPEs and so on. Without such a structure i'd have to, as i said, park most of the icon generator calls in data-updates.lua or data-fixes.lua, as everything else either doesn't work (the referred FLUID/ITEM doesn't exist yet) or is prone to breaking randomly with updates to the prototypes. Worst thing is this isn't exclusive to icon generator stuff, other future functions that reference other things will run into the same problem. And having the prototypes load in a well defined order (e.g. first FLUIDs, then ITEMs, then RECIPEs) is - again, my personal viewpoint - more important than having everything in one file, as the latter can cause so many issues with unpredictable load orders. Most important right now however: There are multiple ways to interpret how the prototype folder is supposed to look, and that's really really bad. I'd heavily encourage a discussion about the pro/cons of the different views, then pick one, then describe the way it is supposed to be structured in a README.md within the prototype folder. |
|
We'll be reviewing that new PR. As for your proposed fluid changes fluids should stay as they are, unifying them into one file would decrease readability. Others are free to chime in but I believe even just the fluid changes from this PR will be denied. |
|
I agree with the other maintainers here. While it might not look sane to an outside user, working within this codebase you tend to see things less as 'items' or 'fluids' or 'recipes' and more as just objects. Objects that have a name that generally tends to align with where you can find them, regardless of what kind they are. They also don't have a reason to be grouped by item or by fluid, as generally, any item or fluid can have multiple connections, meta or not, to other objects, which is much easier to handle when those other objects are all locally defined. |
|
As i said, we have very different viewpoints 😉 And now the really fun part: I see objects too! ITEMs, FLUIDS, RECIPEs, ... - Back to the topic: There have already multiple maintainers spoken for the current way, so i don't think the structure is going to change - I don't like that very much, but oh well. Please do me (and yourselves) the favor and document they way you want the prototype folder to be structured in a README.md or so that everybody can read. Right now it is very unclear what way it is supposed to look (as a lot of stuff is still sorted by object type and not by name), and that's harmful in so many ways. I wouldn't have wasted time for example if such a document was available somewhere, and it helps you to define the way you want it better. Also helps other newcomers to understand the structure better, which is also a boon in my eyes. Doesn't have to be fancy or multiple pages long, just a folder structure example with a couple sentences which describe what you've said to me essentially. To my problems that caused this discussion - I see that there is an update-folder present under prototypes. That could be a way for me to park my icon related recipe changes, but then there's stuff in data-updates.lua that isn't there (e.g. the big "py.allow_productivity" boilerplate), so i'm not sure if that is something you want, or something you want to phase out. Please advice (and document!) 😉 |
protocol-1903
left a comment
protocol-1903
left a comment
There was a problem hiding this comment.
most of these issues will be fixed with linearization
protocol-1903
left a comment
protocol-1903
left a comment
There was a problem hiding this comment.
hit the wrong button smh
4 participants
This pull request is basically the result of me going to the ground of
"Why the hell is py.composite_icon() not finding items or fluids when i use it in recipes?",
which is a problem that is going on my nerves for a few days now.
Currently ITEM, FLUID and RECIPE entries are strewn all over the prototype folders,
which makes the load order effectively unpredictable when trying to refer to FLUID or ITEM
entries from a RECIPE, e.g. with the mentioned py.composite_icon() function.
There is already a structure established for sorting these things properly,
so i did some cut & paste work sorting things where they belong, as well as changing
the load order by loading ITEM and FLUID prototypes before the RECIPE ones.
It's a lot of changes, but they shouldn't affect the mod at all (did some testing too, didn't notice anything). It will also make referring to things more predictable, which is a big thing not only for icon based functions, but for other stuff too (perhaps resulting in fewer data-fixes hacks?).
I'd also like to look over the other pyanodon mods and give them the same treatment if necessary, but i want to make sure these changes are actually wanted before putting more effort into it (i hope they are) ;b