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

moreblocks is required for stairs compatibility #17

Open
Lazerbeak12345 opened this issue Jan 21, 2022 · 14 comments
Open

moreblocks is required for stairs compatibility #17

Lazerbeak12345 opened this issue Jan 21, 2022 · 14 comments

Comments

@Lazerbeak12345
Copy link

See minetest-whynot/whynot-game#49 for more info.

@Lazerbeak12345
Copy link
Author

@bell07 could you explain this further?

@wsor4035
Copy link
Collaborator

wsor4035 commented Jan 21, 2022

it is forward compatible, for example as alias support was added from the old stairs nodes to moreblocks https://github.com/mt-mods/homedecor_modpack/blob/master/building_blocks/node_stairs.lua#L9-L12.

ill be frank, mtg stairs mod has a terrible api, you have to pass it parameters rather than a table def it modifies. this means to support it and fix issues/warnings one would need to use the api, then override it, etc. additionally it register the node in its own namespace instead of the mods namespace as moreblocks properly does.

so tldr, there was three options,

  1. use the terrible mtg stair mod, use overrides/re registration/etc
  2. roll our own stairs support (something the greek mod choose to do)
  3. switch to just supporting moreblocks with backwards compat

option 1 is completely unreasonable from the mess of code and maintainability. additionally following #13 we would like to make mods as game agnostic as possible, so keeping a dep around mtg is rather not a good idea.

option 2 (besides reinventing the wheel) was a possibility, however it didnt make the most of sense when we already had a well used community mod for handling stairs

option 3 won out because it allowed us to use a sane api unlike the mtg one, and also provided a nice easy way to handle aliases for the older nodes

@Lazerbeak12345
Copy link
Author

Thanks for your insights @wsor4035. @bell07 's in a different timezone from me, so it'll take some time for them to see all this (and those comments made in our issue as well).

@wsor4035 wsor4035 changed the title Removal of stairs in a8fceb249d wasn't forwards compatible complaint about mtg stairs support removal and aliasing to moreblocks Jan 22, 2022
@bell07
Copy link
Contributor

bell07 commented Jan 23, 2022

Option 3 is no option, since the moreblocks provides a lot of things more, then just stair.
I think we need to go with the Option 2. Can you provide a link to the "greek mod" ?

@wsor4035
Copy link
Collaborator

Option 3 is no option, since the moreblocks provides a lot of things more, then just stair. I think we need to go with the Option 2. Can you provide a link to the "greek mod" ?

option 3 is a option and is the chosen option for this repo. you are free to do whatever you like in your own project

@bell07
Copy link
Contributor

bell07 commented Jan 23, 2022

I didn't mean to be rude. I meant the moreblock contain a lot more then just some stairs, so it is oversized for my need.

@S-S-X
Copy link
Member

S-S-X commented Jan 24, 2022

Opinions if this issue should stay closed or should be reopened?

I'd like to ask for more opinions about this issue, so ping @OgelGames and I guess @BuckarooBanzay would be approproate here.
There's valid point for complaint in my opinion, main question is should we reopen this issue and look for actual solutions?

Problem?

While moreblocks is the solution selected by @wsor4035 can it be used only for stairs/slabs without bringing in overrides that have very high potential to break things? (for example minetest-mods/moreblocks#172 and basically no configuration available to fine tune moreblocks content)
I think no and it is either everything or nothing.

Solutions?

If moreblocks mod could be made usable for everyone then maybe look at that option and what would need to be changed.
Would it be good enough to have configuration to allow disabling parts of moreblocks functionality?

Need to fork moreblocks to make it usable? Sure we could do that too and change a lot while still being able to include future updates from minetest-mods/moreblocks if it seems that they're not accepting changes that would be required.

Can something be done directly in homedecor modpack? Is it possible to drop hard dependency to moreblocks while keeping support for stairs and slabs? Would it be better than working on or if needed forking moreblocks?

Discussion?

Why I do think that issue should be discussed here instead of under minetest-mods/moreblocks repo: Changes that break compatibility happens here, not in moreblocks mod. Discussion should also stay here until there's acceptable solution (which might be just telling people to fuck off if we that's what we choose but I'm not exactly recommending that)

@wsor4035
Copy link
Collaborator

wsor4035 commented Jan 24, 2022

main question is should we reopen this issue and look for actual solutions?

no. see below

While moreblocks is the solution selected by @wsor4035

actually this has already been discussed in the irc/discord by myself and ogelgames and the moreblocks option was choosen. moreover the whole modpack already only supported moreblocks except for the building blocks mod, so precedence was already set.

Would it be good enough to have configuration to allow disabling parts of moreblocks functionality?

no, because its a optional dependency, so literally have the option of just not using it.

Need to fork moreblocks to make it usable? Sure we could do that too and change a lot while still being able to include future updates from minetest-mods/moreblocks if it seems that they're not accepting changes that would be required.

i would say no as its author is still active in the community and decently responsive to prs. however to fork it or not has no bearing here

Can something be done directly in homedecor modpack? Is it possible to drop hard dependency to moreblocks while keeping support for stairs and slabs? Would it be better than working on or if needed forking moreblocks?

this is incorrect. there is NOT a hard dependency on moreblocks just like there never was a hard dependency on stairs, its always been optional

Why I do think that issue should be discussed here instead of under minetest-mods/moreblocks repo: Changes that break compatibility happens here, not in moreblocks mod. Discussion should also stay here until there's acceptable solution (which might be just telling people to fuck off if we that's what we choose but I'm not exactly recommending that)

moreblocks issues can be discussed in there repo and issues here should be discussed here, not sure whats so hard about that. the current solution is we have provided legacy compat for people if they wish (most servers use moreblocks anyways) and people are free if they want to maintain working with the old, terrible, and deprecated(see mtg in maintaince mode and eventually will be removed from mte distribution - which already happens on some distros) they are free to add that themselves. we should not take on the burden of dealing with legacy and ugly code when better/more viable options occur. forcing use of the stairs api is like forcing use of technology from the 1990s when much better apis and support exist elsewhere

@Lazerbeak12345
Copy link
Author

Lazerbeak12345 commented Jan 24, 2022 via email

@wsor4035
Copy link
Collaborator

wsor4035 commented Jan 24, 2022

In particular, I think moreblocks should be split up into 3 mods:

this is some serious scope creep here talking about moreblocks. anyways, all i have to say on the matter is instead of splitting it into three mods it would be better off with settings to enable/disable parts for legacy compat there

@OgelGames
Copy link
Contributor

The problem I see here is that the compatibility depends on another mod. While stairs was an optional mod, the majority of users will have had it enabled, due to it being part of minetest_game. However, not all users will also have moreblocks installed.

I completely agree with @Lazerbeak12345 that stairsplus should be separated from moreblocks, but that still doesn't solve the problem for users that don't want to install either mod.

I think the only way to solve this is to implement option 1 with a setting to enable it, disabled by default. (something like enable_legacy_stairs)

@OgelGames OgelGames changed the title complaint about mtg stairs support removal and aliasing to moreblocks moreblocks is required for stairs compatibility Jan 24, 2022
@OgelGames OgelGames reopened this Jan 24, 2022
@wsor4035
Copy link
Collaborator

I think the only way to solve this is to implement option 1 with a setting to enable it, disabled by default. (something like enable_legacy_stairs)

so your solution is to add a setting that probably no one will know about, and very few if anyone will enable...

@wsor4035
Copy link
Collaborator

if such a thing is decided on, i would propose we enable add a minetest.log statement warning that it will eventually be removed in X amount of time

@OgelGames
Copy link
Contributor

so your solution is to add a setting that probably no one will know about, and very few if anyone will enable...

If we put it somewhere it can be seen (like in the readme), I'm quite sure those who need it will find it and enable it.

if such a thing is decided on, i would propose we enable add a minetest.log statement warning that it will eventually be removed in X amount of time

Why would it be removed?

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

No branches or pull requests

5 participants