MinecraftServerControl / mscs

Powerful command-line control for UNIX and Linux powered Minecraft servers
https://minecraftservercontrol.github.io
BSD 2-Clause "Simplified" License
489 stars 62 forks source link

Overviewer settings to use env vars for paths #304

Closed queria closed 2 years ago

queria commented 2 years ago

Currently hardcoded paths for overview-settings.py make it hard to keep the configs in git(...) repo as they force the server backup/map files to always be in exact absolute path on the machine.

(While also it cannot be always re-generated if user wants to tweak the rendering settings or such.)

For this lets not hardcode absolute paths, only relative, relative to the environment variables we already have available in msctl, so we can export them and use them as env vars inside python settings file.

queria commented 2 years ago

I wonder, if it would be ok to also pass the world name as variable, as that way same settings/overviewer-settings.py file could be used across multiple servers/worlds at the same time?

queria commented 2 years ago

I would not suggest changing MSCS to use a single Overviewer settings file as it would break having different settings for each world.

I was wondering about this, and You mentioning it explicitly made me wonder even more - due to seeing those 'useless' one-entry-only dropdown boxes on my maps now, wondering if they are meant for switching between worlds.

But I would also feel this may be too far for default configs (as You point it would make it hard to have different settings afterwards). Although if i understand it right, only thing preventing using msctl to render such cross-world maps is ... ... well technically nothing (one can prepare overviewer-settings.py and call msct map just-the-world-with-cross-world-map-config)?

Maybe only for convenience so that 'msctl backup && msctl map' could be used (without having to maintain list of worlds to render, so that those you want grouped are, but rest is auto-generated and mapped as being added/removed):

zanix commented 2 years ago

I would not suggest changing MSCS to use a single Overviewer settings file as it would break having different settings for each world.

I was wondering about this, and You mentioning it explicitly made me wonder even more - due to seeing those 'useless' one-entry-only dropdown boxes on my maps now, wondering if they are meant for switching between worlds.

It kinda is, but mainly for different dimensions in the same world, normal, nether, and the end. I haven't seen a config that merges multiple worlds into one Overviewer map though.

But I would also feel this may be too far for default configs (as You point it would make it hard to have different settings afterwards). Although if i understand it right, only thing preventing using msctl to render such cross-world maps is ... ... well technically nothing (one can prepare overviewer-settings.py and call msct map just-the-world-with-cross-world-map-config)?

The limit is how Overviewer works, like I said, I haven't seen a config that merges multiple worlds into one map. Having a simple default settings file is best for ease of use.

Maybe only for convenience so that 'msctl backup && msctl map' could be used (without having to maintain list of worlds to render, so that those you want grouped are, but rest is auto-generated and mapped as being added/removed):

  • it could be possible to 'exclude/disable' overviewer for selected worlds?
  • will check if there is easy way (eg. empty overviewer-settings or it's configuration part so that overviewer does nothing?)
  • maybe better would be opt in mscs.properties to disable overviewer/mapping for that world?

This kinda exists already with the optional world arguments for mscs map <world1> <world2> <...> By default it renders all worlds but you can just pass only the worlds you want mapped. An option to completely disable it might be nice so you can just run mscs map or you don't accidently run the mapping on a world you don't want to.

  • (i've seen there is issue asking for changing mapping programs, maybe it could be done that way - override the program to empty value to disable mapping?)

There is a request, but no work has been done with it. I started to at one time but I couldn't find a working alternative map renderer at the time. Maybe I didn't look hard enough? I did use Dynmap at one time for a modded world, but that is a realtime renderer and runs as a mod with the server. For that I just ran mscs map for the specific worlds I wanted to render.

queria commented 2 years ago

It kinda is, but mainly for different dimensions in the same world, normal, nether, and the end. I haven't seen a config that merges multiple worlds into one Overviewer map though.

Ah, You are right. Since I'm using spigot/paper servers, it was not rendering nether/end (split world folders case). I've noticed that already and corrected that in my overviewer config. (Wonder if it's worth adding maybe as commented out here into default overviewer config by mscs? Or maybe as detecting if the _nether or _the_end directories exists to use those instead?)

Understanding this more it makes sense that merging different worlds into one is not good idea (chunk/tile change detections, textures etc who know how it would work for completely different servers/worlds when smashing it together).

This kinda exists already with the optional world arguments for mscs map <world1> <world2> <...> By default it renders all worlds but you can just pass only the worlds you want mapped. An option to completely disable it might be nice so you can just run mscs map or you don't accidently run the mapping on a world you don't want to.

Since I have some worlds for which I do not want to have map at all (very big map provided by public server at the end of season before when updating and wiping for new one - so not much to be 'played' at my place more like just explored for curiosity about what all the youtubers did there)

... I've found a way - just leaving the 'overviewer-settings.py' as plain empty file does the trick - i can still use periodic 'msctl backup && msctl map' and this gets skipped (though now i wonder about how to skip backup for that world :] ).

  • (i've seen there is issue asking for changing mapping programs, maybe it could be done that way - override the program to empty value to disable mapping?)

There is a request, but no work has been done with it. I started to at one time but I couldn't find a working alternative map renderer at the time. Maybe I didn't look hard enough? I did use Dynmap at one time for a modded world, but that is a realtime renderer and runs as a mod with the server. For that I just ran mscs map for the specific worlds I wanted to render.

I see, was yesterday quickly searching few lists of mapping tools, fot those i would add to my list to test/consider aside overviewer ... and came out empty handed aside DynMap, but if that works differently as you describe, then there is likely not much reason to add support for non-overviewer mapping tools (unless explicit described case/request would be provided i guess?).

queria commented 2 years ago

Btw, I'm not sure if I read the shellcheck results correctly, but seems to me the failures there are not related to my change, or did I overlooked something there?

zanix commented 2 years ago

... I've found a way - just leaving the 'overviewer-settings.py' as plain empty file does the trick - i can still use periodic 'msctl backup && msctl map' and this gets skipped (though now i wonder about how to skip backup for that world :] ).

Nice, I thought Overviewer would have returned a fatal error and keep other worlds from being rendered.

I see, was yesterday quickly searching few lists of mapping tools, fot those i would add to my list to test/consider aside overviewer ... and came out empty handed aside DynMap, but if that works differently as you describe, then there is likely not much reason to add support for non-overviewer mapping tools (unless explicit described case/request would be provided i guess?).

Indeed, having another map renderer would make it easier to test support for multiple map renders.

Btw, I'm not sure if I read the shellcheck results correctly, but seems to me the failures there are not related to my change, or did I overlooked something there?

Yep, looks like we have issues with our tests, and I'm not 100% sure they were completely working in the first place as we have an open PR (#290) to fix issues with it.

zanix commented 2 years ago

Btw, I'm not sure if I read the shellcheck results correctly, but seems to me the failures there are not related to my change, or did I overlooked something there?

Tests were fixed on main, rebase and recommit.

sandain commented 2 years ago

Hi @queria. Thanks for putting this patch together. I think it will be a nice change. As @zanix mentioned, I just fixed the shellcheck errors in the main branch. If you rebase, you should see those errors go away.

sandain commented 2 years ago

I'm going to go ahead and merge this PR. It looks correct to me, regardless of the shellcheck failure that would probably be fixed with a rebase.

sandain commented 2 years ago

The shellcheck failure disappeared after merge. So all is good. Thanks @queria for the patch.