S-S-X
commented
3 years ago
- [ ] Make CNC depend on
technic(currently optional, which doesn't make sense)
CNC does not require technic and it has been used on some servers without technic.
Closed OgelGames closed 3 years ago
S-S-X
commented
3 years ago
- [ ] Make CNC depend on
technic(currently optional, which doesn't make sense)
CNC does not require technic and it has been used on some servers without technic.
OgelGames
commented
3 years ago Ah, I didn't realize that it was functional without technic.
Still has one thing to fix though, when technic_worldgen doesn't exist:
ERROR[Main]: generateImage(): Could not load image "technic_zinc_block.png" while building texture; Creating a dummy image
ERROR[Main]: generateImage(): Could not load image "technic_zinc_block.png" while building texture; Creating a dummy image
ERROR[Main]: generateImage(): Could not load image "technic_cast_iron_block.png" while building texture; Creating a dummy image
ERROR[Main]: generateImage(): Could not load image "technic_cast_iron_block.png" while building texture; Creating a dummy imageI think it would make sense to divide materials.lua into multiple files possibly moving files to materials/default.lua, materials/technic.lua, materials/ethereal.lua and materials/moreblocks.lua.
That also allows having default materials separated which probably helps with #214
There's even minetest.get_modpath("technic") but some materials are registered outside of that block...
S-S-X
commented
3 years ago It also seems like it would be easy to make also dependency to default for wrench to be optional.
That would be another thing which would also help with #214
Just make registered nodes table empty https://github.com/mt-mods/technic/blob/b2f456d3ff083efed87b67d718d8b0d09145ab3e/wrench/support.lua#L26
And use API method to register all those nodes if default is loaded https://github.com/mt-mods/technic/blob/b2f456d3ff083efed87b67d718d8b0d09145ab3e/wrench/support.lua#L67-L71
github-actions[bot]
commented
3 years ago S-S-X
commented
3 years ago Seems like failure report missing for CNC:
19 successes / 3 failures / 0 errors / 0 pending : 0.079455 seconds
Failure → spec/api_spec.lua @ 28
CNC API Machine control returns product item string
spec/api_spec.lua:30: Expected objects to be the same.
Passed in:
(nil)
Expected:
(string) 'default:stone_technic_cnc_stick 8'
Failure → spec/api_spec.lua @ 62
CNC API Machine control manufactures products
spec/api_spec.lua:72: Expected objects to be the same.
Passed in:
(boolean) false
Expected:
(boolean) true
Failure → spec/interaction_spec.lua @ 107
CNC formspec interaction manufactures products
spec/interaction_spec.lua:120: Expected objects to be equal.
Passed in:
(string) ''
Expected:
(string) 'default:stone_technic_cnc_sphere'That's probably because tests expects that mods are actually available.
S-S-X
commented
3 years ago To fix tests when removing default from dependencies single line is needed
mineunit:set_modpath("default", "spec/fixtures")anywhere in file https://github.com/mt-mods/technic/blob/b2f456d3ff083efed87b67d718d8b0d09145ab3e/technic_cnc/spec/fixtures/default.lua#L1-L9
I'll try to remember fix these things with tests if not fixed here, right that it makes sense to not change that just yet because of crafting recipes and other stuff like that.
S-S-X
commented
3 years ago Reduce optional dependencies of technic_worldgen using minetest.register_on_mods_loaded()
This might not be wise thing to do because every unnecessary registration check during register_on_mods_loaded makes system as a whole lot more complicated and is potential source of problems in future (not as bad but similar to using minetest.after to "fix" bugs).
For many things optional dependencies are good to have if there's real feature dependency, not only because it will allow Minetest to do suggestions for mods and ContentDB to show things that are meant to work together but also because it makes loading order clear and visible.
Is there some real reason (mod conflict) that requires dropping optional dependencies?
OgelGames
commented
3 years ago Is there some real reason (mod conflict) that requires dropping optional dependencies?
No, the main reason is because they aren't dependencies at all, they're just there so some items can be renamed.
This is the code I was thinking of changing, using register_on_mods_loaded() to make it less "hacky", while also eliminating the need to include the mods as optional dependencies: https://github.com/mt-mods/technic/blob/2add4287ba8828f9882da90a93e1dbf0acd2b408/technic_worldgen/crafts.lua#L146-L194
And also this: https://github.com/mt-mods/technic/blob/2add4287ba8828f9882da90a93e1dbf0acd2b408/technic_worldgen/nodes.lua#L156-L196
Yes it doesn't need to be changed, but I think it should be, and also be moved to a separate file.
S-S-X
commented
3 years ago No, the main reason is because they aren't dependencies at all, they're just there so some items can be renamed.
In that case it could make sense, only reason to keep optional dependencies in mod.conf would be to make it visible that mod does something with another mod. Probably not that important for small changes done by technic_worldgen.
I did take a look at the code and one thing made me wonder if it works or breaks things: Translations provided by other mods where technic_worldgen rewrites part of description, will translations actually work (for full description) after doing that or will it break translations?
Anyway, that cannot be really fixed here if translations wont work after editing descriptions but I think it is something that should be considered and tested.
If modified translations do break (or if translations are not tested) then would be good to open separate issue about that and let it be broken for now. If translations do work even after modifications then I guess all good an no need to do anything.
OgelGames
commented
3 years ago Yeah, I think translations were broken before, and they will still be broken after these changes. Probably still a good idea to open an issue about it, or at least mention it in #104
OgelGames
commented
3 years ago Tidied up the commits, so the different changes can be merged as different commits. (rebase merge)
I don't think there is anything else that needs to be done in this PR, so it should be ready for merging (after an approval is added).
github-actions[bot]
commented
3 years ago
depends.txtfilestechnic_worldgenusingminetest.register_on_mods_loaded()Make CNC depend ontechnic(currently optional, which doesn't make sense)Not including #214 in this PR because that will be a much bigger change, as opposed these small changes and fixes.