Divide and conquer the Inventory class

mokun commented 3 years ago

Is your feature request related to a problem? Please describe.

Simplify the Inventory class

Describe the solution you'd like

We may separate the use of Inventory to various class. Currently, it holds -- Unit class such as Person, Robot, Vehicle, Equipment, etc. -- Amount Resources -- Item Resources

Describe alternatives you've considered

Leave it alone at the expense of high cpu utilization

Additional context

Previous threads of issues and discussions :

mokun commented 3 years ago

@bevans2000

Now, I start working on removing the use of Inventory for EVASuit.

It could use a lightweight form of inventory, much like those container types (Bag, Specimen Box, etc.).

Currently, EVASuit tracks only 3 specific resources (oxygen, water and co2) and so it doesn't need to own a full blown Inventory class.

bevans2000 commented 3 years ago

The current Inventory pays 3 roles

Holder of AmountResources - references by AmountResource
Holder of Equipment - referenced by EquipmentType
Holder of Parts - referenced by Part

Each of these could be represented as a dedicated Interface with separate implementations. Then certain Unit sub classes would implement the appropriate interface, e.g Equipment would just be HolderAmountResource. A settlement would be all 3.; however the implementation for Settlement would also record demand automatically.

Each of the interfaces follow the same behaviour and would have just:

How much is stored?
Remove an amount - return how much was actually removed.
Add an amount - return how much was actually added
Transfer and amount to another instance of the same interface.
Drop the exception throwing if there is not enough stored; method return how much was actually transferred

mokun commented 3 years ago

Then certain Unit sub classes would implement the appropriate interface, e.g Equipment would just be HolderAmountResource

Sounds good. Please do go ahead with this plan to simplify the object graph of Inventory

mokun commented 3 years ago

@bevans2000 ,

After this commit, currently, there's a bug that creates the following weight issue on the EVA suit :

00-Adir-01:558.743 (Severe) Inventory : [New Pompeii - Pranav Jai] EVA Suit 063 - Had a mass of 1401.6 kg - too much to put on 'Pranav Jai' to carry. Remaining Gen Cap : 59.9 kg. (Gen Cap : 60.0 kg)
00-Adir-01:558.743 (Severe) Inventory : [Unknown] EVA Suit 063 - Could not be stored.
00-Adir-01:558.884 (Severe) Inventory : [New Pompeii - Aradhana Sandeep] EVA Suit 064 - Had a mass of 1331.4 kg - too much to put on 'Aradhana Sandeep' to carry. Remaining Gen Cap : 64.9 kg. (Gen Cap : 65.0 kg)
00-Adir-01:558.884 (Severe) Inventory : [Unknown] EVA Suit 064 - Could not be stored.

It is caused by the method canStoreUnit(Unit unit, boolean allowDirty) in Inventory. Somehow it reports that an EVA suit weighs too much and can't be put on. Everything looks okay to me and I have no idea why.

Can you take a look and figure out why ? Thanks !

mokun commented 3 years ago

Also, the getMass() in Unit looks normal as follows:

    /**
     * Gets the unit's mass including inventory mass.
     * 
     * @return mass of unit and inventory
     * @throws Exception if error getting the mass.
     */
    public double getMass() {
        double invMass = 0;
        if (this instanceof Equipment) {
            if (this instanceof EVASuit)
                invMass = ((EVASuit)this).getStoredMass();
            else
                invMass = ((Equipment)this).getStoredMass();
        }
        else {
            if (inventory == null) { 
                logger.severe(this + ": inventory is null.");
            }
            else 
                inventory.getTotalInventoryMass(false);
            if (invMass == 0)
                return baseMass;
        }

        return baseMass + invMass;
    }

Guess there's a better way to code getMass() to avoid all those instanceof.

It would call getStoredMass() in EVASuit to obtain the weight of the just resources in an EVA suit.

The baseMass of an empty EVAsuit was computed at the start of the sim as the summation of all the parts the compose the suit, namely, the array EVASUIT_PARTS in ItemResourceUtil.

bevans2000 commented 3 years ago

Sure. I think this code highlights the problem we face where there are a lot of instanceof to cater for Units that do not have an Inventory.

mokun commented 3 years ago

Sure. I think this code highlights the problem we face where there are a lot of instanceof to cater for Units that do not have an Inventory.

Yea. Say, after we fix this problem for handling resources for EVA suit , we can for sure take a closer look at optimizing it and getting rid of those instanceof's.

bevans2000 commented 3 years ago

OK so I looked at this. The problem is in setQuantity you use 'add' instead of 'set' so it keeps adding to the quantities instead of updating the existing value. The was also a mistake in the Unit.getMass method where it was not using the mass of the inventory. There are still a few warnings about a EVA Suit being too heavy to store but these cases look like the Person is just carrying too much stuff as the EVA Suit mass is reasonable. For what I can see these problem cases are when the Person has a small capacity; maybe it is something wrong with the change to do carrying capacity related to age?

Having the separate parallel lists is a dangerous approach because they can get out of synch. Why not just bit the bullet and use a Map? It will be efficient. Each Map Entry would be a simple POJO holding the current amount & capacity.

What I can see is the work you've done in EVASuit & Equipment containers is very similar. One just holds a fixed set of resources and the Containers just hold a single resource. I think what could be done to improve this is to create a new class ResourceStorageImpl that encapsulates the logic which moves the resource logic out of EVASuit into a new place. The supported Resources would be passed in the constructor. Hence the same class can also be used for the Bag, Box, etc BUT only a single resource is used. I would then put an interface ResourceStorage holding the key methods to retrieve & store and then implement this on the appropriate Units. Then many of the instanceof code can be replaced by checking against the interface instead which is always more acceptable. Then the Units would use a delegate pattern to use the ResourceStorageImpl class. This class would be easy to Unit test in isolation as well which is a bonus. The other improvement I would make to the simple class approach is to hold the total mass as a property instead of recalculating each time. The mass will be constant and only change when resources are added or removed. The size of an extra double is nothing compared to the processing avoided by not continuously adding up the volumes. Asking for current volumes is more frequent than adding/removing. Then further ahead; this new class could be backfilled into the existing Inventory to replace logic there.

mokun commented 3 years ago

Having the separate parallel lists is a dangerous approach because they can get out of synch. Why not just bit the bullet and use a Map? It will be efficient. Each Map Entry would be a simple POJO holding the current amount & capacity.

okay. I'll switch to Map

create a new class ResourceStorageImpl that encapsulates the logic which moves the resource logic I would then put an interface ResourceStorage holding the key methods to retrieve & store

I'll let you lead the way to implement this correctly. Is it okay ?

I suppose this can be extend to hold Item Resources as well in future.

We can use this in replace of the Inventory inside the vehicle as vehicle allows a few other resources to be stored.

The other improvement I would make to the simple class approach is to hold the total mass as a property instead of recalculating each time

Sure. It shouldn't need to be recalculated.

bevans2000 commented 3 years ago

Having the separate parallel lists is a dangerous approach because they can get out of synch. Why not just bit the bullet and use a Map? It will be efficient. Each Map Entry would be a simple POJO holding the current amount & capacity.

okay. I'll switch to Map

create a new class ResourceStorageImpl that encapsulates the logic which moves the resource logic I would then put an interface ResourceStorage holding the key methods to retrieve & store

I'll let you lead the way to implement this correctly. Is it okay ?

I suppose this can be extend to hold Item Resources as well in future. Well I would go simple and have a different class and interface for Item Resources. This would remove the need to always check the range if the resource id before using it. Same pattern as Resources.

We can use this in replace of the Inventory inside the vehicle as vehicle allows a few other resources to be stored. Yes. But small steps are needed here since Inventory is so key at the moment.

I should have said I did a commit earlier that fixed the EVA suit problem.

mokun commented 3 years ago

Yes. Thanks for fixing that.

mokun commented 3 years ago

Calculating mass is still an issue from time to time as follows:

00-Adir-10:333.979 (Info) EnterAirlock [x264] : [0.00 N 0.00 W] Ray Bradbury - Could not enter airlock to EVA Airlock 3. Already full.
00-Adir-10:333.979 (Info) EnterAirlock [x264] : [0.00 N 2.23 W] Anna Svoboda - Could not enter airlock to EVA Airlock 3. Already full.
00-Adir-10:338.873 (Info) PlanMission : [New Plymouth] Sarah Thompson - Looking into the settlement's mission needs.
00-Adir-10:338.873 (Info) DeliveryMeta : [New Plymouth] Drone 001 available for delivery mission.
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60052 type:EQUIPMENT baseID:3753
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60052 type:EQUIPMENT baseID:3753
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60052 type:EQUIPMENT baseID:3753
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60052 type:EQUIPMENT baseID:3753
00-Adir-10:338.873 (Severe) UnitManager$SettlementTask : Problem with pulse on Europa: null
java.lang.NullPointerException
    at org.mars_sim.msp.core.Inventory.updateUnitTotalMassCache(Inventory.java:3131)
    at org.mars_sim.msp.core.Inventory.getUnitTotalMassCache(Inventory.java:3118)
    at org.mars_sim.msp.core.Inventory.getUnitTotalMass(Inventory.java:1373)
    at org.mars_sim.msp.core.Inventory.getGeneralStoredMass(Inventory.java:1155)
    at org.mars_sim.msp.core.Inventory.getRemainingGeneralCapacity(Inventory.java:1165)
    at org.mars_sim.msp.core.Inventory.updateAmountResourceCapacityCache(Inventory.java:2729)
    at org.mars_sim.msp.core.Inventory.getAmountResourceCapacityCacheValue(Inventory.java:2637)
    at org.mars_sim.msp.core.Inventory.getAmountResourceCapacity(Inventory.java:634)
    at org.mars_sim.msp.core.Inventory.getAmountResourceRemainingCapacity(Inventory.java:726)
    at org.mars_sim.msp.core.foodProduction.FoodProductionUtil.getFoodProductionProcessItemValue(FoodProductionUtil.java:194)
    at org.mars_sim.msp.core.structure.goods.GoodsManager.getPartFoodProductionProcessDemand(GoodsManager.java:3084)
    at org.mars_sim.msp.core.structure.goods.GoodsManager.getPartFoodProductionDemand(GoodsManager.java:3048)
    at org.mars_sim.msp.core.structure.goods.GoodsManager.determineItemResourceGoodValue(GoodsManager.java:2586)
    at org.mars_sim.msp.core.structure.goods.GoodsManager.determineGoodValue(GoodsManager.java:459)
    at org.mars_sim.msp.core.structure.Settlement.timePassing(Settlement.java:1087)
    at org.mars_sim.msp.core.UnitManager$SettlementTask.call(UnitManager.java:704)
    at org.mars_sim.msp.core.UnitManager$SettlementTask.call(UnitManager.java:1)
    at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    at java.base/java.lang.Thread.run(Thread.java:829)

00-Adir-10:338.873 (Info) MeteorologyStudyFieldWork [x58] : [Drifter] Eloise Belanger - clearDown::totalCollected: 0.0
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - food (balue: 42.4)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - gasket (balue: 13.0)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - gasket - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 8356.1  totalDelivered: 127.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - lubricant bottle (balue: 9.7)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - lubricant bottle - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 1084.7  totalDelivered: 22.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - counter pressure suit (balue: 9.6)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - counter pressure suit - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 1513.7  totalDelivered: 31.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - eva antenna (balue: 7.5)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - eva antenna - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 541.9  totalDelivered: 15.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - power transistor (balue: 1.6)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - power transistor - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 35.6  totalDelivered: 4.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - Buyer: New Plymouth  Seller: Europa  buyerLoadValue: 16047.3  deliveryList: [gasket, counter pressure suit, food, lubricant bottle, power transistor, bag, eva antenna]
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - pulp (balue: 190.7)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - concrete (balue: 41.7)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - electrical wire (balue: 31.3)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - electrical wire - Buyer: Europa  Seller: New Plymouth  totalBuyingValue: 325678.6  totalDelivered: 488.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - wire connector (balue: 12.3)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - wire connector - Buyer: Europa  Seller: New Plymouth  totalBuyingValue: 2571.9  totalDelivered: 9.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - Buyer: Europa  Seller: New Plymouth  buyerLoadValue: 341105.2  deliveryList: [wire connector, concrete, bag, electrical wire, pulp]
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 372295.1
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Load Credit: 390671.9
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 20097.2
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Load Credit: 16124.9
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Selling Credit Home: 372295.1
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Selling Credit Remote: 390671.9
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Buying Credit Home: 20097.2
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Buying Credit Remote: 16124.9
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] totalProfit: 22349.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Estimated Revenue: 22349.0
00-Adir-10:338.873 (Info) VehicleMission : [New Plymouth] Drone 001 - tripDistance: 2562.0 km   fuel economy: 42.6 km/kg   composite fuel margin factor: 1.0   Amount of fuel: 60.2 kg
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 1.6
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Estimated Cost: 1.6
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Delivering to Europa  best profit: 22347.4
00-Adir-10:338.873 (Info) DeliveryMeta : [New Plymouth] Sarah Thompson - DeliveryMeta's probability: 398.0
00-Adir-10:338.873 (Info) MissionManager : [New Plymouth] Sarah Thompson - Mission Delivery with probability=398.0
00-Adir-10:338.873 (Info) MissionManager : [New Plymouth] Sarah Thompson - Mission Mineral Exploration with probability=98.0
00-Adir-10:338.873 (Info) Mission : [New Plymouth] Sarah Thompson - Began organizing a Delivery mission.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - food (balue: 42.4)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - gasket (balue: 13.0)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - gasket - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 8356.1  totalDelivered: 127.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - lubricant bottle (balue: 9.7)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - lubricant bottle - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 1084.7  totalDelivered: 22.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - counter pressure suit (balue: 9.6)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - counter pressure suit - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 1513.7  totalDelivered: 31.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - eva antenna (balue: 7.5)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - eva antenna - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 541.9  totalDelivered: 15.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - power transistor (balue: 1.6)  Buyer: New Plymouth  Seller: Europa.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - power transistor - Buyer: New Plymouth  Seller: Europa  totalBuyingValue: 35.6  totalDelivered: 4.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - Buyer: New Plymouth  Seller: Europa  buyerLoadValue: 16047.3  deliveryList: [gasket, counter pressure suit, food, lubricant bottle, power transistor, bag, eva antenna]
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - pulp (balue: 190.7)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - concrete (balue: 41.7)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - electrical wire (balue: 31.3)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - electrical wire - Buyer: Europa  Seller: New Plymouth  totalBuyingValue: 325678.6  totalDelivered: 488.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - wire connector (balue: 12.3)  Buyer: Europa  Seller: New Plymouth.
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - wire connector - Buyer: Europa  Seller: New Plymouth  totalBuyingValue: 2571.9  totalDelivered: 9.0
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Drone 001 - Buyer: Europa  Seller: New Plymouth  buyerLoadValue: 341105.2  deliveryList: [wire connector, concrete, bag, electrical wire, pulp]
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 372295.1
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Load Credit: 390671.9
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 20097.2
00-Adir-10:338.873 (Info) DeliveryUtil : [Europa] Load Credit: 16124.9
00-Adir-10:338.873 (Info) DeliveryUtil : [New Plymouth] Load Credit: 1.6
00-Adir-10:338.873 (Info) Mission : [New Plymouth] Sarah Thompson - Delivery #1 tagged with 'Not enough members recruited'.
00-Adir-10:338.873 (Info) Mission : [New Plymouth] Sarah Thompson - Ended Delivery #1.
00-Adir-10:338.873 (Info) Mission : [New Plymouth] Sarah Thompson - Disbanded mission member(s) : [Sarah Thompson]
00-Adir-10:341.431 (Warning) Unit : EVA Suit 017 expected pulse #27795 but received 27796
00-Adir-10:341.431 (Warning) Unit : EVA Suit 018 expected pulse #27795 but received 27796

mokun commented 3 years ago

@bevans2000

I received this error :

00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755
00-Adir-10:338.873 (Warning) UnitManager : Unit not found 60084 type:EQUIPMENT baseID:3755

Can we get a printout of all the unit id and resource id via a command in the console ?

bevans2000 commented 3 years ago

Sure. I have seen something similar but rarely. It happens when the simulation adds Equipment. There is a flaw in the logic of events as well. The Unit constructor fire an event but of course the object is not fully constructed so a NPE appears in the monitor tool. That is why I logged #309 .

bevans2000 commented 3 years ago

Looking at the stack trace the question is why are we updating the Inventory if we are just getting the capacity?

bevans2000 commented 3 years ago

Sure. I have seen something similar but rarely. It happens when the simulation adds Equipment. There is a flaw in the logic of events as well. The Unit constructor fire an event but of course the object is not fully constructed so a NPE appears in the monitor tool. That is why I logged #309 .

You can enable diagnostics via the diagnostics command in the Console. This will dump to a text file whenever a Unit is created.

bevans2000 commented 3 years ago

I think this may have something to do with Equipment registering themselves in the Settlement in the constructor. Generally a quote I found is The rules of Java state that you should never pass 'this' to another method from its constructor because the this object is not fully constructed It is certainly a race condition somewhere. I'm going to have a play with some ideas.

mokun commented 3 years ago

Looking at the stack trace the question is why are we updating the Inventory if we are just getting the capacity?

Great question !

It was calling getUnitTotalMass(), which in turns called getUnitTotalMassCache()

I suppose enabling the cache of the mass would mean faster reading but less accurate result.

It's just too many things to decipher in the Inventory class.

bevans2000 commented 3 years ago

Yes it's a beast to understand.

bevans2000 commented 3 years ago

One big problem I see is the use of Class as a filter on getting particular types of Equipment out of the inventory. This is not good practice since it create a very strong coupling between Inventory and the Equipment subclasses. In fact I think we could drop the Box, Bag etc classes and replace with a generic Container class that use the EquipmentType as a selector. All containers essentially function the same so there is no need to have separate classes.

mokun commented 3 years ago

drop the Box, Bag etc classes and replace with a generic Container class that use the EquipmentType as a selector. All containers essentially function the same so there is no need to have separate classes.

Sure.

I'm working on replacing the following method in Inventory :

    /**
     * Checks if any of a given class of unit is in storage.
     * 
     * @param unitClass the unit class.
     * @return if class of unit is in storage.
     */
    public boolean containsUnitClass(Class<? extends Unit> unitClass) {
        boolean result = false;
        // Check if unit of class is in inventory.
        if (containsUnitClassLocal(unitClass)) {
            return true;
        }
        return result;
    }

bevans2000 commented 3 years ago

Super. .My thoughts were the methods that use the explicit subclasses like Bag make it harder to understand. Certainly increases the coupling.

The EquipmentFactory is another related bad example of using Class. So I may try and remove the dependencies of the Class references and replacement with EquiomentType. Then dropping in a generic Container will be easier.

mokun commented 3 years ago

@bevans2000

I changed out most of the instanceof in the last commit, plus a few other minor things.

Somehow it can't pass the LoadVehicleTest in maven.

Can you take a look to see what went wrong ?

mokun commented 3 years ago

Somehow it can't pass the LoadVehicleTest in maven.

If I run this test by itself, it passes. Don't know why it can't pass when running together with other tests.

mokun commented 3 years ago

I also keep having these logs :

00-Adir-01:451.631 (Severe) Inventory [x232] : [Tian Cheng] grey water could not be totally retrieved. Remaining: 0.02295414699169962
00-Adir-01:459.964 (Severe) Inventory [x7989] : [Schiaparelli Point] grey water could not be totally retrieved. Remaining: 0.024622359810129044

They obviously came from retrieveAmountResource(int resource, double amount) in Inventory:

                if (remainingAmount > SMALL_AMOUNT_COMPARISON) {
                    logger.log(getOwner(), Level.SEVERE, 30_000,
                            ResourceUtil.findAmountResourceName(resource)
                            + " could not be totally retrieved. Remaining: " + remainingAmount);
                }

bevans2000 commented 3 years ago

@bevans2000

I changed out most of the instanceof in the last commit, plus a few other minor things.

Super.

Somehow it can't pass the LoadVehicleTest in maven.

Can you take a look to see what went wrong ? Sure. My guess is the tests are not isolated and one is affecting another.

bevans2000 commented 3 years ago

I also keep having these logs :

00-Adir-01:451.631 (Severe) Inventory [x232] : [Tian Cheng] grey water could not be totally retrieved. Remaining: 0.02295414699169962
00-Adir-01:459.964 (Severe) Inventory [x7989] : [Schiaparelli Point] grey water could not be totally retrieved. Remaining: 0.024622359810129044

They obviously came from retrieveAmountResource(int resource, double amount) in Inventory:

              if (remainingAmount > SMALL_AMOUNT_COMPARISON) {
                  logger.log(getOwner(), Level.SEVERE, 30_000,
                          ResourceUtil.findAmountResourceName(resource)
                          + " could not be totally retrieved. Remaining: " + remainingAmount);
              }

I gave alse been seeing this message.

bevans2000 commented 3 years ago

So the problem with the UnitTests is interesting. The unit tests sets the Settlement capacity for each Resource to match what has to be pre-loaded. When the same resource is preloaded twice (required & optional) this is causing a rounding issue such that the capacity is just short of the required space.; it is the smallest fraction that can be presented in a double. This is just a natural consequence of us using doubles for absolute arithmetic because they do not do that. I have increased the Settlement capacity by a small margin to give some headroom. Ideally we should replace the use of double with integers and have all masses in terms of grams, This will be more accurate and quicker as integer arithmetic is faster. However than could be a big piece of work. But I also removed a lot of unused methods from the Inventory class as well. I noticed that the capacity cache & stored cache are pre-filled with all Resource IDs. This will consume memory so I have removed the preloading; i.e. only resourced supported and recorded now not all.

mokun commented 3 years ago

So, after fixing the rounding error, we still have the following:

00-Adir-01:362.771 (Severe) Inventory [x2417] : [Boring Town] black water could not be totally retrieved. Remaining: 8.074323062406902E-4
00-Adir-01:406.026 (Severe) Inventory [x2890] : [Boring Town] solid waste could not be totally retrieved. Remaining: 2.3357863144819966E-4
00-Adir-01:449.286 (Severe) Inventory [x2518] : [Boring Town] black water could not be totally retrieved. Remaining: 8.074323062406902E-4
00-Adir-01:492.835 (Severe) Inventory [x3475] : [Boring Town] grey water could not be totally retrieved. Remaining: 5.712625590074991E-5
00-Adir-01:536.780 (Severe) Inventory [x1180] : [Boring Town] grey water could not be totally retrieved. Remaining: 1.3250707262841497E-5
00-Adir-01:580.064 (Severe) Inventory [x3734] : [Boring Town] regolith could not be totally retrieved. Remaining: 0.004037161531203451

I'm beginning to suspect it may have to do with commenting out those cache methods in the last few commits.

mokun commented 3 years ago

btw, I'm converting those parallel lists in EVASuit to maps as the next incremental change.

This way, it will facilitate the creation of the future MicroInventory class.

bevans2000 commented 3 years ago

So, after fixing the rounding error, we still have the following:
00-Adir-01:362.771 (Severe) Inventory [x2417] : [Boring Town] black water could not be totally retrieved. Remaining: 8.074323062406902E-4
00-Adir-01:406.026 (Severe) Inventory [x2890] : [Boring Town] solid waste could not be totally retrieved. Remaining: 2.3357863144819966E-4
00-Adir-01:449.286 (Severe) Inventory [x2518] : [Boring Town] black water could not be totally retrieved. Remaining: 8.074323062406902E-4
00-Adir-01:492.835 (Severe) Inventory [x3475] : [Boring Town] grey water could not be totally retrieved. Remaining: 5.712625590074991E-5
00-Adir-01:536.780 (Severe) Inventory [x1180] : [Boring Town] grey water could not be totally retrieved. Remaining: 1.3250707262841497E-5
00-Adir-01:580.064 (Severe) Inventory [x3734] : [Boring Town] regolith could not be totally retrieved. Remaining: 0.004037161531203451
I'm beginning to suspect it may have to do with commenting out those cache methods in the last few commits. I did have a quick look at this. It only occurs with the ResourceProcess task. I think in the Task it check to see how much resource is available but inside the Inventory I think it is using a different logic which is way there is a difference. The number is too big to be a rounding error. As I said ideally we would cover to grammes and use ints but that is more work.

bevans2000 commented 3 years ago

btw, I'm converting those parallel lists in EVASuit to maps as the next incremental change.

This way, it will facilitate the creation of the future MicroInventory class.

Makes sense. I would use a simple POJO containing stored & capacity as the value in the Map.

mokun commented 3 years ago

It only occurs with the ResourceProcess task.

Yea. it occurs a lot with grey water. Sometimes with black water and regolith. So it would have to do with calling the retrieveAmountResource from those resource processes that happens in every frame.

mokun commented 3 years ago

a simple POJO containing stored & capacity as the value in the Map.

Yes. I'm creating a POJO MicroInventory class as we speak.

bevans2000 commented 3 years ago

Super. You'll be able to unit test this in isolation. Potentially retro-fit it to Inventory as well are replace the heavyweight AmountResoruceStorage potentially.

mokun commented 3 years ago

@bevans2000

I only tried the MicroInventory in EVASuit so far.

Using it in Equipment should be very similar.

The next will be Person and Robot class.

how would you like to handle the storage of Unit in Person ?

Will you agree using MicroInventory for storing resources only ?

What structure would you propose for storing Unit(s) while avoiding the messiness in our dear old Inventory class ?

mokun commented 3 years ago

@bevans2000 , @Urwumpe

Next would be Vehicle.

Eventually we will need to deal with Settlement's storage.

But as you know, currently resources aren't stored inside Building class per se. All resources are simply stored within Settlement's Inventory.

In order to properly track the whereabout of furniture, people, equipment and tools, I imagine we need to have a graphical hierarchy of what units are within what unit...

How would you like to approach that ?

bevans2000 commented 3 years ago

Person and Robots can only hold Equipment so is that something we can exploit.

mokun commented 3 years ago

@bevans2000 ,

I just did the commit of incorporating MicroInventory for all equipment.

However, EVASuit has unresolved issues in storing amount resources.

I'm still wondering why loadEVASuit() in ExitAirlock can't store oxygen and water properly.

Can you figure out what went wrong ?

bevans2000 commented 3 years ago

@bevans2000 ,

I just did the commit of incorporating MicroInventory for all equipment.

However, EVASuit has unresolved issues in storing amount resources.

I'm still wondering why loadEVASuit() in ExitAirlock can't store oxygen and water properly.

Can you figure out what went wrong ?

Sure I'll have a look

bevans2000 commented 3 years ago

The problem is in the overloading of methods and the EVASuit sometimes using the Equipment version of method which means there are 2 MicroInventory objects in play. I'll sort it. Then I would like to replace the various container classes with a single generic Container. This will simplify the code and with MicroInventory those specific container classes offer nothing.

mokun commented 3 years ago

EVASuit sometimes using the Equipment version of method which means there are 2 MicroInventory objects in play

So, that being the case, we could delete the MicroInventory instance in EVASuit ?!

mokun commented 3 years ago

The unit tests sets the Settlement capacity for each Resource to match what has to be pre-loaded. When the same resource is preloaded twice (required & optional) this is causing a rounding issue such that the capacity is just short of the required space.; it is the smallest fraction that can be presented in a double. This is just a natural consequence of us using doubles for absolute arithmetic because they do not do that. I have increased the Settlement capacity by a small margin to give some headroom.

I can't tell if it has to do with double precision because it didn't happen before and why now ?

00-Adir-01:042.768 (Severe) Inventory : [Schiaparelli Point] fish oil could not be totally retrieved. Remaining: 0.25
00-Adir-01:086.965 (Severe) Inventory : [Schiaparelli Point] grey water could not be totally retrieved. Remaining: 2.4980492831699463E-4
00-Adir-01:186.504 (Severe) Inventory [x2465] : [Schiaparelli Point] grey water could not be totally retrieved. Remaining: 0.016027531278877703
00-Adir-01:191.993 (Severe) Inventory [x606] : [Zvezda] grey water could not be totally retrieved. Remaining: 4.6695143149864656E-6

Right now, most of the time, grey water is being the resource that shows up most of the time as not being "totally retrieved".

But fish oil has the largest amount

mokun commented 3 years ago

Person and Robots can only hold Equipment so is that something we can exploit.

Yes. To do so, I see we need to

add storing item resources in MicroInventory
add a mechanism for storing a list of equipment on Person and Robot.
modify Inventory to detect if the owner is Person or robot and switch to using MicroInventory instead.
create an instance of MicroInventory in Person and Robot.

Are you any concern you foresee ?

mokun commented 3 years ago

btw, you mentioned making micro inventory loosely coupled by using interface and adding a concrete implementation of that interface, right ?

mokun commented 3 years ago

And we'll need to resolve these types of negative "remaining general capacity" msg:

00-Adir-01:124.816 (Severe) Inventory : [Asimov Base - Gladys Kelly] Bag 011 - Had a mass of 0.1 kg - too much to put on 'Gladys Kelly' to carry. Remaining Gen Cap : -19.7 kg. (Gen Cap : 40.0 kg)
00-Adir-01:124.816 (Warning) DigLocalIce : [Asimov Base] Gladys Kelly - Strangely unable to carry an empty bag for ice.
00-Adir-01:124.816 (Info) DigLocalIce : [Asimov Base] Gladys Kelly - No bags for ice are available.

bevans2000 commented 3 years ago

EVASuit sometimes using the Equipment version of method which means there are 2 MicroInventory objects in play

So, that being the case, we could delete the MicroInventory instance in EVASuit ?!

But we need remove all overloaded methods in EVAsuit not just the instantiation. All EVAsuit should do with the MicroInventory is to define the capacity it limits. Equipment should handle the management of the Mircoinventory.

mokun commented 3 years ago

But we need remove all overloaded methods in EVAsuit

But to do so, we'll need to reconcile the difference between how EVASuit recognize one of the 3 amount resources versus how Equipment select only one amount resource.

In future, I suppose we can make a Large Bag suitable for storing ONLY ONE type of amount resource, OR for storing a few different types of item resources.

When that happen, we'll need to modify those methods again in Equipment.

bevans2000 commented 3 years ago

Person and Robots can only hold Equipment so is that something we can exploit.

Yes. To do so, I see we need to

add storing item resources in MicroInventory

add a mechanism for storing a list of equipment on Person and Robot.

modify Inventory to detect if the owner is Person or robot and switch to using MicroInventory instead.

create an instance of MicroInventory in Person and Robot.

Are you any concern you foresee ?

Can containers hold Parts ? No, so we should have a separate class for holding Parts otherwise we will be going back down the Inventory route of a class having multiple. I look at it this way:

Equipment - hold just Resources, Containers only a single resource
Person - hold Units and Parts
Robot - hold Parts
Vehicle- hold Resources, Units and Parts
Settlements - hold all 3 but records the demand So I think we need another Inventory class to handle parts/items and apply it as above. Same for Units, but we serialised it as integers.

mokun commented 3 years ago

but I was going to say that holding amount resources aren't much different from holding item resources.

so may be we can have one just type of micro inventory for holding both.

This last commit aims at modifying micro inventory to store both types of resources.

I only had to add a few similar methods and an extra int quantity in inner class ResourceStored.

So for Settlement and Vehicle and may be Person, it's good to have just one micro inventory that can hold these two different resource types, rather than having two separate type of micro inventory for each resource type, you know.

mars-sim / mars-sim

Divide and conquer the Inventory class #463