Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move transfer logic into the specific Unit subclasses #474

Closed
bevans2000 opened this issue Oct 20, 2021 · 31 comments
Closed

Move transfer logic into the specific Unit subclasses #474

bevans2000 opened this issue Oct 20, 2021 · 31 comments
Labels
improvement

Comments

@bevans2000
Copy link
Member

Describe the bug
The Unit Transfer logic sits inside the Unit class currently but it has logic branching to support different sub-types of Unit; but not all. For example Settlements & Building should not be transferable.

It would make the logic cleaner if we move Unit specific transfer logic into the specific Unit subclasses. For example Person would have a dedicated transfer method handling just Person transfers. The same would need to be done to Vehicle & Equipment.
@bevans2000 bevans2000 mentioned this issue Oct 20, 2021
Closed
@mokun mokun mentioned this issue Oct 22, 2021
Closed
mokun added a commit that referenced this issue Oct 24, 2021
@mokun
Separate logics into each respective type of units
ea78728 
r6332
2021-10-24

## ISSUE
1. #474
2. #482

## CHANGE
1. Separate logics of setContainerUnit, getNewLocationState
   and associated methods to each type of Unit.
2. Replace all instanceof with getUnitType() in Unit.
mokun added a commit that referenced this issue Oct 24, 2021
@mokun
Separate unit transfer logics
13e86bd 
r6333
2021-10-24

## ISSUE
1. #474
2. #482

## CHANGE
1. Separate transfer logics into each respective type of units
@bevans2000
Copy link
Member Author

Why does the transfer method have a parameter for the origin? The origin should be the current container.

@mokun
Copy link
Member

mokun commented Oct 24, 2021

Why does the transfer method have a parameter for the origin? The origin should be the current container.

That's a good point.

Since transfer() is inside the Unit class. It should be able to use getContainerUnit() to get the origin

@bevans2000
Copy link
Member Author

@mokun There is a problem with Missions. The Person transferred to a Vehicle at the start of the Mission are still attached to the Settlement. So during the Mission they continue to walk around and do Task in the Settlement, Person is not transferring correctly.

@bevans2000
Copy link
Member Author

Actually this might be a problem with how Tasks are started. I suspect some Tasks that are Settlement based and not checking the person is in a Settlement before starting.

@mokun
Copy link
Member

mokun commented Oct 25, 2021

btw, how we define if a vehicle is in a settlement or not is crucial.

I have this scenario:

Person P gets inside Vehicle V while parked inside a garage building G in Settlement S

If V exits G and still parked at S, and I do a P.getSettlement(), what do you want to be the result ?

Should it be S or null ?

@mokun
Copy link
Member

mokun commented Oct 25, 2021

Also, what do you want P.getTopContainerUnit() to be ?

@bevans2000
Copy link
Member Author

There is a problem with how Tasks are selected. Some of the Tasks that can only be done in a Settlement use isInSide as a condition to calculate a probability. The predicate isInside correctly returns true now which Tasks like ReviewMissionPlan can be started when a Person is inside a Vehicle. Clearly these Tasks should be using isInSettlement. For ReviewMissionPlan the Person walks to an Adminstration building which means they leave the Vehicle.
My guess is previously there was a bug that meant isInside would return false in a Vehicle? Hence so this problem didn't occur.

bevans2000 added a commit that referenced this issue Oct 25, 2021
@bevans2000
Stop Settlement based task should not be started in a Vehicle
247ca35 
The problem is the some Settlement only task uses isInside as a
precondition but it should be isInSettlement. This means that without
this a Person could try and Walk out of a Vehicle to a building.

Changes:
- ReviewMissionPlan & Meta use isInSettlement
- ReviewJobReassignment & Meta use isInSettlement

#474
mokun added a commit that referenced this issue Oct 25, 2021
@mokun
Remove the origin param in transfer()
5b6967f 
r6342
2021-10-25

## ISSUE
1. #483 (comment)
2. #474

## NEW
1. 3. Add codes in transfer() in Robot.
2. Add robotsWithin list in Settlement.

## CHANGE
1. Remove the origin param in transfer() in Person, Vehicle,
   Robot, Equipment.
2. Remove transfer() in Unit.
@mokun
Copy link
Member

mokun commented Oct 25, 2021 •
edited
Loading

Some of the Tasks that can only be done in a Settlement use isInSide as a condition to calculate a probability.

ok. So we'll need to fix isInside() or how LocationStateType should be derived when being inside a vehicle.

I suppose it makes more sense to allow a person such as a commander or a chief inside a vehicle to review and approve a mission plan, or else there isn't much to do inside a vehicle during the trip.

@bevans2000
Copy link
Member Author

bevans2000 commented Oct 26, 2021 •
edited
Loading

Some of the Tasks that can only be done in a Settlement use isInSide as a condition to calculate a probability.

ok. So we'll need to fix isInside() or how LocationStateType should be derived when being inside a vehicle.

I suppose it makes more sense to allow a person such as a commander or a chief inside a vehicle to review and approve a mission plan, or else there isn't much to do inside a vehicle during the trip.

No 'isInside' is correct. It means I am in an air environment. The problem is some Tasks were not taking being in a Vehicle into account. I've fixed the 2 I found.

@bevans2000
Copy link
Member Author

isInSide is correct and doesn't need changing. However there is an odd problem where sometimes Persons still have a Building attribute defined even thought they are not in a Settlement. I think in the Person.transfer we should clear the buildingLocation property when leaving a Settlement. We don't want to be explicitly setting everywhere in the code base as the transfer logic is the correct place for this to sit. When a Person leaves a Settlement they leave any Buildings as well.

@bevans2000
Copy link
Member Author

I'm looking into this tonight. I think the issue might be a race condition in MarsSurface.

bevans2000 added a commit that referenced this issue Oct 26, 2021
@bevans2000
Corrected transferred Person still attached to a Building
f2e69b2 
Changes:
- Person.transfer clears the building id when leaving a Settlement
- MarsSurface added some synch blocks around the shared lists
- BuildingAirlock relies on the transfer method to correctly move Person
- Location description logic moved out of SimLogger. Supprots future
reuse to remove LocationTag
#474
@mokun
Copy link
Member

mokun commented Oct 27, 2021

might be a race condition in MarsSurface.

So what race condition do you experience ?

A settlement runs on a thread. A person.timePassing() would be run from his settlement's timePassing() in the same thread, right ?

and why it only seems to affect it when loading from a saved sim ?

@bevans2000
Copy link
Member Author

No it wasn't a race condition. But there some warnings still that Units can not be retrieved from a container.

@mokun
Copy link
Member

mokun commented Oct 27, 2021

No it wasn't a race condition. But there some warnings still that Units can not be retrieved from a container.

Can you copy and paste here next time you see it ?

@bevans2000
Copy link
Member Author

The problem occurs because of 2 coding issues that we can fix:

  1. Callers are calling setContainerUnit directly instead of going through the transfer logic. So some callers are changing the container without de-registering the Unit. We should make setContainerUnit a protected method and fore all callers to obey the contract of using a transfer. Exception is contractors to setup the Unit.
  2. The transfer logic needs to be more defensive; if it can not retrieve a Unit from the current container it should still be allowed to add it to the new one. Not adding it to the new container has a larger impact and is just continuing the problem. If we tolerant failed retrieval; then at least transfer will try and correct the situation going forward.
    What do you think?

@mokun
Copy link
Member

mokun commented Oct 27, 2021

Sure. I'm committing some simple changes to see if it fairs better. If not, we'll need to do it more defensively, like you suggested.

@mokun
Copy link
Member

mokun commented Oct 27, 2021

I see 3 potential issues :

  1. when a person or robot trying to board a rover parked inside a garage and leaving a rover parked inside a garage.

  2. Setting currentBuildingInt = -1 may not work since LifeSupport class still has that person.

  3. May need to rework on when and how canExitAirlock() in Walk is being used.

@bevans2000
Copy link
Member Author

Ok. We need to make sure we don't have logic spread around the classes. The new Inventory approach has simplified and consolidated things a lit so we don't want to lose that. For clearness I see only the transfer method should be allowed to change a container.
Why does life support have it's own list of Persons?

mokun added a commit that referenced this issue Oct 27, 2021
@mokun
Explicitly remove a person/robot from a building
2ac042d 
r6352
2021-10-27

## ISSUE
1. #474

## FIX
1. Use removePersonFromBuilding() within transfer() in Person and
   Robot.
2. Check if a transfer is successful or not in various methods.
@mokun
Copy link
Member

mokun commented Oct 27, 2021

Why does life support have it's own list of Persons?

LifeSupport class has always had a collection of occupants for a long time by previous developers.

If there's no longer any merit to do so, we can certainly change that.

@bevans2000
Copy link
Member Author

Yes I don't like the top level container aspects. If a Person is in a Vehicle then they delegate to the vehicle for isInSettlement, getSettlement etc ? Then as the vehicle is transferred out of a Settlement or building the Person inherits the move ?

mokun added a commit that referenced this issue Oct 28, 2021
@mokun
Check if a unit is in settlement
b70e018 
r6358
2021-10-28

## ISSUE
1. #482
2. #474

## CHANGE
1. Add isInSettlement() in Person, Robot, Vehicle.
mokun added a commit that referenced this issue Oct 29, 2021
@mokun
Correct vehicle disembarking at a settlement
4b3def6 
r6363
2021-10-29

## ISSUE
1. #474

## FIX
1. Revise disembark() in RoverMission and DroneMission
   - Adopt the use of transfer().
2. Revise the robot's action in enteringRoverInsideGaragePhase()
   and exitingRoverGaragePhase() in Walk.
mokun added a commit that referenced this issue Oct 29, 2021
@mokun
Correct transfering a person when disembarking at a settlement
d4e31ab 
r6364
2021-10-29

## ISSUE
1. #474

## CHANGE
1. Revise rescueOperation() and disembark() in RoverMission.

## NEW
1. Add findNumContainersOfType() in EquipmentOwner.

## FIX
1. Avoid NPE in UnloadVehicleEVA and LoadVehicleEVA.
mokun added a commit that referenced this issue Oct 29, 2021
@mokun
Do NOT unload EVA suit
693740e 
r6365
2021-10-29

## ISSUE
1. #474

## CHANGE
1. Ensure each crew member in the vehicle has an EVA suit to wear
   - Revise unloadingPhase() in UnloadVehicleEVA.
2. Ensure getSettlement() in various class return object
   correctly.
mokun added a commit that referenced this issue Oct 29, 2021
@mokun
Count num of EVA suits correctly
2d3f2bc 
r6366
2021-10-29

## ISSUE
1. #474

## CHANGE
1. Count the total num of EVA suits in a mission vehicle.
   - Add hasBaselineNumEVASuit() in RoverMission.
   - Include suits
     - stored in vehicle
	 - on mission members' inventory
	 - worn by him/her
mokun added a commit that referenced this issue Oct 30, 2021
@mokun
Fix rover embarking
06279f0 
r6367
2021-10-30

## ISSUE
1. #474

## CHANGE
1. Revise performEmbarkFromSettlementPhase() in RoverMission.

## FIX
1. Avoid NPE in walkingPhase() in WalkOutside.
@bevans2000
Copy link
Member Author

Not sure what is happening but after my recent pull I have 2 issues:

  1. Sometimes (not often) the EVA Suit is no longer reporting in the inventory when someone is on an EVA; it was previously
  2. Missions are stuck in the Disembarking phase with no leaving the vehicle

I had a couple of warnings about not being able to retrieve in the transfer method I think. Do we still enforce a retrieval before the transfer? I feel the setContainer method has to be protect to stop logic bypassing the transfer.

@bevans2000
Copy link
Member Author

Also my simulation run has now stalled going into an infinite loop with a Persons walking task so the pulse is stuck just repeating the same sequence of steps.

@mokun
Copy link
Member

mokun commented Oct 30, 2021

I revised is settlement() and getSettlement() recently and it seems to have affected walking

@mokun
Copy link
Member

mokun commented Oct 30, 2021 •
edited
Loading

With regard to the EVA suit transfer issue, it may have to do with the recent change in the embarking and disembarking methods in RoverMission

@bevans2000
Copy link
Member Author

I think the problem is isInSettlement in Person., The scenario I have is a Person is in a Vehicle parked at a Settlement but outside. The person returns true for isInSettlment because the Vehicle returns true;; this means that they can now do inside Settlement tasks like tend the greenhouse which in turn prevents them from walking out of the Vehicle.
I'm thinking a rework on the transfer to clean it up and simplify it but do it on a branch.

@bevans2000
Copy link
Member Author

Maybe if you are in a Vehicle even if it is parked in a Garage you can't be in the settlement until you leave the Vehicle. That would work as that is what we had before. Remind me, why was it a problem with the previous way?

@bevans2000
Copy link
Member Author

Yes I can see teh changes to Person.getSettlement & Person.isInSettlement maybe causing problems. But I can't see how Persons leave the vehicle in a disembark phase.; do you know how it is triggered?

@bevans2000
Copy link
Member Author

I have no idea how MissionMember are meant to leave the Vehicle during the disembarking phase. Seems that the unloading cannot start unless everyone is out of the vehicle but only no one is ever told to leave. I think the mission member should be assigned an UnloadVehcile task as part of disembarking phase. Make sense but can't understand why it isn't there already.
I'm going to be a bit short of time over the next few days so won't be able to help much.

@mokun
Copy link
Member

mokun commented Oct 30, 2021

Maybe if you are in a Vehicle even if it is parked in a Garage you can't be in the settlement until you leave the Vehicle. That would work as that is what we had before. Remind me, why was it a problem with the previous way?

My bad ! I'm now correcting getNewLocationState() in Vehicle to distinguish between returning the LocationStateType of INSIDE_SETTLEMENT and WITHIN_SETTLEMENT_VICINITY.

This way, LocationStateType will be reliable again and isInSettlement() and getSettlement() can depend on this enum value.

@mokun
Copy link
Member

mokun commented Oct 30, 2021

how MissionMember are meant to leave the Vehicle during the disembarking phase.

Let's discuss this portion further in #488

@mokun mokun mentioned this issue Oct 30, 2021
Closed
mokun added a commit that referenced this issue Oct 30, 2021
@mokun
Distinguish if a unit is in a vehicle not in garage
b11c3e8 
r6368
2021-10-30

## ISSUE
1. #474
2. #488

## FIX
1. Revise getSettlement() and isInSettlement() in Person,
   Robot, Equipment.
mokun added a commit that referenced this issue Oct 30, 2021
@mokun
Remove people from settlement as vehicle removed from garage
8acb4c9 
r6369
2021-10-30

## ISSUE
1. #474
2. #488

## FIX
1. Revise transfer() in Vehicle.
2. Remove calling transfer() from two methods in Walk
   - exitingRoverGaragePhase
   - enteringRoverInsideGaragePhase
3. Correct addToGarageBuilding() in BuildingManager.
4. Avoid ClassCastException in unloadingPhase()
   in UnloadVehicleEVA.

## NEW
1. Add removeFromGarage() in BuildingManager.
   - remove people and robots from settlement when called.

## CHANGE
1. Revise from 2 methods in RoverMission to incorporate
   removeFromGarage() in BuildingManager.
   - performEmbarkFromSettlementPhase()
   - disembark()
2. Incorporate removeFromGarage() in various other classes.
mokun added a commit that referenced this issue Oct 31, 2021
@mokun
Correct TestContainment
9c1d29f 
r6370
2021-10-30

## ISSUE
1. #474
2. #488

## CHANGE
1. Revise TestContainment to accommodate new way of removing
   a person from settlement from previous commit.
2. Add calling kotlin-compiler and kotlin-scripting-compiler
   explicitly.

## FIX
1. Add assertVehicleGaraged() in TestContainment.
mokun added a commit that referenced this issue Nov 1, 2021
@mokun
Correct occupants entering/exiting vehicle in garage
0b61454 
r6386
2021-10-31

## ISSUE
1. #474
2. #488
3. #482

## FIX
1. Revise two methods in Walk
   - exitingRoverGaragePhase
   - enteringRoverInsideGaragePhase
mokun added a commit that referenced this issue Nov 1, 2021
@mokun
Correct transfer for garaged vehicle. Adopt UnitSet
cc82dac 
r6387
2021-10-31

## ISSUE
1. #479
2. #474
3. #488
4. #482

## NEW
1. Add isInAGarage() in Vehicle.

## CHANGE
1. Adopt UnitSet for Rover and MarsSurface.
2. Revise addVehicle() and removeVehicle() in
   VehicleMaintenance.
   - Transfer robot and person.
   - Revise removeFromGarage() in BuildingManager.
mokun added a commit that referenced this issue Nov 1, 2021
@mokun
Attempt to correct entering/exiting garaged vehicle
7af22cc 
r6389
2021-10-31

## ISSUE
1. #474
2. #488
3. #482

## FIX
1. Revise addVehicle() and removeVehicle() in VehicleMaintenance.
2. Revise two methods in Walk
   - exitingRoverGaragePhase
   - enteringRoverInsideGaragePhase
mokun added a commit that referenced this issue Nov 2, 2021
@mokun
Return false on person's isInSettlement when inside vehicle
0b3ef27 
r6404
2021-11-02

## ISSUE
1. #474
2. #482
3. #488

## CHANGE
1. Modify person's isInSettlement
2. Revise getLocalRobotGroup() in Robot and
   getLocalGroup() in Person.
mokun added a commit that referenced this issue Nov 5, 2021
@mokun
Set coordinates on belonging units
356ad70 
r6410
2021-11-04

## ISSUE
1. #474
2. #482
3. #488

## NEW
1. Add setCoordinates() in Vehicle, Person, Robot.
mokun added a commit that referenced this issue Dec 9, 2021
@mokun
Fix transferring to a building. Fix code smells
1103370 
r6559
2021-12-08

### ISSUE
1. #474

### FIX
1. Revise transfer() in Person and Robot
2. Fix code smells.
mokun added a commit that referenced this issue Dec 15, 2021
@mokun
andle a person inside a vehicle in a garage
61156f8 
r6590
2021-12-14

### ISSUE
1. #474

### NEW
1. Add isInVehicleInGarage() in Worker.

### CHANGE
1. Modify getBuilding() in BuildingManager to see if a person
   is inside a vehicle in a garage.
   - Address "Not currently in a building" message.
2. Modify '# of Crew' label to '# Crew Boarded' in
   TabPanelCrew.
3. Increase the height of the member table in TabPanelCrew.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mokun @bevans2000