Add a config option to change the max water pumping distance.#3964
Add a config option to change the max water pumping distance.#3964AlexIIL merged 3 commits intoBuildCraft:8.0.xfrom
Conversation
AlexIIL
left a comment
There was a problem hiding this comment.
Looks good! Few minor things that probably want changing.
|
|
||
| propPumpMaxDistance = config.get(general, "pumpMaxDistance", 64); | ||
| propPumpMaxDistance.setMinValue(16).setMaxValue(256); | ||
| propPumpMaxDistance.setComment("How far, in minecraft blocks, should pumps reach in water?"); |
There was a problem hiding this comment.
Probably change comment to "How far, in minecraft blocks, should pumps reach in fluids?"
| outer: while (!nextPosesToCheck.isEmpty()) { | ||
| List<BlockPos> nextPosesToCheckCopy = new ArrayList<>(nextPosesToCheck); | ||
| nextPosesToCheck.clear(); | ||
| final int maxLengthSquared = BCCoreConfig.pumpMaxDistance * BCCoreConfig.pumpMaxDistance; |
There was a problem hiding this comment.
Move this up out of the while loop - just under the boolean isWater variable assignment.
| none.setTo(propMarkerMaxDistance); | ||
|
|
||
| propPumpMaxDistance = config.get(general, "pumpMaxDistance", 64); | ||
| propPumpMaxDistance.setMinValue(16).setMaxValue(256); |
There was a problem hiding this comment.
any reason the minimum is 16? if we are making this a config option anyways it might be best to set the min at 1 or 2, could be usefull for modpackmakers
There was a problem hiding this comment.
I'd assume it was copied from above, and I'm not really sure what the actual minimum and maximum values should be.
My concern is primarily confusion: if someone doesn't know that the maximum distance has been changed, then having a really small distance might be annoying...
There was a problem hiding this comment.
it could indeed be annoying if the config is set to small but i think it's more the responsability of whoever changes it to keep things sane
as for the max: 256 is 16 chunks so quite a huge distance, @Yuki-nyan you mentioned performance impacts, how much of an impact is it when you set it to things like 100, 200 and the max?
There was a problem hiding this comment.
I've only tried it in single player at 128 and 256 so far, the former was playable with around 50 pumps (with occasional freezes of a second or two) but I had allocated 64GB of RAM to Minecraft and it was using over half of that. With it set to the maximum and having the same amount of pumps, the game spends more time frozen than playable.
There was a problem hiding this comment.
i assume those pumps where all above an ocean? we might limit things at 128 to protect servers as they don't have that kind of RAM and more often then not everyone sets up his own lava pump in the nether so this would quickly put a lot of stress on servers for RAM usage
There was a problem hiding this comment.
You would assume correctly 😆 I agree lowering the limit to 128 is probably a sensible idea for the majority of players / server admins.
There was a problem hiding this comment.
Sounds like we might want to look into optimising pumps to be able to handle larger distances properly. Although that's pretty minor, so I'll merge it in if you change the limit to 128.
|
forgot to put this here earlier, if you are on the discord (or join in the future) you can get the contributor role for this PR, to get it reply here with your discord username and discriminator |
|
If I'm not too late for this, it's @SiliconExarch Ի#3368 Pumps seem to handle large distances much better now FWIW! |
I decided to add this option for myself since I wanted to drain an Ocean Monument using a bunch of pumps around the perimeter and a finite water mod - it makes this possible but has a huge effect on performance when increased much above 64 as you might expect. If you don't want to merge this I'll just maintain my own fork 😄