New algorithm for Fluid Regulator Keep Exact Mode #1717
Conversation
Fixes #1705, Supersedes #1710 The existing algorithm was buggy and the proposed rework was hard to follow the logic, so I came up with a more straightforward algorithm which should be easier to understand and maintain. I have also created some tests to demonstrate the functionality of the revised algorithm and ensure that the behavior is the same as before, besides the bug.
Exaxxion
requested review from
LAGIdiot,
serenibyss and
ALongStringOfNumbers
| .map(IFluidTankProperties::getContents) | ||
| .filter(Objects::nonNull) | ||
| .filter(fluidTypeFilter) | ||
| .filter(Objects::nonNull) // objects can and have been null here, shut up IntelliJ |
There was a problem hiding this comment.
filter(Objects::nonNull) is being called twice. guess you really hate your nulls (:
There was a problem hiding this comment.
While testing the code I encountered nulls there. Not sure how exactly since I agree with IntelliJ, it shouldn't have been possible at that point. It's possible the accompanying test case was set up wrong at the time and subsequently fixed; that was one of the first methods I wrote while working on this.
There was a problem hiding this comment.
Do you need first nonNull filter shouldn't last one catch everything?
There was a problem hiding this comment.
As I told Proto 15 days ago, I figured I wouldn't have needed it but I got a NPE without it. I could try it again and see if it was an unrelated problem.
|
The exceptions are thrown because those outcomes are exceptional circumstances. Save for some bizarre circumstance like a mod mutating its state from another thread, it clearly indicates a flaw with the implementation of the fluid tank. It is typically safer for system integrity to fail immediately rather than blast the log with error messages (as it may occur every tick) and simply hope for the best. |
There was a problem hiding this comment.
New implementation looks correct and tests looks to confirm it's functionality.
Though I have some comment to code which I would like you to address.
Regarding use of exception I think it is OK in this case as these would be abnormal situation and my corrupt us later.
| .map(IFluidTankProperties::getContents) | ||
| .filter(Objects::nonNull) | ||
| .filter(fluidTypeFilter) | ||
| .filter(Objects::nonNull) // objects can and have been null here, shut up IntelliJ |
There was a problem hiding this comment.
Do you need first nonNull filter shouldn't last one catch everything?
| // expect that 44mB of water and 144mB of lava will be moved | ||
| assertEquals("Wrong fluid amount moved", 44+144, amountTransferred); |
There was a problem hiding this comment.
In this case you should also check content of destination to be really sure that correct amount of both fluids was moved.
| } | ||
|
|
||
| @Test | ||
| public void doKeepExact_respects_transfer_limit() { |
There was a problem hiding this comment.
I think this case can be simplified
Fixes #1705, Supersedes #1716
What:
The existing Keep Exact algorithm was buggy and the proposed fix by ALSON - while it seemed to function fine - was hard to follow the logic due to the way it was written to work without substantially changing the original implementation.
How solved:
I came up with a more straightforward algorithm to completely replace the existing one, in the hopes that it would be easier to understand and maintain.
I have also created some tests to demonstrate the functionality of the revised algorithm and ensure that the behavior is the same as before, besides the bug.
Outcome:
Fluid Regulator Keep Exact mode works properly and the algorithm is simpler so it should be easier to verify and maintain.
Possible compatibility issue:
As currently proposed, a misbehaving fluid tank implementation may cause an exception to intentionally be thrown; this will crash the game but is useful for diagnostic purposes during mod development and is not anticipated to be encountered when interacting with properly implemented code from other mods.