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

Default mod is bloated #1771

Closed
Wuzzy2 opened this issue Jun 9, 2017 · 34 comments
Closed

Default mod is bloated #1771

Wuzzy2 opened this issue Jun 9, 2017 · 34 comments

Comments

@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Jun 9, 2017

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:

  • Registers a ton of nodes and items with no clear theme or scope, including, but not limited to:
    • Trees and saplings (+API)
    • Fences (+API)
    • Ladders
    • Signs
    • Keys
    • Chests
    • Furnaces
    • Basic plants
    • Torches
    • Almost all tools
  • Almost the entire mapgen code (+mapgen aliases)
  • Player model (+API)
  • Default sounds

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

C1ffisme commented Jun 9, 2017

Related : #726

@rubenwardy
Copy link
Member

👏

@sfan5
Copy link
Member

sfan5 commented Jun 9, 2017

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

@paramat
Copy link
Contributor

paramat commented Jun 10, 2017

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

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

ghost commented Jun 10, 2017

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

paramat commented Jun 11, 2017

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

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

ghost commented Jun 11, 2017

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

I agree in part with Wuzzy

@tacotexmex
Copy link

tacotexmex commented Jun 27, 2017

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

ghost commented Jun 27, 2017

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

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

@paramat
Copy link
Contributor

paramat commented Jun 28, 2017

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

@rubenwardy
Copy link
Member

rubenwardy commented Jun 28, 2017

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

paramat commented Jun 28, 2017

a bunch of default_* games

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

@ghost
Copy link

ghost commented Jun 28, 2017

@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
Copy link
Contributor

paramat commented Jun 28, 2017

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

rubenwardy commented Jun 28, 2017

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

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

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

rubenwardy commented Jun 28, 2017

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

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

ghost commented Jun 28, 2017

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

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

@ghost
Copy link

ghost commented Jun 29, 2017

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

paramat commented Jun 29, 2017

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

Wuzzy2 commented Jun 29, 2017

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

minetest.registered_aliases

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jun 29, 2017

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

@Desour
Copy link
Member

Desour commented Jun 30, 2017

Maybe we need something like minetest.is_same_item.

@ghost ghost mentioned this issue Jul 5, 2017
@paramat
Copy link
Contributor

paramat commented Aug 13, 2017

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

paramat commented Aug 30, 2017

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.

@paramat paramat closed this as completed Aug 30, 2017
@AntumDeluge
Copy link
Contributor

AntumDeluge commented Sep 7, 2017

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.

  • cleaner parses a file named clean_nodes.txt in the word path to remove unknown nodes.
  • override parses a file named overrides.txt which is used to unregister old items & nodes & alias their names to new ones.

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.

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

No branches or pull requests

10 participants