Skip to content

feat: add minimum stock to the postbox #10651

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

Draft
wants to merge 4 commits into
base: version/main
Choose a base branch
from

Conversation

mcmanustfj
Copy link
Contributor

@mcmanustfj mcmanustfj commented Feb 9, 2025

Testing

  • Yes I tested this before submitting it.
  • I also did a multiplayer test.

image

Structural notes that might need changed:

  • As noted in discord, I copied a bunch of code, which doesn't feel great. WindowPostBox extends AbstractWindowRequestTree extends AbstractWindowSkeleton, but the existing code for rendering those tabs is in AbstractModuleWindow extends AbstractWindowSkeleton. I copied the code to WindowPostBox because I don't see a good way of refactoring the inheritance to make it work without multiple inheritance (which isn't in Java)

  • The Minimum Stock module gets the amount of minimum stocks based on the hut level. I added a check to use '1' if the building getMaxBuildingLevel() == 0.

Review please

@CLAassistant
Copy link

CLAassistant commented Feb 9, 2025

CLA assistant check
All committers have signed the CLA.

@Thodor12
Copy link
Contributor

Thodor12 commented Feb 9, 2025

The Minimum Stock module gets the amount of minimum stocks based on the hut level. I added a check to use '1' if the building getMaxBuildingLevel() == 0.

I wouldn't touch this behavior. Otherwise level 0 huts could start having min stocks which is not allowed.

I'd rather create a specific implementation of the min stock module and have a separate module producer.

Use this new implementation for the postbox only, and give a way to change the implementation for getting the limit.

@mcmanustfj
Copy link
Contributor Author

The Minimum Stock module gets the amount of minimum stocks based on the hut level. I added a check to use '1' if the building getMaxBuildingLevel() == 0.

I wouldn't touch this behavior. Otherwise level 0 huts could start having min stocks which is not allowed.

I'd rather create a specific implementation of the min stock module and have a separate module producer.

Use this new implementation for the postbox only, and give a way to change the implementation for getting the limit.

This only uses 1 if the max hut level = 0, which AFAIK is only stash and postbox? not that stash would get minimum stock

@mcmanustfj mcmanustfj force-pushed the feature/postbox-minimum-stock branch from 3fe3bbc to 97ee572 Compare February 10, 2025 15:14
@mcmanustfj mcmanustfj force-pushed the feature/postbox-minimum-stock branch from 97ee572 to 12f0546 Compare February 10, 2025 15:16
final Random random = new Random(building.getID().hashCode());
int offset = 0;
final int iconXOffset = xOffset+5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting

@@ -32,12 +33,19 @@ public abstract class AbstractModuleWindow extends AbstractWindowSkeleton implem
* @param building {@link AbstractBuildingView}.
* @param res the resource String.
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline (docs should be attached to class)

public AbstractModuleWindow(final IBuildingView building, final String res)
{
super(res);
this.buildingView = building;
placeWindowModuleViews(this, building, -20, this.mc);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline

placeWindowModuleViews(this, building, -20, this.mc);

}
// need mc as its own arg because it's protected in Pane
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline between methods


}
// need mc as its own arg because it's protected in Pane
static void placeWindowModuleViews(AbstractWindowSkeleton window, final IBuildingView building, int xOffset, Minecraft mc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give methods the correct access modifier, if this is intended to be called by subclasses only, it should be protected


return (int) (building.getBuildingLevel() * STOCK_PER_LEVEL * increase);
return (int) (buildingLevelOr5 * STOCK_PER_LEVEL * increase);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, please remove this change and override getBuildingLevel() of the postbox instead

@mcmanustfj
Copy link
Contributor Author

I think after discussion in discord, it was decided that y'all don't want this implemented without changing the layout and adding an upgradable tier for the postbox or similar, which I'm not willing to do at this time. I welcome someone else picking this up, though

@mcmanustfj mcmanustfj marked this pull request as draft May 21, 2025 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants