Integrate sound_api submodule, and make recipes configurable per game, but fallback to Minetest Game items otherwise.

[MysticTempest]

Basically, title.
This is to fix: #4

Respectfully, adding a submodule for sounds; for 3 nodes is a bit extreme.
While the goal of making 'basic_materials' game agnostic is a good idea. There are still 57 instances of "default:" in the "crafts.lua" file.

So, I've integrated the sound_api, and added a gameconfig section for recipe ingredients as well.
I added recipe support for MineClone2, and fallback to Minetest Game if MCL2 is not present.
But, this can obviously be expanded in the future.

[wsor4035]

integrating the submodule complete defeats the point

[wsor4035]

integrating the submodule complete defeats the point

to follow this up, just doing a copy paste is about literally the worse thing you could as its the most terrible way to update it if/when upstream makes changes/add support for more mods. better ways would be optionally depending on the sound api mod wrapper via https://github.com/mt-mods/sound_api#option-2-agnostically-depend, using a metatable to handle it if the submodule isnt present as found in https://github.com/mt-mods/basic_materials/pull/5/files#diff-7387f793c5e46539c37b6b0b0847d89eaa62f4533aae2c54a0cb67ca6e95fddcR6-R22, or including a shell script (optionally in github actions) that pulls the latest init.lua on a commit

[wsor4035]

i approved the workflows for this, it appears you did not check luacheck as you are introducing over 160 warnings (from 0) in this PR

EDIT: to fix this, use a local config variable, and stuff your values in there, and return it.

[S-S-X]

integrating the submodule complete defeats the point

This is not exactly true, there's a lot of different approaches for dependency management and for example dependabot supports mulltiple (including fully customized). Just about any custom automated dependency management can be integrated fairly easily under just about any platform utilizing git somehow.
Basically switching from submodule to integrated source files does not change outcome in any way, it just changes how dependency managment is done and what are requirements for development workflow.

There's still multiple reasons to not do this, it is not that management would be harder or worse but more about pros/cons and if additional features would really bring in any real improvements.

Few actual reasons why source files should not be included directly in repository:

• It will be a lot harder to understand what is actually going on under the hood, it'll look like a piece of code that could and should be updated directly while actually updates should be done for library and then library updated for project.
• Any direct updates will make it harder to pull new features and bug fixes from main library.
• Minetest mods in general do not have any official guidelines for library deployment and dependency management, switching into anything that is not immediately obvious will cause confusion.

Basically only clean solutions I can see here assuming that library will be used as a library:

• Minetest mod dependency which is standard way and immediately obvious no matter how mod code is delivered. Packages are clearly isolated and distinguished from each other anywhere (be it git, contentdb, your mt mods directory, anything basically).
• git submodule which basically offers lightweight dependency management. Not immediately obvious before looking at git repository but will be if going to participate in development. (not visible through contentdb or on your average mt mods directory, real structure is visible for git users).

Currently sound_api itself supports both ways.
Minetest mod dependency system allows easy mod updates anywhere but is not aware of mod / "library" versions at all.
Submodule "dependency system" allows some mod version tracking, can emulate library version system.

There's also multiple reasons to not bring in full blown dependency management but afaik so far nobody has seriously suggested anything like that.

[S-S-X]

Respectfully, adding a submodule for sounds; for 3 nodes is a bit extreme.

Sound library afaik is supposed to allow supporting all games without including same support code into every individual mod, so while it is currently just 3 nodes and few games situation could very well be 3 mods with 3 nodes each and support for 4 games.
When support for game 5 gets added? It is possible that someone might remember to update this mod... or maybe not.
(note: mt-mods/xcompat#8)

While the goal of making 'basic_materials' game agnostic is a good idea. There are still 57 instances of "default:" in the "crafts.lua" file.

Craft registration actually does not have to be checked, you just wont get recipes if items are missing. Also mt-mods/home_workshop_modpack#5 (comment)

[wsor4035]

unless im missing something, luacheck is missing the fact that bm is global

[MysticTempest]

i approved the workflows for this, it appears you did not check luacheck as you are introducing over 160 warnings (from 0) in this PR

EDIT: to fix this, use a local config variable, and stuff your values in there, and return it.

unless im missing something, luacheck is missing the fact that bm is global

Fixed the luacheck errors. Updated .luacheckrc

Respectfully, adding a submodule for sounds; for 3 nodes is a bit extreme.

Sound library afaik is supposed to allow supporting all games without including same support code into every individual mod, so while it is currently just 3 nodes and few games situation could very well be 3 mods with 3 nodes each and support for 4 games. When support for game 5 gets added? It is possible that someone might remember to update this mod... or maybe not.

As for the sound_api integration. I somewhat agree.
Honestly, if it were purely up to me. I'd just add 2 sound files. The sounds are for 3 nodes that originate here in basic_materials.
There's really no reason to have such an involved system for these nodes.

But, I figured since the original intent of the previous PRs was to seamlessly merge with games. I'd fix the hardcoded recipes.
And then, I'd just merge in the external submodule to keep things mostly unchanged, but self-contained.
So users don't have to rely on yet another external dependency.

Additionally, you mentioned adding support for hypothetical new games. But, currently with the submodule; you'd have to remember to update 2 mods: 'basic_materials' first, and then update 'sound_api'.
Here, it's all in one config.

While the goal of making 'basic_materials' game agnostic is a good idea. There are still 57 instances of "default:" in the "crafts.lua" file.

Craft registration actually does not have to be checked, you just wont get recipes if items are missing. Also mt-mods/home_workshop_modpack#5 (comment)

How are you supposed to use basic_materials in other games if the items are only craftable when using the "default:" mod though?
I figured the point was to have the items always be craftable so everyone can depend on them.

[wsor4035]

ah, thats why luacheck is not complaining, there is no valid reason to make bm a global, make it a local and return it

EDIT: even if it there was a point, it should not be listed twice

[wsor4035]

Additionally, you mentioned adding support for hypothetical new games. But, currently with the submodule; you'd have to remember to update 2 mods: 'basic_materials' first, and then update 'sound_api'.
Here, it's all in one config.

This is incorrect, you would add support in the sound_api first, and then dependabot would make a PR to bump the submodule which is just a click to merge (assuming its a simple sound add)

EDIT: if the optional mod dependency route is used, then all that would need to be done is the sound api mod updated by the user to add support for a new game

[S-S-X]

bm should not be global, if global access is needed then sub table under basic_materials should be used.

---

Also including reply to another question here, unrelated to above bm variable change request:

While the goal of making 'basic_materials' game agnostic is a good idea. There are still 57 instances of "default:" in the "crafts.lua" file.

Craft registration actually does not have to be checked, you just wont get recipes if items are missing. Also mt-mods/home_workshop_modpack#5 (comment)

How are you supposed to use basic_materials in other games if the items are only craftable when using the "default:" mod though?
I figured the point was to have the items always be craftable so everyone can depend on them.

Depending on exact situation either similar way how you implemented it, through separate mod to handle recipes or leave it for game. For current situation would have probably done similar way.

Basically comment was simply just note that comments "While the goal of making 'basic_materials' game agnostic is a good idea." and "There are still 57 instances of "default:" in the "crafts.lua" file" have nothing to do with each other or with mod itself being usable in any other game, all internal logic is available and all items are available anyway.

[S-S-X]

Everything below this line is almost sure bad idea, you already mentioned "support for new hypothetical game" but it is not just hypothetical as there's open issue in sound_api repo for that.

Duplicating code here means harder to maintain and more places for mistakes, using library means that you'll only need to update library which is then used by multiple different projects. Fixes and feature updates can be used everywhere without rewriting code for every mod that is using library.

If submodule requirement causes problems then there's other solutions for that but for these issues it would be better to open separate issues and pull request. Duplicating code and dropping central repository unnecessarily like that should not be accepted without very good reason and so far there does not seem to be actual reasons provided.

I'm not sure if you've changed something while copying code from sound api but if you did then those changes should almost sure go into sound api itself.

Honestly, if it were purely up to me. I'd just add 2 sound files. The sounds are for 3 nodes that originate here in basic_materials.
There's really no reason to have such an involved system for these nodes.

You probably also understand that doing it this way would take away game specific experience while increase media footprint, especially if every mod that needs just few sounds would follow this logic?

---

It is true that sound api as a submodule has potential for additional problems but ignoring upsides of library and purpose of library wont bring better solutions. Basically some discussion is needed if mentioning that "There's really no reason to have such an involved system for these nodes." and at the same time copy pasting a lot of code that duplicates maintenance requirements effectively creating two such an involved systems.

If you see some problem in sound api then best to open separate issue for sound api.
If you think sound api is not useful enough for basic materials then open separate issue to discuss that, prefereably include some reasoning why something else would be better and/or why library is worse.

[S-S-X]

I would suggest dividing this pull request into 2 separate pull requests:

• One for (or issue for discussion): Integrate sound_api submodule
• Another for: make recipes configurable per game, but fallback to Minetest Game items otherwise.

Btw, configuration should not happen through editing lua files. Game / mod detection and item selection based on that would be best.
If for some reason it seems that actual configuration could be useful then maybe better to just add per world configuration files.