search

minetest-mods / moreblocks

More Blocks
https://content.minetest.net/packages/Calinou/moreblocks/
zlib License
45 stars 67 forks source link

partial rewrite and major bugfix #191

Closed fluxionary closed 1 year ago

fluxionary commented 2 years ago

It's become time for me to focus on cleaning up moreblocks and fixing a bunch of outstanding, (long-standing!) issues.

the major motivation is to make it easier to add "cut" shapes of various nodes without blowing up the node count too much, because the 32767 node id limit is a real problem. the minor motivation is that there's currently a lot of ways to end up losing material (or even generating it ex nihilo!) when using the circular saw.

I want to invite other people to comment on the changes I'm making, because I'm having to make a bunch of critical choices. Because moreblocks is in used by a huge number of people, I'm trying to make my changes not cause crashes on current servers, or result in any unknown nodes, but at the same time, I've made (and will continue to make) some serious structural changes to the mod, and would appreciate some extra eyes to check for situations that may escape my attempts to think things through or test things.

I anticipate the work I've got planned may take a couple more days to a couple more weeks, depending on time and health.

Some goals:

fix certain bugs:

maybe:

maybe some of the other bugs/PRS, but they mostly either seem to be fixed or unfixable

other remaining TODOs:

pie in the sky stuff, which i'll probably not fix in this PR:

outstanding PRs:

fluxionary commented 2 years ago

Problem exists with and without node in the saw

hm, i can't replicate the issue. what version of the minetest client/server are you using?

figured out how to replicate it - set the client language to non-default. i'll see what i can figure out.

Niklp09 commented 2 years ago

hm, i can't replicate the issue. what version of the minetest client/server are you using?

server: 5.6.0-dev-master client: 5.5.1

fluxionary commented 2 years ago

When I open the circular saw formspec I get this warning: ERROR[Main]: Ignoring out-of-bounds argument escape sequence in translation

the most recent commit should fix the issue.

Niklp09 commented 2 years ago

I found another bug: Many homedecor recipes are broken because the names of the half stairs have changed.

For example: In moreblocks 2.x building_blocks:slab_hardwood works but now it's building_blocks:slab_hardwood_8

Niklp09 commented 2 years ago

Slabs which are registered with stairsplus.compat.override_stairs have no sounds

fluxionary commented 2 years ago

Many homedecor recipes are broken because the names of the half stairs have changed.

they're still craftable, but unified_inventory seems confused:

screenshot_20220725_100737 screenshot_20220725_100748

fluxionary commented 2 years ago

Slabs which are registered with stairsplus.compat.override_stairs have no sounds

it's not just override_stairs, it's everything. not entirely sure how i missed that before, the fix is trivial: https://github.com/minetest-mods/moreblocks/pull/191/commits/f7f6c79f74376874cd7db3f3d1c491c6f84ae709

fluxionary commented 2 years ago

they're still craftable, but unified_inventory seems confused:

i created an issue upstream: https://github.com/minetest-mods/unified_inventory/issues/207

i don't see that issue as something that should block this PR.

Niklp09 commented 2 years ago

Another bug 😕 : when i dig "moretrees:stair_fir_trunk", i get back "fir_trunk" which is an unknown item.

fluxionary commented 2 years ago

apparently something broke when using a 5.6.0 client/server/game vs. 5.5.1:

screenshot_20220808_125609

screenshot_20220808_125710

i've also gotten at least one report that loading a schematic using worldedit which contained moreblocks/stairsplus nodes crashed someone's server, but i haven't been able to replicate that locally.

@Niklp09 sorry i missed your comment, i'll look into that.

fluxionary commented 2 years ago

when i dig "moretrees:stair_fir_trunk", i get back "fir_trunk" which is an unknown item.

should be handled by the most recent commit

fluxionary commented 2 years ago

apparently something broke when using a 5.6.0 client/server/game vs. 5.5.1:

apparently what broke was related to having "autoscale_mode = true" set, so that's an upstream client (engine?) issue

EDIT: created an upstream issue: https://github.com/minetest/minetest/issues/12672

fluxionary commented 2 years ago

i've also gotten at least one report that loading a schematic using worldedit which contained moreblocks/stairsplus nodes crashed someone's server, but i haven't been able to replicate that locally.

this was due to doing a //save on a machine w/ the code from this PR, and trying to //load it on a machine w/out it. WE crashes the game if you try to load something w/ nodes that the engine isn't aware of. i created an upstream issue: https://github.com/Uberi/Minetest-WorldEdit/issues/210

Niklp09 commented 2 years ago

I think the PR is ready for merging. I've not found any other bugs

Calinou commented 2 years ago

I think the PR is ready for merging. I've not found any other bugs

I'll try to find some time next week for a cursory review, then I think we can merge this and release 3.0.0 :slightly_smiling_face:

kilbith commented 2 years ago

Can you check that the micronodes are still collapsed using the i3 inventory mod?

fluxionary commented 2 years ago

Can you check that the micronodes are still collapsed using the i3 inventory mod?

i'm not sure i understand what you mean by collapsed. do you mean "hidden" by adding them to the not_in_creative_inventory group? the name of the setting has changed to stairsplus.in_creative_inventory, but the old name (moreblocks.stairsplus_in_creative_inventory) still works as well.

kilbith commented 2 years ago

By collapsed I mean that they're hidden behind a button that "groups" them. Clicking the item button expand the inventory list.

fluxionary commented 2 years ago

By collapsed I mean that they're hidden behind a button that "groups" them. Clicking the item button expand the inventory list.

got it, i don't use i3 so i didn't even realize that was a feature. that stops working because i3 tries to handle "compressing" the groups itself. i'll try to write a fix to incorporate that mechanism, though i3 doesn't seem to provide a real API for that feature.

fluxionary commented 2 years ago

By collapsed I mean that they're hidden behind a button that "groups" them. Clicking the item button expand the inventory list.

should work now w/ the commit i just pushed, please verify if you want

kilbith commented 2 years ago

though i3 doesn't seem to provide a real API for that feature

API

Also ensure that the slopes still represent the groups. The behavior should not change at all. Take note that i3 is the top mod on ContentDB.

Niklp09 commented 2 years ago

https://github.com/minetest-mods/unified_inventory/issues/207 needs to be fixed

fluxionary commented 2 years ago

Also ensure that the slopes still represent the groups.

microblocks (1/8th node) now represent groups. slopes are no longer guaranteed to even exist. microblocks currently have such a guarantee.

fluxionary commented 2 years ago

minetest-mods/unified_inventory#207 needs to be fixed

fixing unified_inventory is left as an exercise for the reader.

EDIT: it's on my todo list to make a successor to unified_inventory, but it's got serious architectural problems, and it's not fair to gate this issue on it.

Niklp09 commented 2 years ago

next Issue. Only appears with default:stone

grafik

fluxionary commented 2 years ago

next Issue. Only appears with default:stone

i removed the attempt to register craft schemata w/ inventory managers. that never worked the way i wanted it to.

Niklp09 commented 2 years ago

more crafting schemata

This will break a lot of recipes that used by other mods

fluxionary commented 2 years ago

more crafting schemata

This will break a lot of recipes that used by other mods

can you provide an example?

Niklp09 commented 2 years ago

techage:ta1_board1_xx

Niklp09 commented 2 years ago

I found a fix for https://github.com/minetest-mods/unified_inventory/issues/207

  1. outcomment line 38 in stairsplus/init.lua
  2. Set stairsplus.in_craft_guide to true

I know its hacky but it works grafik

fluxionary commented 2 years ago

I know its hacky but it works

doesn't work for me screenshot_20220922_080715

outcomment line 38 in stairsplus/init.lua

i'm guessing the locus is moreso https://github.com/fluxionary/minetest-moreblocks/blob/635f0af8e2e14c44dacf67468fc18eb484777d40/stairsplus/api/node.lua#L318-L334

it's certainly possible to do that synchronously (when variants and schemas are registered), but is much more complicated... but i guess i'll do that work.

techage:ta1_board1_xx

i see. i'm not quite sure what i'll do about this yet. on the one hand, i don't want to break stuff, but

  1. recipe conflicts are inevitable and are inherently an integration problem
  2. recipe conflicts don't even have to be an "one or the other" thing, inventory managers could absolutely provide a means to pick a particular output of a recipe. however, none do.

i will at the very least provide a config option to disable the crafting schemata.

p.s. why in the world does techage come w/ an outdated version of the stamina mod? O_o

fluxionary commented 2 years ago

it's certainly possible to do that synchronously (when variants and schemas are registered), but is much more complicated... but i guess i'll do that work.

did that in https://github.com/minetest-mods/moreblocks/pull/191/commits/6e16c54cc365f37fb0236cd2b27b2deb25fe589a

i will at the very least provide a config option to disable the crafting schemata.

did that in https://github.com/minetest-mods/moreblocks/pull/191/commits/7414e274afb9768df70f6f42cf35ac978132c9db

fluxionary commented 2 years ago

doesn't work for me

actually, i figured out what was going on there, i confused stairsplus.in_craft_guide and stairsplus.in_creative_inventory. i'm going to set stairsplus.in_craft_guide to default to true to avoid this sort of issue.

fluxionary commented 2 years ago

though i3 doesn't seem to provide a real API for that feature

API

this doesn't work because the variants are not all registered at the same time.

https://github.com/minetest-mods/i3/blob/066e0a5d9d82a37d730eee73a1fa2fbf94e1e9c3/src/api.lua#L293-L294

Niklp09 commented 2 years ago

With my settings i can see mtg stairs but actually should there be only the "real" crafting recipe (only happens with blocks what are converted from mtg stairs to stairs+)

grafik

Settings:

stairsplus.in_creative_inventory = false
stairsplus.in_craft_guide = true
stairsplus.crafting_schemata_enabled = false
fluxionary commented 2 years ago

With my settings i can see mtg stairs but actually should there be only the "real" crafting recipe (only happens with blocks what are converted from mtg stairs to stairs+)

i'm don't understand. what do you mean, you can see them? and what's wrong w/ the crafting recipe?

fluxionary commented 2 years ago

With my settings i can see mtg stairs but actually should there be only the "real" crafting recipe (only happens with blocks what are converted from mtg stairs to stairs+)

i'm don't understand. what do you mean, you can see them? and what's wrong w/ the crafting recipe?

oh i see, you disabled the crafting schemas, but you still get the recipes that the stairs mod creates.

hm... one solution would be to disable the stairs mod?

Niklp09 commented 2 years ago

hm... one solution would be to disable the stairs mod

I'm testing

Niklp09 commented 2 years ago

That works, but makes a lot of other things broken so also not a good solution

fluxionary commented 2 years ago

That works, but makes a lot of other things broken so also not a good solution

yeah, fair point, so now i am removing the recipes if crafting schemas are disabled. possibly a better solution would have been to have completely overridden stairs as a distinct mod in this pack. maybe at a later date.

fluxionary commented 2 years ago

a better solution would have been to have completely overridden stairs as a distinct mod in this pack. maybe at a later date.

i decided that's a much better solution actually, it simplifies a lot of the legacy code.

however, it doesn't work because of a dependency loop. for some reason unified_inventory depends on farming. *#@!

EDIT: ah, because of the need for cotton in bag crafting. ugh and that is not easy to fix w/out breaking up unified inventory into a modpack as well. not gonna do that now.

Niklp09 commented 2 years ago

I have this PR running on my server for a few weeks without any further issues. I would say the PR is ready for merging

Niklp09 commented 2 years ago

yeah, fair point, so now i am removing the recipes if crafting schemas are disabled. possibly a better solution would have been to have completely overridden stairs as a distinct mod in this pack. maybe at a later date.

Simple but ugly hack: remove crafting functions from mtg stairs

depascalis commented 1 year ago

Legacy API doesn't seem to be working, this broke Cool Trees for me (cdb version) when using stairsplus:register_all(), or am I missing something?

ERROR[Main]: ModError: Failed to load and run script from C:\...netest\minetest\bin\..\mods\cool_trees\mahogany\init.lua:
ERROR[Main]: ...netest\minetest\bin\..\mods\cool_trees\mahogany\init.lua:268: attempt to index global 'stairsplus' (a nil value)
ERROR[Main]: stack traceback:
ERROR[Main]:    ...netest\minetest\bin\..\mods\cool_trees\mahogany\init.lua:268: in main chunk

https://github.com/runsy/cool_trees/blob/af8e445c475efb050c1291da459336812b84591e/mahogany/init.lua#L266

depascalis commented 1 year ago

@fluxionary I created some new worlds and now I can't get Cool Trees to fail. However, MT Expansion fails every time.

ModError: Failed to load and run script from C:\Minetest\minetest\bin\..\mods\mt_expansion\init.lua:
...t\bin\..\mods\moreblocks_fork\stairsplus\compat\init.lua:5: attempt to index local 'def' (a nil value)
stack traceback:
    ...t\bin\..\mods\moreblocks_fork\stairsplus\compat\init.lua:5: in function 'is_legacy_drawtype'
    ...bin\..\mods\moreblocks_fork\stairsplus\compat\stairs.lua:22: in function 'register_stair'
    ...\Minetest\minetest\bin\..\mods\mt_expansion/stairs.lua:23: in function 'my_register_stair_and_slab'
    ...\Minetest\minetest\bin\..\mods\mt_expansion/stairs.lua:79: in main chunk
    [C]: in function 'dofile'
    ...\Minetest\minetest\bin\..\mods\mt_expansion\init.lua:6: in main chunk
fluxionary commented 1 year ago

Legacy API doesn't seem to be working, this broke Cool Trees for me (cdb version) when using stairsplus:register_all(), or am I missing something?

do you have the stairsplus mod disabled? currently, the "moreblocks" mod optionally depends on the new "stairsplus" mod, so it should always be present unless it's explicitly disabled.

depascalis commented 1 year ago

@fluxionary I have everything enabled included under More Blocks except invsaw.

Cool Trees can however be something caused by me since I'm unable to reproduce the issue. MT Expansion however is still a valid issue.

fluxionary commented 1 year ago

MT Expansion however is still a valid issue.

this is because mt_expansion tries to register stairs for nodes that don't exist... which i suppose was a valid use of the "stairs" mod. i'll have to do some work to fix that.

fluxionary commented 1 year ago

pushed a fix, which still requires work on the part of the player, and will create a dependency cycle (stairs -> stairsplus -> unified_inventory -> farming -> stairs) until this PR against unified_inventory is accepted: https://github.com/minetest-mods/unified_inventory/pull/219

depascalis commented 1 year ago

Sorry @fluxionary but something else got screwed now.

ModError: Failed to load and run script from C:\Minetest\minetest\bin\..\mods\moreblocks_fork\stairsplus\init.lua:
...\..\mods\moreblocks_fork\stairsplus\resources\sounds.lua:7: attempt to index global 'default' (a nil value)
stack traceback:
    ...\..\mods\moreblocks_fork\stairsplus\resources\sounds.lua:7: in main chunk
    [C]: in function 'dofile'
    ...in\..\mods\moreblocks_fork\stairsplus\resources\init.lua:4: in main chunk
    [C]: in function 'dofile'
    ...minetest\bin\..\mods\moreblocks_fork\stairsplus\init.lua:43: in main chunk

Btw I'm on Minetest 5.6.0 and I'm not using Unified Inventory.

load_mod_i3 =                         mods/i3
load_mod_mt_expansion =               mods/mt_expansion
load_mod_invsaw =                     false
load_mod_stairsplus_legacy =          mods/moreblocks_fork/stairsplus_legacy
load_mod_stairsplus =                 mods/moreblocks_fork/stairsplus
load_mod_moreblocks =                 mods/moreblocks_fork/moreblocks
load_mod_moreblocks_legacy_recipes =  mods/moreblocks_fork/moreblocks_legacy_recipes
load_mod_stairs =                     mods/moreblocks_fork/stairs