Divide and conquer the Inventory class #463
Comments
|
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. |
|
The current Inventory pays 3 roles
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:
|
r6240 2021-10-03 ## DISCUSSION 1. #463 2. #293 (reply in thread) ## NEW 1. Adopt a lightweight approach to storing resoures for EVASuit - Remove the need of having an Inventory instance 1. Testing microstream - Add microstream related maven artifacts. - Create an instance of DataRoot and EmbeddedStorageManager in SimulationConfig. - Save using microstream's EmbeddedStorageManager - a collection of settlement - located in .mars-sim/saved/ ## CHANGE 1. Modify EVASuit and related class that used to use Inventory for storing and retrieving resources.
Sounds good. Please do go ahead with this plan to simplify the object graph of Inventory |
|
After this commit, currently, there's a bug that creates the following weight issue on the EVA suit : 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 ! |
|
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. |
|
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. |
Changes: - The resource mass record in the suit keep growing instead of one per resource type. - Override the getMass method for Equipment to use stored mass - Inventory transfer put Unit back into source if the new Owner can not hold it. This prevents a "leak" of Equipment. #463
|
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 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 |
okay. I'll switch to Map
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.
Sure. It shouldn't need to be recalculated. |
I should have said I did a commit earlier that fixed the EVA suit problem. |
|
Yes. Thanks for fixing that. |
|
Calculating mass is still an issue from time to time as follows: |
|
I received this error : Can we get a printout of all the unit id and resource id via a command in the console ? |
|
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 . |
|
Looking at the stack trace the question is why are we updating the Inventory if we are just getting the capacity? |
You can enable diagnostics via the diagnostics command in the Console. This will dump to a text file whenever a Unit is created. |
|
I think this may have something to do with Equipment registering themselves in the Settlement in the constructor. Generally a quote I found is |
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. |
|
Yes it's a beast to understand. |
|
One big problem I see is the use of |
Changes: - Remove registering the Equipment in teh constructor as Unit is not fully constructed - Move the registration of new Equipment to the EquipmentFactory class - Added an Equipment command to show used Equipment of a Unit, Person, Vehicle & Settlement. - Reused the minimumPath method of Exploration in the Collection missions. #463
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;
} |
|
Super. .My thoughts were the methods that use the explicit subclasses like Bag make it harder to understand. Certainly increases the coupling. The |
r6262 2021-10-07 ## ISSUE 1. #463 ## NEW 1. Add containsEquipment(), containsVehicleType() and containsEVASuit() in Inventory. ## CHANGE 1. Rename containsUnitClass() containsEquipment(int typeID) and Rework to use EquipmentType enum. 2. Rework containsUnit()
r6263 2021-10-07 ## ISSUE 1. #463 ## FIX 1. Rework containsUnit() in Inventory to fix maven test.
|
In terms of transfer, we have the following scenarios: For Person
For Equipment
For Vehicle
|
I'm beginning to wonder if it has to do with the recent changes made in recognizing MarsSurface as an Unit inside Inventory. |
|
Or it's because in retrieveUnit() in Inventory, containedUnitIDs doesn't really contain the person's instance to begin with. So retrieveUnit() always return false. |
|
oh I found the reason. It's missing an else clause in transfer() in Unit. Committing the change in a moment. |
r6309 2021-10-19 ## ISSUE 1. #463 ## FIX 1. Modify retrieveUnit() in Inventory - Ensure calling setContainerUnit(null) when only retrieving a unit. 2. Modify transfer() in Unit.
|
Good. |
So ,looking at these tables it feels like we could move the transfer to the specific Unit classes. Also we can consider Person and Vehicle the same if we use EquipmentOwner. I will commit my changes later today. |
|
btw, one change to make is adding that an equipment can be transferred between two persons, between two robots and between a person and a robot. |
True. then it will need less if-else conditional clauses in each type of unit. |
|
I create a new issue for this #474 |
|
Most things seem to be working OK. I've updated the Inventory & Equipment console commands so they work in all cases now. In particular you can use these to see the Outstanding issues:
I have created a new Project for the Inventory rework so we can start creating smaller Issues than the current #463 mammoth. |
Problem relates to logic being based on the heavyweight Inventory class and not the smaller EquipmentOwner. Changes: - Console inventory command correctly handles Person, Robot, Vehicle & Settlement now - Change Tasks & Mission classes that do EVAs from Vehicles - EatDrink Task changes as food comes from different classes now - Mind stopped it removing mission members based on Coordinates. Coordinates can only be trusted when on the MarsSurface. - Temporarily stopped ConsolidateContainers running in Vehicles. - DigLocal Tasks were not checking the Person was in a Settlement. Due to other problem this allowed Vehicle people to jump out and start digging. mars-sim#463
Changes: - Removed ResourceHolder.hasResource method as not needed - Fixed problem in UnitTests - Fixed concurrent modification as People/Equipment disembark by holding a private list copy mars-sim#463
r6317 2021-10-20 ## ISSUE 1. #463 ## FIX 1. Modify canCollectMinerals() in CollectMinedMinerals 2. Modify doffEVASuit() in EnterAirlock. 3. Modify canStoreUnit() in Inventory. 4. Modify repairWithParts() in Malfunction.
r6318 2021-10-20 ## ISSUE 1. #475 2. #463 ## CHANGE 1. Convert various class to supporting Robot's use of EquipmentInventory. 2. Modify getMalfunctionables() in MalfunctionFactory. ## FIX 1. Correct isFullyUnloaded() in UnloadVehicleEVA. 2. Modify GoodsManager, Trade, TradeUtil, Delivery, and DeliveryUtil to quit calling Inventory for vehicle.
r6321 2021-10-21 NOTE: fail a lot of maven tests. Await for further debugging. ## ISSUE 1. #463 2. #479 ## NEW 1. Add new methods in Settlement - findNumContainersOfType() - getVehicleTypeList() - getEquipmentTypeList() - findNumVehiclesOfType() 2. Add new methods in EquipmentInventory - findAllContainers() - addCapacity() - removeCapacity() - setResourceCapacity() - setResourceCapacityMap() - getRemainingCargoCapacity() 3. Add new methods in MicoInventory - addCapacity() - removeCapacity() 4. Add methods in ResourceHolder - hasItemResource() ## CHANGE 1. Remove getInventory() from Malfunctionable. 2. Building to implement Malfunctionable. 3. Revise getAmountResourceRemainingCapacity() in EquipmentInventory.
r6331 2021-10-24 ## ISSUE 1. #476 2. #463 ## NEW 1. Add calling fireUnitUpdate() in Unit and MicroInventory. - INVENTORY_RESOURCE_EVENT - INVENTORY_STORING_UNIT_EVENT - INVENTORY_RETRIEVING_UNIT_EVENT 2. Add getUnit() in Loggable. Q: should Unit implement Loggable ? ## FIX 1. Avoid ConcurrentModificationException in MicroInventory - Revise updateAmountResourceTotalMass() 2. Avoid ConcurrentModificationException in EquipmentInventory - Revise getAmountResourceStored() to use Iterator 3. Add capacity for initial resources in Storage. - Call setResourceCapacityMap()
|
Should be close some of these Issues now? Stability seems to be returning |
|
Sure. If we see any anomaly, let's open a new thread. Closing it. |
Is your feature request related to a problem? Please describe.
Describe the solution you'd like
-- Unit class such as Person, Robot, Vehicle, Equipment, etc.
-- Amount Resources
-- Item Resources
Describe alternatives you've considered
Additional context
The text was updated successfully, but these errors were encountered: