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

stairs mod shouldn't have a hard dependency on default #2403

Open
FaceDeer opened this issue Jul 8, 2019 · 13 comments
Open

stairs mod shouldn't have a hard dependency on default #2403

FaceDeer opened this issue Jul 8, 2019 · 13 comments

Comments

@FaceDeer
Copy link

FaceDeer commented Jul 8, 2019

I've noticed that the stairs mod's dependency on default is kind of backwards from how it seems like it should go. Half of the stairs mod's init.lua file consists of a bunch of calls to stairs.register global functions to register various default mod node types as stairs. Shouldn't it be that the stairs mod simply provides an API that other mods make use of? Other mods define a dependency on the stairs mod instead.

So ideally I think default should have a "stairs" optional dependency, and have this big pile of node registrations done from inside default (inside a check for whether stairs is enabled, of course).

I see that the default mod doesn't have much in the way of dependencies, though, so if there's a general "no dependencies for default" rule behind that I think the dependency stairs has on default should at least be made optional, and those registrations split out into a separate lua file to make it easier to reuse the stairs mod without the default mod.

Either approach is easy, I can send a pull request. I figured I'd check first to see which option was more likely to see support.

@TumeniNodes
Copy link
Contributor

But the reason stairs have default as a hard dep, is because it relies on default for the textures, sounds.
I'm not knocking the idea, it just seems odd to me to reverse the calls.
Bloating default seems the wrong way to go, and I'm not sure how it would improve efficiency or anything.
Every game has a "default" file (whether it is called core, or whatever) which provides a base code which other mods within the game need, to exist.

Anyway, it's just my own opinion on the subject.

@Napiophelios
Copy link
Contributor

Napiophelios commented Jul 8, 2019

I think the sounds would be the main obstacle.

@TumeniNodes
Copy link
Contributor

TumeniNodes commented Jul 8, 2019

.. and textures, and material groups, etc..
But I could be missing the overall idea too.
Default, in any setting, is the "go to", where all the good stuff happens.
I mean, if it is doable to make default an optional dep for stairs (or any other mod for that matter) then by all means, go for it.
But all the soup has to come from somewhere... or things die off from starvation.

stairs could be made as a self contained mod, with specialty textures and it's own sound files, etc., but then we would be wasting resources using duplicated files.
Neither proposal seems practical tbh

@Napiophelios
Copy link
Contributor

yeah, I really don't see the point in the suggested changes.
"to make it easier to reuse the stairs mod without the default mod"
If you are gonna do that it's easy enough to just edit the file.

@TumeniNodes you can use stairs mod without registering any materials from the default mod, but you would still need the soundFX registered for groups.

@FaceDeer
Copy link
Author

FaceDeer commented Jul 8, 2019

All of the sounds, textures, and so forth are only referenced in the second half of the mod's init.lua, where all the default-node-specific registration is going on. That's the part that would be made dependent on the existence of the default mod, or moved into the default mod directly, depending on which of my two suggestions was implemented. The first half of init.lua consists of all the API stuff, it doesn't depend on any resources from the default mod.

Imagine splitting stairs' init.lua in half at line 458. Everything above that line has no references whatsoever to the default mod, everything below that is purely default-mod-specific.

The reason I've brought this up is that I'm planning on actually making a game that doesn't include most of the existing default mod's geological node types in it - no default:stone, no default:cobble, etc. I'm probably going to rip the default mod apart entirely, in fact, and call it something else. So I started scouting around inside the default game to see what bits can be reused without that and which bits will require modification. The stairs mod will require lots of modification, but it shouldn't have to require modification - making the default dependency optional is really quite trivial.

Edit: haven't tested it yet, but I think this is the minimum change to accomplish this.

@paramat
Copy link
Contributor

paramat commented Jul 9, 2019

FaceDeer, interesting idea and does make sense. But if i was using the stairs mod in another game i would prefer to just edit out the registrations to remove unnecessary lines, rather than have them disabled by your 'minimum change'.

Moving the registrations into default makes the most sense, to register the stair nodes in the same mod that registers the full cube versions. If this is actually practical i'll support this. Best wait for another core dev to agree though, before you make a PR.
It seems this is how it should have been done from the start.

default can have dependencies if it is practical to do so.

Some mods in MTG are coded as if they are an integral part of MTG, making them more difficult to use for other games, i'm discovering this myself by writing a very simple game of my own. Stairs is a mod that will be often reused in other games.
I support making changes to make MTG mods more usable in other games.

@Napiophelios
Copy link
Contributor

we could do fences, doors, and walls too :)

@FaceDeer
Copy link
Author

Yeah, I noticed those too shortly after filing this issue but figured I'd see how this stairs proposal went before going on to a grand "reorganize-all-the-things!" proposition. :)

I agree with Paramat that the diff I posted above is not the ideal solution, this collection of mods would be more easily swappable and reusable if everything referencing "default:whatever" was physically located inside the default mod itself (where conveniently possible, of course). It was just meant to show how well encapsulated the default mod references already were.

@Napiophelios
Copy link
Contributor

I think it ultimately leads back to Rubenwardy's suggestion to split up default
#726

@paramat
Copy link
Contributor

paramat commented Jul 10, 2019

Stairs are different to doors and walls though, as the stair node registrations belong in the same mod that registers the full cube version.
For doors and walls, the registrations could be split off into separate mods instead of being added to default.
The fence APIs are already in default so i'm not sure it makes sense to move the fence registrations out of default.
This isn't really about splitting up default, which is difficult and has already been done as much as is easily possible.

@TumeniNodes
Copy link
Contributor

TumeniNodes commented Jul 10, 2019

just an fyi, I have fences & gates which can use any material for posts, and another for the rails.
So if anything is changed regarding those, it should probably be to switch them to this method. ; )

The only thing which concerns me in this subject is, perhaps some want to remove the stairs mod in lieu of a custom stair mod, or none at all... right now it is very simple... remove the mod.
If they are embedded into default's code, now they have to frig with that.

Sure, it would only be a matter of commenting out some lines, or even deleting them but (as long as the insertion is done clean), it's still easier to simply remove the mod in question.

A while back, the talk was to start removing aspects from default (which you, paramat, even did some) and now we're talking about throwing other mods into it?
I'm just a little confused about it.
Are you suggesting this is how it should have been done from the beginning, based on the fact they are just full nodes with bites taken out of them?

I thought the overall desire, was to try to unbloat 'default' as much as possible?
Integrating stairs into default, makes them even more dependent on default, does it not?
I guess making default an optional depends for stairs, sound better now than throwing them into default itself to me.
So I'm all for that option... that could be done with each of the mods outside of default. Even though it actually makes no sense, as they will still need to call on default for their defs.

And if default has 'stairs' as a dep, now if someone wants to remove them, default is missing a dep and will fail... just sounds like creating more work in the long run, both ways, for no real improvement on efficiency or anything.

I won't say any more on this as it is just confusing me xD
And I must have a complete misunderstanding of the reasoning related to the matter.

@paramat
Copy link
Contributor

paramat commented Jul 10, 2019

Yes, we could create a new mod that contains the stair registrations, they just seemed more suited to be together with the full cube nodes, but i admit that's probably not essential, and possibly not ideal, i haven't thought it through much.

@FaceDeer
Copy link
Author

FaceDeer commented Jul 10, 2019

Stairs wouldn't be a hard dependency for the default mod, it'd be an optional one. The series of calls to the stairs.register_stair_and_slab API would be wrapped inside a "if minetest.get_modpath("stairs")" check.

So if someone just ripped the "stairs" mod out of minetest_game, default would load up just fine and the missing methods would not be called.

If someone created a new "stairs" mod that was named the same and provided the same API as the existing minetest_game stairs, that new mod could be slotted right in and it would seamlessly take over handling those stair registration.

If someone created a new stairs mod and it didn't provide the same API as the existing stair mod, well, they probably shouldn't have done that - if the API is incompatible it should probably have a different name. This could still be guarded against by having the "if stairs is installed" check also check if a "stairs" global table existed and whether that table had a "register_stair_and_slab" member before trying to call it, I've done that sort of thing to handle mods and even minetest builtin APIs that have changed between versions. Adding extra checks here has trivial performance costs since it's executed once at server startup and is just a couple of nil checks.

The net result of this would be that you could install default or stairs mods in any combination and it would all "just work" - default would register default node stair types if the stairs mod was present, and stairs wouldn't try registering them if default wasn't present.

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

4 participants