Reorganize Inventory code and add Reward Chest

[BenCheung0422]

This depends on #630
Originally, Inventory resolves for both bounded and unlimited inventory types for item list menus and container inventories, but this is not the best design. So, Inventory is instead made into an interface and 2 separate subclasses BoundedInventory and UnlimitedInventory are made for the corresponding situations. That said, BoundedInventory only handle inventories that have a size limit while UnlimitedInventory does not make any constrains on the size limit by default. This makes the code neater without concerning about "different unused fields exist for each circumstance", which shall be regarded as bad practice.
Then, a RewardChest is added to resolve the original problem that a chest is generated each time rewards are sent. Furthermore, world loading used DeathChest for saving inventory-overflowing items, which may not be well for players, so a RewardChest without timer is used instead. Now, the RewardChest cannot be treated as ordinary chest as items cannot be added and the chest is removed when its inventory is empty. This can reduce the chances players getting more survival-usable chests without any consumption of the desired materials. Also, RewardChest provides an interactive menu, unlike DeathChest. Perhaps, DeathChest should also be made with an interactive menu though.

[BenCheung0422]

As dependency #630 is done, this should now be ready.

[Makkkkus]

I don't see the need for this. Reward chest doesn't have any custom functionality other than a different menu title, and bounded and unlimited inventory can be implemented by just using a bool in the inventory class.

Seems like the new classes are created just to use the types, which isn't necessarily the best way to check variant. Like I said, boolean, but enums are also very helpful.

[BenCheung0422]

In terms of code structure design, having a completely unused field under certain configuration does not feel right. maxItem & unlimited (false) and unlimited (true) are mutually exclusive, which Rust enum can specifically solve, but not in Java (similar design can be achieved by using sealed classes actually). It is quite a bad practice/design in high level programming if such thing exists in a single implemented class.

What RewardChest does is to use UnlimitedInventory over BoundedInventory compared to ordinary chest (as well as read-only) and no timer compared to DeathChest, thus designed specifically for sending rewards.

[Makkkkus]

I see no reason why reward chests have to be functionally different than regular chests.

In terms of code structure design, having a completely unused field under certain configuration does not feel right. maxItem & unlimited (false) and unlimited (true) are mutually exclusive, which Rust enum can specifically solve, but not in Java (similar design can be achieved by using sealed classes actually).

I'm not sure I understand what you are trying to convey.

It is quite a bad practice/design in high level programming if such thing exists in a single implemented class.

I'm generally against adding more classes if the required functionality is possible without them. Enums can be used in this specific case, as they are quite powerful in Java, but I'll take your response to the last question into account when you respond.

[BenCheung0422]

When giving rewards, the chests used are supposed not be a part of the rewards, which would break the expectation and balancing instead. Therefore, the reward chests are supposed to be read-only, which would only allow players getting items from but not putting items into, instead of regular chests.

For the part of the classes and enums part, here, there are 2 cases:

• unlimited == true - maxItem is unused and the value of this field does not affect anything, which means it has no use here while both of these fields can be conflict to each other. Therefore, when it is true, a better practice would be "removing the field", but not possible for Java but Rust enums. Sealed classes in Java are designed for this use case.
• unlimited == false - maxItem is definitely used and the value affects a lot with the functionalities.

These two are entirely different cases and ideally should be separated for code design. You can see that there are different numbers of methods implemented in the 2 new subclasses.

It is not barely just "adding new classes" but reorganizing the code structure and design.

[zandgall]

I know I'm a C brained programmer so this might not fit the codebase, but does a single 'maxItem' value, where 'maxItem == -1' or even just 'maxItem == Integer.MAX_VALUE' implies unlimited? Where you can create a constructor that defaults to maxValue == -1 or MAX_VALUE (or something else), and a constructor where maxValue is specified by a parameter?

[BenCheung0422]

I think it is not a recommended or the best practice in Java (while there is null). Also, even for BoundedInventory, it can be extended to be abstract for getMaxSlots that the subclasses can decide the value and the properties (behaviors) of slots, even the design logic of the inventories. In Rust logic, it is a redundant field and Rust Enum could resolve this instead (even for memory usage).

[zandgall]

What would you make BoundedInventory abstract for? imo maxSlots should just be a property, not the return of an abstract method

[BenCheung0422]

The design of the codebase would going towards to way that allowing flexibility and feasibility of moddability. There can be cases that there are variable bounds in some designs, like there can be another case that the inventory is max for another condition. Perhaps isFull makes more sense.

[Makkkkus]

When giving rewards, the chests used are supposed not be a part of the rewards, which would break the expectation and balancing instead. Therefore, the reward chests are supposed to be read-only, which would only allow players getting items from but not putting items into, instead of regular chests.

I disagree with this sentiment fully. I don't think it is unbalanced to give players the chests they find, on the contrary, I believe it is very logical to be able to use the chests you find. On the other hand, empty reward chests in the world with no use will be nothing but an annoyance. Therefore, I believe replacing regular chests with reward chests will make the gameplay worse.

It seems you are trying to fix a problem that does not exist.

The design of the codebase would going towards to way that allowing flexibility and feasibility of moddability. There can be cases that there are variable bounds in some designs, like there can be another case that the inventory is max for another condition. Perhaps isFull makes more sense.

I think setting the maxItems property to null when it is unbounded is reasonable. Better than my first proposal to do that using a boolean. If extensibility is what you want, enums are amazing.

[BenCheung0422]

I don't think it is unbalanced to give players the chests they find, [...]

Note that the rewards chests are used for the quests, not the ones really exist in the levels. Therefore, your disagreement does not hold.

I think setting the maxItems property to null when it is unbounded is reasonable. [...]

maxItems is a primitive type field, therefore it is not a valid thing.

Also, having strictly defined fields that always have @NotNull and @Nullable should be more practical for safe programming, that this project should be going to be (it also helps reducing potential bugs and unnecessary considerations that have to be explicitly stated separately in comments, which could reduce ease of maintenance). Productivity and simplicity is a thing, not reducing amount of code with nonsense. Explicitly categorizing code and sensibly organize code parts can help both maintenance of the core code and even modding.

[BenCheung0422]

I have updated and ruled the usage and implementation of Inventory. It now acts as an item storage space for general purposes, based on the maximum number of item stacks, and the default item maximum count for stackable items.

Then, for special conditional storage space for items, like custom maximum count of items and storage space that is variable to other conditions, slots or a custom implementation is recommended instead. This includes the implementations of slot entries as in #532. A kind of storage space like bundle in Minecraft should not be using this class.

BoundedInventory (abstract) now allows expandable storage space, but not other changes. FixedInventory now acts as the simple version with a fixed inventory size, based on BoundedInventory. UnlimitedInventory still acts as the simple inventory without inventory size limit.

Some information above is already mentioned in the comments added just a moment before.