mars-sim / mars-sim

Mars Simulation Project Official Codebase
https://mars-sim.com
GNU General Public License v3.0
101 stars 37 forks source link

NPE in the Weather class #1008

Closed bevans2000 closed 1 year ago

bevans2000 commented 1 year ago

Describe the bug A recent commit means the Weather class uses UnitManager but it is null. The Weather class is initialised before the Unitmanager is created so the reference it uses will also be null. It's odd that this does not fail immediately though. Needs fixing on the 3.6 bugfix as 3.6.1 is impacted.

To Reproduce (Please provide steps) Odd case to reproduce but I think it is an on demand wind speed calculation on the first pulse of a new Sol.

Stacktrace

01-Adir-02:000.150 (Severe) MasterClock : Can't send out clock pulse: 
java.lang.NullPointerException: Cannot invoke "org.mars_sim.msp.core.UnitManager.findSettlement(org.mars.sim.mapdata.location.Coordinates)" because "this.unitManager" is null
        at org.mars_sim.msp.core.environment.Weather.computeWindSpeed(Weather.java:187)
        at org.mars_sim.msp.core.environment.Weather.getWindSpeed(Weather.java:297)
        at org.mars_sim.msp.core.environment.Weather.lambda$addWeatherDataPoint$3(Weather.java:644)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
        at org.mars_sim.msp.core.environment.Weather.addWeatherDataPoint(Weather.java:635)
        at org.mars_sim.msp.core.environment.Weather.timePassing(Weather.java:666)
        at org.mars_sim.msp.core.Simulation.clockPulse(Simulation.java:1405)
        at org.mars_sim.msp.core.time.MasterClock$ClockListenerTask.call(MasterClock.java:648)
        at org.mars_sim.msp.core.time.MasterClock$ClockListenerTask.call(MasterClock.java:605)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)

01-Adir-02:000.301 (Warning) Unit : Schiaparelli Point expected pulse #6545 but received 6546
01-Adir-02:000.301 (Warning) Unit : Hallway 1 expected pulse #6545 but received 6546

Specifications (please complete the following information)

mokun commented 1 year ago

@bevans2000 @Urwumpe

It seems we could do more to improve how we model the dust storm.

First, we'll decouple the need to track each dust storm through its associated settlement.

As a result, we'll need to modify createDustDevils() method in Weather as well.

    /**
     * Checks if a dust devil is formed for each settlement.
     * 
     * @param probability
     * @param L_s_int
     */
    private void createDustDevils(double probability, double ls) {
        // TODO this needs fixing
        Collection<Settlement> settlements = Simulation.instance().getUnitManager().getSettlements();
        for (Settlement s : settlements) {

            if (s.getDustStorm() == null) {
                // if settlement doesn't have a dust storm formed near it yet

                double chance = RandomUtil.getRandomDouble(100);
                if (chance <= probability) {

                    // arbitrarily set to the highest 3% chance (if L_s is 241 or 270) of generating
                    // a dust devil
                    // on each sol since it is usually created in Martian spring or summer day,
                    checkStorm++;

This way, Weather will track DustStorm without having to mess with Settlement.

Second, if we are to track dust storm across the whole planet of Mars, we could come up with a simplified global climate model

Third, each dust storm or dust devil should be able to move around. Its current position is defined by its center coordinate. And its lat and lon changes over time.

Fourth, it can grow and shrink in size. We could do a search on how to do realistic physics modeling but customize it for Mars.

Depending on its size, it could potentially affect multiple settlements that are in close proximity.

Fifth, how would a dust storm affects mission trips and local tasks ? We'll need to consider that. How should damage be modeled and handled ?

bevans2000 commented 1 year ago

Yes, I'm going to make the change as it's a bit of a jumble. To make a cleaner solution isn't much change so I'm going to do it. But what do you want to do about 3.6.1 which is broken because of this? Create a 3.6.2 off the bugfix branch with just this fix?

bevans2000 commented 1 year ago

I've fixed this by changing the timePassing method in Weather such that it is driven by the active storms and not the Settlements. This makes the logic simpler and easier to follow. Also I have added the ability to create a Dust Storm in the Console Weather command when in export mode. This allows some scenarios to be tested/done by the end user. The fix is on the 3.6_bugfix branch and also merged onto back onto master. We should create a 3.6.2 release to include the fix.

mokun commented 1 year ago

But what do you want to do about 3.6.1 which is broken because of this? Create a 3.6.2 off the bugfix branch with just this fix?

I'll let you handle it the way you want.

It's tedious but be sure to update the date and version in the changlog file in the src folder in both mars-sim-main and mars-sim-headless.

mokun commented 1 year ago

Btw, can we rename and refer the current alphanumeric string build as sub-build or something ?

Then we can back to defining build cleanly as a four or five digit number.

bevans2000 commented 1 year ago

This should be part of #1007