minetest / minetest_game

Minetest Game - A lightweight and well-maintained base for modding [https://github.com/minetest/minetest/]
http://minetest.net/
Other
1.43k stars 575 forks source link

Default mod is bloated #1771

Closed Wuzzy2 closed 7 years ago

Wuzzy2 commented 7 years ago

This is more a criticism of Minetest Game's development than anything else. Take from it what you want.

The default mod is way too bloated. It reminds me a lot of the anti-pattern “God Class”(https://lostechies.com/chrismissal/2009/05/28/anti-patterns-and-worst-practices-monster-objects/).

default has the following responsibilities:

And the bloat doesn't seem to ever stop. It is still common that new features are added into default, making the bloat even worse. The scope of default is undefined either,

This is bad because whenever you need to depend on anything from Minetest Game which happens to be in default, you have to depend on all the baggage the default carries along. Also, default provides an API for three (!) relatively unrelated things. This is bad software engineering.

More evidence of bloat is given if you just look at the line counts:

  2441 nodes.lua
  1826 mapgen.lua
  1229 crafting.lua
   574 functions.lua
   535 trees.lua
   419 tools.lua
   334 craftitems.lua
   330 furnace.lua
   154 player.lua
   146 torch.lua
    77 aliases.lua
    74 item_entity.lua
    52 init.lua
    25 legacy.lua

  8216 total

3 files alone have a line count greater than 1000. nodes.lua is extremely heavyweight, especially since it cobbles togehter many nodes which are not really related to each other.

Note I have no interest in working for Minetest Game. This post is more an opinion piece. I think my criticism is still valid, however. And it is a complaint from Minetest Game modders which comes up quite often.

C1ffisme commented 7 years ago

Related : #726

rubenwardy commented 7 years ago

👏

sfan5 commented 7 years ago

@rubenwardy In contrast to other bug reports that actually affect end users in some visible way, this should definitely be low priority.

paramat commented 7 years ago

We have actually already stated that from now on new stuff will be in separate mods if possible, rubenwardy's presence helps with that and there is no resistance to that.

And the bloat doesn't seem to ever stop. It is still common that new features are added into default, making the bloat even worse.

If so it is done for good reason.

See #726 it's all been discussed before.

C1ffisme commented 7 years ago

We have actually already stated that from now on new stuff will be in separate mods if possible, rubenwardy's presence helps with that and there is no resistance to that.

That's not really any better, that's just the difference between shoving everything into one box, and shoving each individual item into it's own box, and then throwing them onto the shelf without any organization. Plus, the /mod spam would be unbearable.

Instead, it would be a good idea to organize features into categories, such as mapgen, commands, underground, flora, player, etc.

ghost commented 7 years ago

This is totally NOT a “possible close”. This is a long-term enhancement and should be dealt with properly, because in it s current state it’s the worst we can get.

Plus, the /mod spam would be unbearable.

Then it has to be reworked to hide the stuff that currently is in the default mess.

paramat commented 7 years ago

Ok, since there is no other issue on this open, removed label.

That's not really any better, that's just the difference between shoving everything into one box, and shoving each individual item into it's own box,

What i mean is, this discussion has been had before and we agree that future changes to MTG will be done keeping these requests in mind.

This is a long-term enhancement and should be dealt with properly, because in it s current state it’s the worst we can get.

Yes it's bad, but there was discussion before in 726 about re-organising default and it was decided it is not worth doing due to the disruption, better to start a new fresh subgame.

So, re-organisation is refused, and we already accept the request for the future, so the only remaining issue is this:

The scope of default is undefined either,

So this is why i added the label.

C1ffisme commented 7 years ago

Then it has to be reworked to hide the stuff that currently is in the default mess.

This should probably be a parameter you type after the command, e.g: /mod no_subgame.

ghost commented 7 years ago

This should probably be a parameter you type after the command, e.g: /mod no_subgame.

This should be as simple as possible for everyday use. So /mods for listing all the mods except the subgame mods and /mods subgame to list the subgame mods only.

ThomasMonroe314 commented 7 years ago

I agree in part with Wuzzy

tacotexmex commented 7 years ago

Even if this has been brought up before, the fact that it is again pointed out is telling. Most subgames rely on it. Since

MTG is currently a simple core to add mods to, it's not meant to be fun, exciting, a complete subgame or any kind of specific subgame. MTG is lacking many things and this is intentional.

a modular design of it is crucial.

I don't understand the

it is not worth doing due to the disruption

argument. The point is to move out stuff out of default, not to break compatibility, yes? It would be the opposite of disruption. Besides, burli has already succeeded in breaking up default into smaller parts (though this was made in order to also slim it down to a new mod base, but that's not the point).

If default was broken up into smaller mods, backward compatibility would simply be a question of an empty default mod with dependencies on the new mods.

ghost commented 7 years ago

Simply extracting every aspect from default into an own mod and creating a new default mod depending on all the new individual aspect mods would not break anything that depends on default but would make it possible do depend on individual aspects of the current default mod.

rubenwardy commented 7 years ago

That would only work if you didn't rename any items, which isn't ideal - but is better than a bloated default mod

paramat commented 7 years ago

No that's not better, names should follow convention. However, seems better and easier to start a fresh new mod base game.

rubenwardy commented 7 years ago

It's better to have a bunch of default_ mods with items called default: than a fucked dependency system.

And an incompatible new mod base completely ruins the point - the point is to make cross subgame support easier by having granular dependencies, so a mod for the mod base needs to also work in MTG

paramat commented 7 years ago

a bunch of default_* games

That's a good solution to the naming issue. And good point.

ghost commented 7 years ago

@rubenwardy The new default mod would not only depend on the other mods but also adds aliases for all currently registered things so nothing will break.

New things won't get aliased and with two more releases aliases will be removed (wasn't it that MT now only will be compatible for the the current and the wo previous releases?).

0.5 would be a good idea. Next "breaking release" will be 1.0 I guess.

paramat commented 7 years ago

I would rather avoid aliases, especially for all nodes. They only very slowly change nodenames in a world, i believe the nodename is only changed if a mapblock gets resaved, so aliases may have to remain for years to support old worlds.

rubenwardy commented 7 years ago

Aliases don't work very well if other mods use them in logic, as mods don't tend to check aliases.

Eg:

if stack:get_name() == "default:book" then
rubenwardy commented 7 years ago

I would rather avoid aliases, especially for all nodes. They only very slowly change nodenames in a world, i believe the nodename is only changed if a mapblock gets resaved, so aliases may have to remain for years to support old worlds.

I don't think this is a very good argument against them, aliases don't incur any lag or take up any memory. The code is also minimal.

C1ffisme commented 7 years ago

Recently an issue was created about redoing MTG for 0.5.0. As silly as it may be to completely redo MTG from scratch, it might not be a bad idea to copy-paste from the current MTG and slowly recreate a better MTG to replace the original.

It would break compatibility with older worlds, but at least we would have a solution to the problem of the default mod being bloated, and we can reorganize other bits of code that don't deserve an entire mod to their namesake. (I.E. give_initial_stuff)

rubenwardy commented 7 years ago

That's a very bad idea. For one, the main reason to debloat default is to make dependencies easier. Breaking every mod in this existence doesn't really help.

Give_initial_stufff 100% deserves its own mod as it's a standalone feature. Good mods are cohesive and have low coupling - that is, the mods introduce a single concept that's as self contained as possible. For example, books should be a separate mod as it's a self contained craft chain and festure. The dependencies go one way - books would depend on default for wood, but default would work perfectly without books.

C1ffisme commented 7 years ago

That's a very bad idea. For one, the main reason to debloat default is to make dependencies easier. Breaking every mod in this existence doesn't really help.

If we split default, mods already have to rewrite code. I can't say whether or not that will be the case if there is a new MTG for 0.5.0. (We're breaking support for mods anyway, though, so a new MTG just helps in the process, and we can focus on the other objective: organizing code to make MTG easier to develop.)

Give_initial_stufff 100% deserves its own mod as it's a standalone feature. Good mods are cohesive and have low coupling - that is, the mods introduce a single concept that's as self contained as possible.

In the case of a subgame mod, I don't think this should be the case. Sure, it's easy to find, but you can't get away with this for every single piece of code.

Mods should be organized into categories of related stuff that you can look at and know what it contains. (e.g. a mod named surface should contain dirt, trees, and respective mapgen code. A mod named stone should contain stones, ores, and oregen.) We should not have 40-50 mods that add seemingly random content. (E.G. mapgen,trees,stone,ores,dirt,plants,)

Does anyone use give_initial_stuff anymore? I feel like I have only heard of it whenever someone mentions removing it.

ghost commented 7 years ago

Aliases don't work very well if other mods use them in logic, as mods don't tend to check aliases.

Sorry, but that’s entirely their fault.

NathanSalapat commented 7 years ago

The only times I've used give_initial_stuff is in a custom subgame.

ghost commented 7 years ago

I can't say whether or not that will be the case if there is a new MTG for 0.5.0.

There won't be, and there never will be with how it currently is implemented. And all other subgames depend on default in one or another way.

Without completely breaking backwards compatibility by rigorously removing the current default and rewriting a default set of individual mods nothing will change on this situation.

paramat commented 7 years ago

aliases don't incur any lag or take up any memory. The code is also minimal.

Yes. However, they are messy in the way they have to be kept for many years and the way worlds then contain a mix of replaced or not-replaced nodenames that changes very slowly over time. Having a few is bearable but the idea of everything being an alias is a nightmare. They cause other issues too as described, likely to outweigh any advantage.

Wuzzy2 commented 7 years ago

Aliases don't work very well if other mods use them in logic, as mods don't tend to check aliases.

Sorry, but that’s entirely their fault.

As far I know, it is currently impossible to check whether an itemstring is aliased. :-(

rubenwardy commented 7 years ago

minetest.registered_aliases

Wuzzy2 commented 7 years ago

This is not mentioned in lua_api.txt, therefore it does not exist. :wink:

Desour commented 7 years ago

Maybe we need something like minetest.is_same_item.

paramat commented 7 years ago

So as i wrote above, is there any point keeping this open? Rubenwardy has done all he can to split stuff off, and is keen to do so whenever possible. We seem to have done all we can, and we state we will keep new things to new mods in future. So the issue is rather pointless now. @rubenwardy shall we close?

paramat commented 7 years ago

Obvious close. Rubenwardy is very keen to split default up more if at all possible, if anything more can be done it will be. Future development will be as requested.

AntumDeluge commented 7 years ago

I know this is closed, but I want to offer what I think is a solution for the "aliases" issue. If MTG contains a mod like cleaner (a fork of PilzAdam's clean mod) or override (I think override would be the better option for this), then server owners could easily manage aliasing old nodes without having to modify any mod code.

I think an idea like this would be a good solution because then the MTG game devs wouldn't have to worry about keeping any aliases in the code. That would be handled by server owners on an individual basis as needed.