-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some #farm
improvements
#4561
base: 1.21.1
Are you sure you want to change the base?
Some #farm
improvements
#4561
Conversation
I noticed the original |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that I have pending comments on this. The review is probably incomplete, but I'll submit it now before I forget it again.
Having FarmProcess
enable and disable itself in a kind of paused-but-not-quite state looks really weird. Is there a reason to not have it return a DEFER
pathing command while it is waiting for crops?
/** | ||
* Farm whitelist, only interact with crop that is on the {@link #farmWhitelist} list | ||
*/ | ||
public final Setting<Boolean> farmEnableWhitelist = new Setting<>(false); | ||
|
||
/** | ||
* Crop block that Baritone is allowed to farm and collect | ||
* {@link #farmEnableWhitelist} | ||
*/ | ||
|
||
public final Setting<List<Block>> farmWhitelist = new Setting<>(new ArrayList<>(Arrays.asList( | ||
Blocks.WHEAT, | ||
Blocks.POTATOES, | ||
Blocks.CARROTS | ||
))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whitelist should default to allowing everything so farmEnableWhitelist
is not needed.
Also I'm not sure whether we really should use Block
rather than a new enum type for farm targets.
@@ -1397,7 +1436,7 @@ public final class Settings { | |||
/** | |||
* Desktop notification on farm fail | |||
*/ | |||
public final Setting<Boolean> notificationOnFarmFail = new Setting<>(true); | |||
public final Setting<Boolean> notificationOnFarmProcess = new Setting<>(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you rename this?
public Long getTime() { | ||
return time; | ||
} | ||
|
||
public void setTime(Long time) { | ||
this.time = time; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these public?
added options:
changed:
#farm <range>
partially resolves #396 #893 #3537
closes #775 #2315