Implement resource cooldown for Oil Pump #4010
Conversation
|
Your Pull Request was automatically labelled as: "🎈 Feature" |
Slimefun preview buildA Slimefun preview build is available for testing! https://preview-builds.walshy.dev/download/Slimefun/4010/c3d2469a
|
|
instead of waiting 5 seconds,if no resources are available just shut it down completely? |
This would assume that resources would never exist in that place again, which another addon could potentially change. It's best I think to not change the actual functionality of the machine during a "bug"fix or improvement to it's current functionality. |
|
Personally I feel like there are better ways to address this. A five second cooldown is also just not intuitive for the player. A better solution would be changing both the cargo logic and the get recipe logic. Change the cargo logic to only accept one bucket in each slot at a time. Change the recipe logic to not allow normal buckets to go to the output. |
I believe the reason originally for moving to the output the buckets when no resource was to attempt to prevent even checking resources the next slimefun tick (otherwise it'd always have a bucket and thus always check every tick for every pump). If you automated buckets into the pump it'd just lock up and cause problems. That in itself also fixed potential lag as no room for an output = no check resources. I think this is the best way of doing it while keeping existing reasoning in place for performance. This is not an issue for players in vanilla SF with no addon to modify resources as the only way it'd trigger is if there was no resource - which is the fail condition there (but also the thing that's potentially causing lag). The PR here doesn't really change much other than actually implementing a performance fix that doesn't break a whole slimefun cargo / network storage system by filling it entirely with buckets. Performance fix is only a side effect here |
The point about player intuitiveness still stands. If the worry is performance than there are other ways to address it. But honestly I don't think it puts buckets in the output for performance anyway because.... if you have buckets going in and you are taking the output then it will just loop through anyways and still check for the resource. |
|
And changing the cargo logic to only accept buckets 1 at a time wouldnt break existing networks afaik. I can't see how that would happen just by limiting the amount inputted and stopping outputting them 🤔 |
The looping through is exactly what's solved here. Looping through is what locks the system up, it'll keep putting buckets into the machines and taking them out until a storage is full. This allows autocrafting of buckets to keep happening as there is "room" since they're stored temporarily in the pump itself. Eventually the autocrafting fills up the pumps AND the storage system, and then it's in deadlock where nothing works and has to be manually emptied. The intuitiveness point is null here because it only exists when another precondition is not met (oil existing). Therefore the player by design IS NOT exposed to the 5 second cooldown. For the cooldown to be in effect, there must be no oil. If there is no oil, the pump cannot work anyway, even if it has buckets. If oil comes back from another SF addon replenishing it, then the pumps after a short delay will start working again. This is what currently happens: Allowing buckets into the output is not good in any case because it can be removed, and then cycled around and put back in when the system is balanced. Even if it only accepted 1 bucket, it would still then cycle that 1 bucket around.... thus causing the lag since check oil exists each SF tick etc. |
|
I'm not gonna lie I haven't experienced the problem that's being attempted to solve here anyways. Whenever Ive played you just have a static small storage for the buckets to go into and out of. And you can limit that to whatever amount you want you just gotta be creative. Then there's no deadlock. If we want to make deadlocking extremely hard to achieve/impossible then again I think my proposed solution is better. We remove the looping of buckets through the inputs and outputs. I can't really think of a reason to keep it anyways. |
1 similar comment
|
I'm not gonna lie I haven't experienced the problem that's being attempted to solve here anyways. Whenever Ive played you just have a static small storage for the buckets to go into and out of. And you can limit that to whatever amount you want you just gotta be creative. Then there's no deadlock. If we want to make deadlocking extremely hard to achieve/impossible then again I think my proposed solution is better. We remove the looping of buckets through the inputs and outputs. I can't really think of a reason to keep it anyways. |
|
You said performance but the looping has the same if not worse performance than just not letting the buckets through. |
Hence why the 5 sec cooldown exists - so it doesn't even check the SF resources, and also doesn't put the bucket into output. Could even potentially add a map cache for location -> date millis like Networks has for Quantum caches... that means it wouldn't even need to check blockstorage. I am experiencing this problem, hence why I decided to fix it. I know it's an actual real problem because I am the one actually having to deal with it currently, and it makes me not want to even bother with oil / plastic sheets. It's already annoying since it can't be fully automated, but this just makes it even more annoying to deal with constantly. I'm not trying to improve performance majorly here, or doing it for performance reasons, I am just trying to eliminate the problem that my network is going to get filled with 12,000+ buckets due to some fairly stupid (imo) initial design. |
| ItemStack item = inv.getItemInSlot(slot).clone(); | ||
| inv.replaceExistingItem(slot, null); | ||
| inv.pushItem(item, getOutputSlots()); | ||
| this.setOnResourceCooldown(b); |
There was a problem hiding this comment.
@JustAHuman-xD as you can see here, this PR removes the bucket cycling already, which is what you're suggesting. It's already done.
|
I just don't get the five second delay. Why not just stop the buckets from going into the output, that's my question. |
Because if there's a bucket in the input, and no bucket in the output it'll lookup resources for the current chunk every tick - which causes lag in large numbers. If there is oil, the machine goes to a processing state so isn't checking if the resource exists for that period of time. When there's no oil, essentially every single tick, every single pump performs a lookup of resources for it's chunk. That's the performance problem initially "solved" by moving buckets to the output slot. It didn't really solve it though, just appeared that way when there was no room for buckets to exit the output slots. This PR actually "fixes" the performance problem , while also fixing the buckets in output thing. I'm pretty sure this was also covered in the intial "Proposed Changes" section |
|
I don't think checking resources is laggy at all since it's a quick memory lookup. But as I said previously if it's a performance problem there are other ways to do it. I'm going to check something real quick because I'm pretty sure I know a more intuitive, performant, procedural way to do this. Instead of an arbitrary five seconds. Id also still like to note I'm kind of against this in general as this is a problem already solvable by the player and by doing this we remove that "difficulty" / "critical thinking" necessary |
|
ok, I give up. I'll let someone else fix it. |
|
What I was thinking was caching the fact that the resource was out already and then listening the geo resource generation event. However due to the fact that event isn't fired when resources are added only when initially generated, then we can't use it. I'd like to see the numbers on the geo checks, cause I was under the assumption that it was just a memory check like most custom slimefun world data. |
Description
Currently, Oil Pumps can block entire systems when automated with Buckets, if buckets are not a static quantity (such as autocrafting buckets to fill a barrel or network storage for addons). When there is no oil, this causes the output slots currently to fill with the bucket, which then gets pulled into the storage system again. However, once the bucket goes into the oil pump, there's room in the storage for another to be crafted and as such it's crafted and stored. This means there's now 1 too many buckets to be stored. The same happens again as an oil can fit in the output and eventually you get both output slots filled with buckets with no storage free for them at all. This then prevents the oil pump from working entirely until they're cleaned out.
Proposed changes
I propose adding a cooldown to the oil pump when it finds no resource, such that it does not move the bucket but instead for 5 seconds will not even bother checking the recipes, resource supplies, or anything else. It simply returns fast (or fast enough). This means we no longer need to move the bucket to the output slot for lag prevention (even though this doesn't actually prevent lag really from what I can see, because there will always be more buckets in the input getting ticked).
Related Issues (if applicable)
None
Checklist
NonnullandNullableannotations to my methods to indicate their behaviour for null values