-
Notifications
You must be signed in to change notification settings - Fork 18
Add a chunkloader #158
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
base: master
Are you sure you want to change the base?
Add a chunkloader #158
Conversation
Sorry if it's crappy, it's my first serious mod and first PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, textures in sbz are 1 node = 16x16 pixels, you have to change them
mods/chunkloader/crafting.lua
Outdated
{ "", matter_blob, "" }, | ||
{ matter_blob, "", matter_blob } | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks unfinished? you should buff the recipe, make it involve warp devices or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did mention that the crafting recipe is for testing only and I am counting on you on providing more balanced recipe because no one could know your game better, I suppose :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not my game (i only wrote/added majority of the code), i stopped actively working on it, and i don't design recipes that well, i find it an annoyance tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this resolved?
mods/chunkloader/logic.lua
Outdated
@@ -0,0 +1,39 @@ | |||
chunkloader.action = function(pos, _, meta, supply, demand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be ideal if the chunkloader worked only if the player was online, in multiplayer
Because having it be always loaded might not be a great idea for server performance
Also, you should maybe check how switching stations behave when you walked away from them (Are they loaded?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of chunkloaders came to my mind when I created a super advanced battery factory and wanted it to work all night through so that I could wake up in the morning and see all batteries ready for use.
I understand concerns about server performance and I think that we might add a limit for a player to, for example, have a limit of N chunk loaders per server or something (the N may be put to the config for better configurability).
I checked it in singleplayer and it looks like everything's okay but yea I think it needs better testing and some improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of chunkloaders came to my mind when I created a super advanced battery factory and wanted it to work all night through so that I could wake up in the morning and see all batteries ready for use.
well, in sbz i had always the idea of "Instead of waiting, how about expanding your factory instead"
I understand concerns about server performance and I think that we might add a limit for a player to, for example, have a limit of N chunk loaders per server or something (the N may be put to the config for better configurability).
Oh yeah, i completely forgot about that, that is needed, limit the player to only be able to make like 30 chunkloaders
As for performance, skyblock zero is not very performant, at least not as performant as i'd want it to be i guess, it would be nice if machines weren't constantly loaded for no reason
I checked it in singleplayer and it looks like everything's okay but yea I think it needs better testing and some improvements
ok
mods/chunkloader/logic.lua
Outdated
local prev_state = meta:get_int("prev_state") == 1 | ||
local perpetual = meta:get_int("perpetual") == 1 | ||
|
||
local power_needed = 25 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buff this to a lot more, like 300 power
core.register_node("chunkloader:loader_inf", { | ||
short_description = "Chunk Loader (INF)", | ||
description = | ||
"Chunk Loader (INF)\nKeeps the mapblock it's located in active\nDoes not consume power.\nInfinite.\nOnly for admins.\nAnd mods maybe\nMods like moderators, not ones that are written in Lua.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to simplify to "Creative Mapblock Loader"
Also, nitpick: i suggest renaming it to mapblock loader, as a chunk implies a mapgen chunk, which is a LOT larger of an area
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I kinda came from minecraft and it's a kind of legacy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in general, I do agree with the renaming
I think this is good for your first PR |
So, if I:
This could be accepted? |
yes and
|
I like this PR, when the requested changes are implemented i'll merge it |
…d storage instead of players' meta
Just so you know, I'm working on it. I have implemented the limit mechanism, resized textures and renamed it to mapblock loader. Now I'm thinking about the recipe and the quest. |
I think that all your requirements should be met now and it is ready for merge. I'd like to also note that I'd like to make it highlight the mapblock it is located in on punch, just like the jumpdrive, but it is planned for now. I think we could merge it as soon as I implement it :) |
You know it was a lot easier when you read the docs and have a nice and handy vizlib. I think it is done now |
meta:set_string("infotext", run and "Running" or "Idle") | ||
|
||
if prev_state == 0 and run == 1 then | ||
core.forceload_block(pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is limited to 16 blocks by default (see doc).
(Also, I'd recommend to use transient
forceloading, and keeping track of the active forceloaders yourself. This avoids leaks if something is modified (e.g. forceloader is removed with worldedit, or mod is removed). The non-transient is kind of an unnecessary and bad feature.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will fix it, thanks for the comment!
is this ready? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr i think there still is some work to do
mods/mapblockloader/crafting.lua
Outdated
output = "mapblockloader:loader", | ||
recipe = { | ||
{ warp_crystal, retaining_circuit, warp_crystal }, | ||
{ retaining_circuit, titanium_block, retaining_circuit }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have some fun with phlogiston circuits, and phlogiston, xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, what do you think if I place phlogiston crystal in the middle, instead of titanium block and replace retaining circuits with phlogiston ones?
core.register_node("mapblockloader:creative_loader", { | ||
short_description = "Creative Mapblock Loader", | ||
description = | ||
"Creative mapblock Loader\nKeeps the mapblock it's located active.\nDoes not consume power.\nInfinite.\nOnly for admins.\nAnd mods maybe\nMods like moderators, not ones that are written in Lua.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\nOnly for admins.\nAnd mods maybe\nMods like moderators, not ones that are written in Lua.
remove this... or no... just replace it with
description = "Forces the mapblock to be always loaded"
assuming that is what it does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted it to be some sort of a joke :D
meta:set_string("infotext", "Working") | ||
end, | ||
on_destruct = function(pos, _) | ||
core.forceload_free_block(pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If deleted with something like worldedit, there could be some trouble, is this acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Desour told me about it a bit higher.
I think that I should save coordinates of new loaders and when a new mapblock emerges, check if there are loaders in this mapblock (using mod storage) and if there is, check the position and if there's no loader, do not force load the mapblock and remove the pos from the storage.
Would that be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
- Use transient forceload only.
- Store list of forceloader node positions in memory (or pos hash set). And store it in modstorage.
- On server startup (i.e. in core.after(0)), load the list. And go through all the positions. If there is a foreceloder (you can load the block with
core.load_area
, and then useget_node
), forceload the block, otherwise remove the nodepos (and log to warning or info). - Also do this check of the positions also every now and then.
- Use an lbm, or an abm with a long interval, to add missing forceloader positions to the list.
- Note that forceloading does counting, see
builtin/game/forceloading.lua
. So match every load with a free, or else there will be conflicts with other mods.
Btw. to avoid players forceloading the whole map, you could also make a per-player limit of forceloaders (with a priv (also for singleplayer) to allow infinitely many). And you could make it so that foreceloaders of a player only work while the player is on the server (store owner(s) in node meta, and store pos list per player in worlstorage).
mods/mapblockloader/logic.lua
Outdated
|
||
mapblockloader.on_punch = has_vizlib and function(pos, _, player) | ||
if not player or player:get_wielded_item():get_name() ~= "" then | ||
-- Only show loaded area when using an empty hand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should remove this check, dunno why vizlib has the tradition of doing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I just copied it from jumpdrive :)
@@ -0,0 +1,64 @@ | |||
sbz_api.register_stateful_machine("mapblockloader:loader", { | |||
short_description = "mapblock Loader", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mapblock Loader
@@ -0,0 +1,57 @@ | |||
mapblockloader.action = function(pos, _, meta, supply, demand) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when it gets moved by a piston or jumpdrive? (sbz development is so joyful i know)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rn it seem that the only problem that previous forceload will persist forever..
I think that if I'll implement the tracking mechanism described above, it will be OK with pistons and jumpdrives
if demand + power_needed > supply then | ||
meta:set_int("prev_state", meta:get_int("running")) | ||
meta:set_int("running", 0) | ||
return power_needed, false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add core.forceload_free_block here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is setting running to 0 and on the next called action
it will unload the mapblock
function mapblockloader.create_config() | ||
file = io.open(mapblockloader.config_path, "w") | ||
local default_config = { | ||
mapblockloaders_per_player = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably notify the user of this limit in some way, do that in the questbook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is up to server owner and yea, I'll write about it in QB
@@ -0,0 +1,15 @@ | |||
|
|||
# Questline: Mapblock loaders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An entire questline? just for the mapblock loader?
mods/sbz_progression/quests.lua
Outdated
@@ -15,6 +15,7 @@ local quest_files = { | |||
"Pipeworks_and_fluid_transport", | |||
"Reactor", | |||
"Jumpdrive", | |||
"Mapblock_loaders", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct would be "Mapblock_loader", as this would try to unlock you the questline (Side note: you shouldn't make your own questline just for mapblock loaders)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I thought about where to place it and Didn't really find a good place. I see that the 'Completionist' questline is a kind of questline for everything that didn't match for any of the others :D
… the Completionist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also resolve conversations that have been solved
or can only zander do that, i dunno github
{ warp_crystal, retaining_circuit, warp_crystal }, | ||
{ warp_crystal, phlogiston_circuit, warp_crystal }, | ||
{ phlogiston_circuit, phlogiston_crystal, phlogiston_circuit }, | ||
{ warp_crystal, phlogiston_circuit, warp_crystal }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but maybe something expensive at the center, maybe a warp device?
@@ -1,7 +1,7 @@ | |||
core.register_node("mapblockloader:creative_loader", { | |||
short_description = "Creative Mapblock Loader", | |||
description = | |||
"Creative mapblock Loader\nKeeps the mapblock it's located active.\nDoes not consume power.\nInfinite.\nOnly for admins.\nAnd mods maybe\nMods like moderators, not ones that are written in Lua.", | |||
"Forces the mapblock to be always loaded", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forces the mapblock that it is inside of to always be loaded
If this PR won't have the requested changes/testing, i may implement something like this on my own (where i believe i know all the ways a player could seriously mess with a forceloader) |
This PR has two versions of a chunk loader: a normal one and for admins which does not require power and works infinitely.
The crafting recipe for the normal one is:
Where M is a sbz_resources:matter_blob and # is empty.
I think that the recipe must be changed and it was added purely as a test and it is very unbalanced, obviously.
The chunk loader consumes 25 power.
Textures are CC-BY-SA 4.0 (they are a slight rework of the textures from the original mod I was basing on.)