Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a config option to change the max water pumping distance. #3964

Merged
merged 3 commits into from Jan 7, 2018

Conversation

@SiliconExarch
Copy link
Contributor

@SiliconExarch SiliconExarch commented Jan 7, 2018

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 馃槃

Copy link
Member

@AlexIIL AlexIIL left a comment

Looks good! Few minor things that probably want changing.

@@ -177,6 +179,11 @@ public static void preInit(File cfgFolder) {
propMarkerMaxDistance.setComment("How far, in minecraft blocks, should markers (volume and path) reach?");
none.setTo(propMarkerMaxDistance);

propPumpMaxDistance = config.get(general, "pumpMaxDistance", 64);
propPumpMaxDistance.setMinValue(16).setMaxValue(256);
propPumpMaxDistance.setComment("How far, in minecraft blocks, should pumps reach in water?");

This comment has been minimized.

@AlexIIL

AlexIIL Jan 7, 2018
Member

Probably change comment to "How far, in minecraft blocks, should pumps reach in fluids?"

@@ -114,11 +114,12 @@ private void buildQueue() {
outer: while (!nextPosesToCheck.isEmpty()) {
List<BlockPos> nextPosesToCheckCopy = new ArrayList<>(nextPosesToCheck);
nextPosesToCheck.clear();
final int maxLengthSquared = BCCoreConfig.pumpMaxDistance * BCCoreConfig.pumpMaxDistance;

This comment has been minimized.

@AlexIIL

AlexIIL Jan 7, 2018
Member

Move this up out of the while loop - just under the boolean isWater variable assignment.

@@ -177,6 +179,11 @@ public static void preInit(File cfgFolder) {
propMarkerMaxDistance.setComment("How far, in minecraft blocks, should markers (volume and path) reach?");
none.setTo(propMarkerMaxDistance);

propPumpMaxDistance = config.get(general, "pumpMaxDistance", 64);
propPumpMaxDistance.setMinValue(16).setMaxValue(256);

This comment has been minimized.

@AEnterprise

AEnterprise Jan 7, 2018
Member

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

This comment has been minimized.

@AlexIIL

AlexIIL Jan 7, 2018
Member

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...

This comment has been minimized.

@AEnterprise

AEnterprise Jan 7, 2018
Member

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?

This comment has been minimized.

@SiliconExarch

SiliconExarch Jan 7, 2018
Author Contributor

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.

This comment has been minimized.

@AEnterprise

AEnterprise Jan 7, 2018
Member

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

This comment has been minimized.

@SiliconExarch

SiliconExarch Jan 7, 2018
Author Contributor

You would assume correctly 馃槅 I agree lowering the limit to 128 is probably a sensible idea for the majority of players / server admins.

This comment has been minimized.

@AlexIIL

AlexIIL Jan 7, 2018
Member

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.

@AlexIIL
AlexIIL approved these changes Jan 7, 2018
@AlexIIL AlexIIL merged commit e178631 into BuildCraft:8.0.x Jan 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@AEnterprise
Copy link
Member

@AEnterprise AEnterprise commented Jan 13, 2018

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

@SiliconExarch
Copy link
Contributor Author

@SiliconExarch SiliconExarch commented Apr 20, 2020

If I'm not too late for this, it's @SiliconExarch#3368

Pumps seem to handle large distances much better now FWIW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants