TumeniNodes / stoneworks

Adds various arches and thin wall systems to Minetest_Game
Other
2 stars 2 forks source link

add recipes #2

Closed ThorfinnS closed 5 years ago

ThorfinnS commented 5 years ago

Plus modify code to use function call to define nodes and recipes.

TumeniNodes commented 5 years ago

lots of work, thank you for the time an interest. However, I am about to make more work for you...

It is best to do one PR at a time. i.e. adding the recipes should be one PR Adding new nodes should be a separate PR as should PRs to add support for other mods. Changes to existing nodes should be sep PRS unless explained why it is needed within another PR.

Also, I think it best to leave other mods to add the support for this mod, due to the fact this mod already adds so many individual nodes.

This is just too much for a single PR If these can be separated and submitted one at a time, I will review them Sorry for the extra work but, it is necessary. Thank you again

ThorfinnS commented 5 years ago

Doesn't each commit function as a separate PR? So you could accept recipes but nothing else, or everything but the recipes, or whichever you want?

ThorfinnS commented 5 years ago

For instance, if you wanted to review JUST the recipes, you have to look only at 88013c5. If you reject the recipes, so be it. No point in any of the rest. That's the beauty of version control, right?

TumeniNodes commented 5 years ago

Just pulling in your first commit https://github.com/TumeniNodes/stoneworks/commit/88013c5d9ccfc28ec0fa8b14b6d2d58a42ea52f2, which is supposed to add recipes, also adds several new nodes. One being the "mini" which I have no idea what it's use is, not for lack of trying to figure it out. It should only contain the additional new code to add recipes, not to add new nodes (that should be a separate PR

That is one of the issues I am referring to. I do appreciate all of this work, but I simply cannot accept any of it the way this PR is structured, sorry. The way this is done, it adds about 7/8 new pages of inventory.

As I stated, if you can break it all down and submit it as one PR for each set of changes, I will more than gladly review/test, and merge if possible.

I am also not going to accept a PR to add support for other mods (such as cool trees), those mods are able to add support for this mod if they like, but as I said... this mod already adds a lot to inventory and I have already had some mention this is the only reason they are unable to add it to their mods list. And not every user is a coder, who knows anything about commenting out materials they will not use.

Again, I fully appreciate the time and work put into this, but it all needs to be broken down into individual PRs please.

ThorfinnS commented 5 years ago

The mini has no real use, apart from being the basic building block for all the recipes for the nodes you defined. Way too many recipes are already taken for wood and stone to make anything consistent without an intermediate block. The lower nodes are easier to use building blocks. Kind of like building a computer using chips rather than building one using entirely transistors, resistors, capacitors, etc. Neither set of nodes serves much useful purpose as a separate PR, their only real use is in recipes for the rest of the stuff. I really wanted to keep the recipes separate, but since you pass in "recipeitem" it seemed pretty obvious you intended to put recipes inside the scope of stoneworks.register_arches and stoneworks.register_thin_walls. All I was doing was keeping things consistent with the scope you seemed to want rather than redoing much of your code.

It's only 6 new nodes in addition to your current 35, per material, but since your mod supports 40 materials, I get that it adds up. Another 240 on top of the existing 1500 or so.

Mod support only adds those nodes if you have those mods active, but I get your point. Much better to have those other mods call stoneworks.register_all similar to the way they call stairsplus.register_all or whatever it's called.

But I get that it's a lot to review. And maybe not your vision for this mod anyway. I won't feel hurt if you choose to accept nothing. I hope you don't mind if I leave my fork out there.

Cheers!

ThorfinnS commented 5 years ago

PS If I were you, I'd just close this PR. Otherwise when I add darkage for Nordal you are going to get another notification.

TumeniNodes commented 5 years ago

Yes, I figured out what the mini nodes were after I commented on them. The hard part about that is, now they are also added into inventory. Someone had suggested adding a milling type machine or to add support for moreblocks in the past... but I am lazy and realize this sounds harsh but, creating recipes does not interest me in any way, as I use MTG strictly to build and do really weird experiments with code.

I feel bad about all the work you did but, I am glad at the same time you will produce a fork for people and probably expand on it (this is the overall spirit of this community)

To be honest, this mod was something I never really intended to put out, it was just a mash of bits I created as I needed during a build (which is how most of my mods come to be) They are rough and crude, and typically done on the fly as I am quite lazy and impatient, and a creative person who can mess with things I do not understand, just enough to make them do what I want. And quite often, other members are nice enough to lend a big hand when I can't. I just share them in case others may enjoy them and get use from them... or even to build them into something bigger and better.

Thank you again for even taking the time, I still appreciate it.

TumeniNodes commented 5 years ago

closing to see a much better and more complete version of this mod come to life