shapez.io
shapez.io copied to clipboard
tobspr-games
Fix processor charge handling to avoid losing time
This changes the item processor's charge handling code so that it doesn't wait for processed items to finish ejecting before it starts processing the next charge, as discussed in PR #995.
It also fixes the handling of bonus time so that it is applied to the next charge that is processed, instead of to the next charge that is added. (This is not the same charge when a charge is added before the current one is finished processing.)
It does not attempt to fix the case of bonus time being applied to a charge that is added significantly later, as that was not considered a big issue and a simple fix might also affect a charge that happens on the very next tick, which I don't think would be quite right. So I decided to leave that for later.
Fixes #842 Closes #995
Looks good! It'll take me some time to test and review to see if all building still work as desired. But the code looks good on a first look
the problem with this is that a backed up building will process its second charge, which is not right
Well, yes, that will happen - though I think it's arguable whether that is right or not. My previous fix attempt didn't process the second charge, but that got rejected in favor of this. (Also, even if not quite right, it might be more right than the current behavior, and thus a step in the right direction.)
In terms of in-game observable behavior, most (but not all) buildings that have been backed up already eject two charges when they get unblocked, which shows that they have already processed more than one charge despite the blockage. This PR thus does not change whether that happens, just increases the amount to three charges for some buildings (e.g. the cutter), while others stay at two (e.g. the extractor), or are increased to two (e.g. the double painter). That last bit is what fixes the throughput issue, since the double painter basically backs up every time it ejects a charge, even if the output belt is empty.
I think having another charge be processed while the output is backed up is equivalent to having a longer output belt, and thus it doesn't really affect the throughput much, especially since it's only an issue when the other end speeds up after having been too slow.
However, if you see a better solution that avoids the problem, I'd like to hear about it. I don't claim that anything I make is perfect.
In terms of in-game observable behavior, most (but not all) buildings that have been backed up already eject two charges when they get unblocked, which shows that they have already processed more than one charge despite the blockage. This PR thus does not change whether that happens, just increases the amount to three charges for some buildings (e.g. the cutter), while others stay at two (e.g. the extractor), or are increased to two (e.g. the double painter). That last bit is what fixes the throughput issue, since the double painter basically backs up every time it ejects a charge, even if the output belt is empty.
As a modder of the game myself, I know the games code pretty well, and the second charge only processes if the last charge has started the animation of ejecting the last item of the previous charge. So buildings do not by default process the second charge, even if it looks like they do. By having it process charges in advance, when a backed up belt gets unbacked it will not clear properly, as the building is processing charges in advance.
I am working on a different rework of the item processor code though, partly involving some of the code in this PR but mainly new stuff.
the second charge only processes if the last charge has started the animation of ejecting the last item of the previous charge.
Yes, I know, but that's why I specified "in-game observable" - from that perspective it doesn't matter whether it's due to the animation or something else, what you see is that if you hook up the output belt last, a while after the others, you get two charges out from the building immediately (one of which was already visible on the output port), then you have to wait for the next one. From the game-code perspective, sure, there it matters, and explains why the in-game behaviour is like it is, with the animation being separate from the actual processing. I just happen to think the in-game perspective is more important here.
Besides, that animation starts immediately as soon as the first charge is done processing (unless the ejector already has a zeroeth charge in it), even if the output belt is blocked (or not connected), which means that the second charge does start processing immediately after the first one. The one that currently doesn't is the third charge. Unless I missed your point entirely here?
By having it process charges in advance, when a backed up belt gets unbacked it will not clear properly, as the building is processing charges in advance.
I'm not sure what you mean by the belt not clearing properly, in particular how that's substantially different from now - at the moment, clearing a backed-up belt allows two charges to come out of the building immediately (technically, one from the ejector and one from the processor). This PR changes that to be three charges, which would only make it take a bit longer to fully clear the belt. Or, come to think of it, it would actually take less time, since that third charge would still appear regardless (even if the input belts were cleared first), just, after being processed instead of already being ready. I guess we might have different definitions of "clear properly"?
I am working on a different rework of the item processor code
I'll just note that I was going for the smallest change that would fix the issue at hand (double painter throughput), with some input from tobspr on how to do it. A larger rework can probably do much better, potentially also fixing other issues at the same time, so I look forward to seeing what you come up with.
As mentioned in #1124#issuecomment-1017933263, the problem this aimed to fix has now been fixed in 1.5.0 (I verified it in both 1.5.1 and current master) without this PR, so I'm closing it as no longer relevant.