Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

partial rewrite and major bugfix #191

Closed
wants to merge 68 commits into from

Conversation

fluxionary
Copy link

@fluxionary fluxionary commented Jun 12, 2022

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:

  • don't break existing functionality, at least for servers which are somewhat up-to-date, and at least sometimes used (in progress, see mods using circular_saw global or other weirdness fluxionary/minetest-moreblocks#9 for current issues)
  • reduce the implicit nodecount penalty of making nodes usable w/ the circular saw (via default-on "legacy" settings)
  • make game-independent. properly, without breaking existing functionality. by parameterizing texture/sound/craftitems.
  • remove dead code
  • adopt current game features which didn't exist when moreblocks was first made
  • fix a number of unreported bugs
  • create new API features
  • turn the mod into a modpack, separating the additional blocks from stairsplus
  • stricter luachecking
  • API for registering moreblocks variants (all faces, no faces, trap)
  • proper integration w/ i3, unified_inventory, and possibly other inventory managers
  • more crafting schemata
  • hide individual crafting schemata recipes (via config option)
  • API for adding new stations which respect the existing mechanics, so that other mods can take advantage e.g. Facade

fix certain bugs:

maybe:

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

other remaining TODOs:

  • update user documentation
  • update API documentation
  • update i18n files
  • test legacy support some more

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

  • create a tool to help server owners reduce the # of registered shaped nodes
  • create a tool to help server owners easily add more shapes for specific nodes or groups

outstanding PRs:

@BuckarooBanzay
Copy link
Member

you've set yourself some goals there 🤔 are you sure you don't want to solve one thing after another instead?

In any case: good luck, take your time and feel free to ping me up for testing or feedback 👍

@fluxionary
Copy link
Author

Query: does anyone know if any mods use the exported "circular_saw" global? I'd prefer to remove the circular saw as a distinct namespace vs. stairsplus. I haven't personally seen any evidence it's used, but the minetest ecosystem is quite diverse and hard to grep.

@fluxionary
Copy link
Author

are you sure you don't want to solve one thing after another instead?

at the moment, i think it'll be easier to achieve all the goals than to split it into distinct stages. i also admit that one of my common areas of failure is biting off more than i can chew. let's see if i can get this done in a week or two, as i expect.

In any case: good luck, take your time and feel free to ping me up for testing or feedback 👍

will do :)

@BuckarooBanzay
Copy link
Member

Query: does anyone know if any mods use the exported "circular_saw" global?

Did a quick grep in our modpack (https://github.com/pandorabox-io/pandorabox-mods/) and the following popped up:

I don't think that's a problem if just the global name changes 👍

@BuckarooBanzay
Copy link
Member

Query: does anyone know if any mods use the exported "circular_saw" global?

Relayed search result from @rubenwardy from the cdb: https://content.minetest.net/zipgrep/a831eba5-af21-4c4a-9cc9-c45981f21141/ (thanks)

@fluxionary
Copy link
Author

Relayed search result from @rubenwardy from the cdb: https://content.minetest.net/zipgrep/a831eba5-af21-4c4a-9cc9-c45981f21141/ (thanks)

that's a fantastic tool i didn't know existed. wish i had access to run new queries, but i can certainly see why it would be restricted.

i'll review the code usage and make a decision about whether to publish the old API as well, or submit PRs for those mods (if necessary)

@fluxionary
Copy link
Author

Things are at a point where I wouldn't mind having people test this to find bugs and integration issues. Please report the issues on my fork https://github.com/fluxionary/minetest-moreblocks/issues

@fluxionary
Copy link
Author

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
Copy link
Author

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: minetest-mods/unified_inventory#219

@depascalis
Copy link

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

@fluxionary
Copy link
Author

Sorry @fluxionary but something else got screwed now.

easy fix, i removed 'default' from mod.conf, when it was actually still needed.

@depascalis
Copy link

@fluxionary we should probably move this exchange, feels like we are polluting this thread 😂 Are you only available on IRC?

ModError: Failed to load and run script from C:\Minetest\minetest\bin\..\mods\mt_expansion\init.lua:
...test\minetest\bin\..\mods\moreblocks_fork\stairs\api.lua:23: attempt to register stairs for unknown node "wood:black". set `stairs.legacy_stairs_without_recipeitem = true` in minetest.conf to enable this behavior.
stack traceback:
	[C]: in function 'error'
	...test\minetest\bin\..\mods\moreblocks_fork\stairs\api.lua:23: in function 'register_stair'
	...es\Minetest\minetest\bin\..\mods\mt_expansion/stairs.lua:23: in function 'my_register_stair_and_slab'
	...es\Minetest\minetest\bin\..\mods\mt_expansion/stairs.lua:79: in main chunk
	[C]: in function 'dofile'
	...ames\Minetest\minetest\bin\..\mods\mt_expansion\init.lua:6: in main chunk
Check debug.txt for details.

@fluxionary
Copy link
Author

Are you only available on IRC?

i'm fluxionary on libera.chat and am usually in the #minetest channel. i can't use discord because they keep blocking me w/out telling me why.

in this case, the solution is actually in the error message:

attempt to register stairs for unknown node "wood:black". set stairs.legacy_stairs_without_recipeitem = true in minetest.conf to enable this behavior.

@depascalis
Copy link

depascalis commented Dec 10, 2022

in this case, the solution is actually in the error message:

Alright my bad, I suck at this, I got it running now though.

Next issue: All textures are missing on nodes generated from moreblocks. It might be because in the original moreblocks the name of a generated node is moreblocks:stair_cobble_inner but in this PR default:stair_cobble_inner. Even after disabling the whole moreblocks mod (your version) and running with the default stairs mod, textures are missing.

edit: textures are actually missing on most nodes from moreblocks I think, check moreblocks:cactus_brick_inner for example.

image

The left node was created with the original moreblocks and was named moreblocks:stair_cobble_inner and had a texture.
Disconnected and switched mod version
The right node was created with your version of moreblocks and was named default:stair_cobble_inner.
Both left and right are however named default:stair_cobble_inner now.

image

I should add that I have renamed the mod folder, and name/title in modpack.conf if that could be a cause.

@depascalis
Copy link

Another issue, but I don't know if it's related to this PR? Something weird is going on with groups. See the compression feature in i3 is nested, and the nested group can't be expanded since it collapses everything on click.

image

@kilbith
Copy link
Contributor

kilbith commented Dec 10, 2022

@Calinou please don't merge this and let @fluxionary fork this project with their changes. This is too disruptive and there are apparently too much issues... Sorry man but I'm not fan of these changes packed up in a gigantic PR 👎

@depascalis
Copy link

depascalis commented Dec 11, 2022

@Calinou please don't merge this and let @fluxionary fork this project with their changes. This is too disruptive and there are apparently too much issues...

But they are so close 😥

Sorry man but I'm not fan of these changes packed up in a gigantic PR 👎

I can understand this though. I don't know how it could have been done incrementally, but what do I know.

I love where this PR is going, but yeah it's a bit messy with all the sub mods. But I prefer it being messy and more functional than the other way around.

@fluxionary
Copy link
Author

fluxionary commented Dec 11, 2022

Next issue: All textures are missing on nodes generated from moreblocks.

cf.

this was a bug in the engine that this PR unearthed, which has been fixed, but isn't yet part of a proper release.

See the compression feature in i3 is nested, and the nested group can't be expanded since it collapses everything on click.

i will work on understanding why that's behaving wrong, but i3 is not designed to work well w/ other mods, and i won't promise to try to fix it.

please don't merge this and let @fluxionary fork this project with their changes.

i'd rather not take over the entire minetest mod ecosystem, but it seems like maybe i have to in order to fix it. i certainly can't maintain all the mods forever, but a lot of other people's "fixes" are just making the ecosystem worse.

@fluxionary
Copy link
Author

in any event, it'd be more useful for me to respond to individual issues on my own fork of moreblocks, vs. trying to fix everything in this PR.

@fluxionary
Copy link
Author

if the community wants me to actually fork moreblocks, it will end up w/ an AGPL license get have the structure and API normalized according to my own current standards. i want to find a compromise where this mod doesn't become my personal responsibility, but i'm getting the feeling that that's the only way forward.

@Lazerbeak12345
Copy link

Flux, if you don't want to be the maintainer for this, and if this pr gets rejected, I can maintain it.

@fluxionary
Copy link
Author

fluxionary commented Dec 12, 2022

Flux, if you don't want to be the maintainer for this, and if this pr gets rejected, I can maintain it.

i want minetest-mods to be the maintainer. i think that something as crucial as moreblocks (and in particular, the stairsplus bit) ought to have more than one set of eyes on its maintenance. creating a hard fork won't solve that, but at the same time, i'd rather do that than just have this passed over indefinitely.

what's my alternative, creating yet another minetest mods collective? i'd have to recruit active mod maintainers that i respect? i have no desire to administrate such a project, i'd rather get existing community to sign off on my changes.

but when a single issue like this becomes its own meta-bug-tracker for everything wrong w/ a project, forking the project very much seems to be a reasonable option.

:\

@fluxionary
Copy link
Author

Another issue, but I don't know if it's related to this PR? Something weird is going on with groups. See the compression feature in i3 is nested, and the nested group can't be expanded since it collapses everything on click.

i can't replicate this locally; can you give more detailed instructions, involving which versions of i3/moreblocks/other things?

@fluxionary
Copy link
Author

is there anything i can do to get this accepted? if i don't get a response about what is still needed by 2023-02-01, i will abort this PR and do a hard fork, and commit to maintaining that.

@wsor4035
Copy link

wsor4035 commented Jan 8, 2023

is there anything i can do to get this accepted? if i don't get a response about what is still needed by 2023-02-01, i will abort this PR and do a hard fork, and commit to maintaining that.

pinging @Calinou since i believe this is primarily there mod

@fluxionary fluxionary closed this Feb 1, 2023
@Lazerbeak12345
Copy link

Good luck with your fork Flux! :)

@depascalis
Copy link

@fluxionary, @Calinou said they'd accept this pr on irc.

@fluxionary
Copy link
Author

fluxionary commented Feb 2, 2023

@fluxionary, @Calinou said they'd accept this pr on irc.

wait, did they? apologies if i missed it. if they can post a list of things i need to do to get it accepted, i'll re-open it.

EDIT: this PR has been one of several where i've gotten pushback because i changed too much at once, and i'm trying to learn from that., but there's so much going on here now that it'd be a huge project to break it up into separate PRs. on the other hand, i'd rather do the work to redo this PR if there's a sane way to refactor it, than have this PR ignored and have to maintain this code on my own.

@wsor4035
Copy link

wsor4035 commented Feb 2, 2023

@fluxionary, @Calinou said they'd accept this pr on irc.

wait, did they? apologies if i missed it. if they can post a list of things i need to do to get it accepted, i'll re-open it.
--snip--

https://irc.minetest.net/minetest/2023-01-08#i_6046025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants