Fix #3430 - Four pipe beam crash in remove

openstudiocore/src/model/AirLoopHVAC.cpp

@@ -74,6 +74,8 @@
 #include "AirTerminalSingleDuctConstantVolumeCooledBeam_Impl.hpp"
 #include "AirTerminalSingleDuctConstantVolumeFourPipeInduction.hpp"
 #include "AirTerminalSingleDuctConstantVolumeFourPipeInduction_Impl.hpp"
+#include "AirTerminalSingleDuctConstantVolumeFourPipeBeam.hpp"
+#include "AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp"
 #include "AvailabilityManagerAssignmentList.hpp"
 #include "AvailabilityManagerAssignmentList_Impl.hpp"
 #include "AvailabilityManager.hpp"
@@ -407,7 +409,7 @@ namespace detail {
 
       // Reconnect the cloned terminal to the plant loop(s)
 
-      // TODO: (Temporary?) Ugly hack for FourPipeInduction for now, which has both a cooling and heating plantLoop
+      // Special case for FourPipeInduction for now, which has both a cooling and heating plantLoop
       if ( lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_FourPipeInduction ) {
 
         // Safe to directly cast
@@ -421,14 +423,41 @@ namespace detail {
 
         // If the original ATU has a cooling coil, if it's a CoilCoolingWater, and the cooling coil has a plantLoop, reconnect it here
         if (lastAtuFourPipe.coolingCoil()) {
-          if (boost::optional<CoilCoolingWater> _lastCC = lastAtuFourPipe.coolingCoil()->cast<CoilCoolingWater>() ) {
+          if (boost::optional<CoilCoolingWater> _lastCC = lastAtuFourPipe.coolingCoil()->optionalCast<CoilCoolingWater>() ) {
             if (boost::optional<PlantLoop> _coolingPl = _lastCC->plantLoop()) {
               _coolingPl->addDemandBranchForComponent(newAtuFourPipe.coolingCoil().get());
             }
           }
         }
 
-        // TODO: Another ugly hack for CooledBeam, which isn't a HVAComponent but a StraightComponent
+      // Special case for FourPipeInduction, which has both optional cooling and heating coils, that can or cannot be linked to some PlantLoops
+      } else if ( lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_FourPipeBeam ) {
+
+        // Safe to directly cast (checked iddObjectType already)
+        AirTerminalSingleDuctConstantVolumeFourPipeBeam lastAtuFourPipeBeam = lastAirTerminal->cast<AirTerminalSingleDuctConstantVolumeFourPipeBeam>();
+        AirTerminalSingleDuctConstantVolumeFourPipeBeam newAtuFourPipeBeam = airTerminal.cast<AirTerminalSingleDuctConstantVolumeFourPipeBeam>();
+
+        boost::optional<PlantLoop> _loop;
+
+        // If the original ATU has a cooling coil, and the cooling coil has a plantLoop, reconnect it here
+        boost::optional<HVACComponent> _hc = lastAtuFourPipeBeam.heatingCoil();
+        if ( _hc && (_loop = _hc->plantLoop()) ) {
+          boost::optional<HVACComponent> _hcClone = newAtuFourPipeBeam.heatingCoil();
+          // FourPipeBeam::clone should clone and re-set the coils, so it should have worked
+          OS_ASSERT(_hcClone);
+          _loop->addDemandBranchForComponent(*_hcClone);
+        }
+
+        // If the original ATU has a cooling coil, and the cooling coil has a plantLoop, reconnect it here
+        boost::optional<HVACComponent> _cc = lastAtuFourPipeBeam.coolingCoil();
+        if ( _cc && (_loop = _cc->plantLoop()) ) {
+          boost::optional<HVACComponent> _ccClone = newAtuFourPipeBeam.coolingCoil();
+          // FourPipeBeam::clone should clone and re-set the coils, so it should have worked
+          OS_ASSERT(_ccClone);
+          _loop->addDemandBranchForComponent(*_ccClone);
+        }
+
+        // Special case for CooledBeam, which isn't a HVAComponent but a StraightComponent
       } else if (lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_CooledBeam) {
 
         // Safe to directly cast
@@ -992,6 +1021,7 @@ namespace detail {
     return result;
   }
 
+  // TODO: REMOVE!
   //bool AirLoopHVAC_Impl::addBranchForZoneImpl(ThermalZone & thermalZone, OptionalStraightComponent & airTerminal)
   //{
   //  boost::optional<HVACComponent> comp;

openstudiocore/src/model/AirLoopHVAC_Impl.hpp

@@ -181,6 +181,7 @@ class MODEL_API AirLoopHVAC_Impl : public Loop_Impl {
                         Splitter & splitter,
                         Mixer & mixer);
 
+  // TODO: remove?
   //bool addBranchForZoneImpl(openstudio::model::ThermalZone & thermalZone,
   //                          boost::optional<StraightComponent> & optAirTerminal);
 

openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam.cpp

@@ -139,21 +139,22 @@ namespace detail {
     return result;
   }
 
-  // TODO
-/*
   std::vector<IdfObject> AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl::remove()
   {
     Model _model = this->model();
     ModelObject thisObject = this->getObject<ModelObject>();
 
-    HVACComponent _coolingCoil = coilCoolingCooledBeam();
+    boost::optional<PlantLoop> loop;
+    boost::optional<HVACComponent> _coolingCoil = this->coolingCoil();
+    boost::optional<HVACComponent> _heatingCoil = this->heatingCoil();
 
     boost::optional<ModelObject> sourceModelObject = this->inletModelObject();
     boost::optional<unsigned> sourcePort = this->connectedObjectPort(this->inletPort());
 
     boost::optional<ModelObject> targetModelObject = this->outletModelObject();
     boost::optional<unsigned> targetPort = this->connectedObjectPort(this->outletPort());
 
+    // Remove from any ZoneHVACEquipmentList
     std::vector<ThermalZone> thermalZones = _model.getConcreteModelObjects<ThermalZone>();
     for( auto & thermalZone : thermalZones )
     {
@@ -162,7 +163,6 @@ namespace detail {
       if( std::find(equipment.begin(),equipment.end(),thisObject) != equipment.end() )
       {
         thermalZone.removeEquipment(thisObject);
-
         break;
       }
     }
@@ -184,9 +184,14 @@ namespace detail {
             inletNode->disconnect();
             inletNode->remove();
 
-            if( boost::optional<PlantLoop> loop = _coolingCoil.plantLoop() )
+            if( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) )
+            {
+              loop->removeDemandBranchWithComponent(*_coolingCoil);
+            }
+
+            if( _heatingCoil && ( loop = _heatingCoil->plantLoop() ) )
             {
-              loop->removeDemandBranchWithComponent(_coolingCoil);
+              loop->removeDemandBranchWithComponent(*_heatingCoil);
             }
 
             return StraightComponent_Impl::remove();
@@ -195,16 +200,21 @@ namespace detail {
       }
     }
 
-    model().disconnect(getObject<ModelObject>(),inletPort());
-    model().disconnect(getObject<ModelObject>(),outletPort());
+    model().disconnect(thisObject, inletPort());
+    model().disconnect(thisObject, outletPort());
 
-    if( boost::optional<PlantLoop> loop = _coolingCoil.plantLoop() )
+    if ( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) )
     {
-      loop->removeDemandBranchWithComponent(_coolingCoil);
+      loop->removeDemandBranchWithComponent(*_coolingCoil);
+    }
+
+    if ( _heatingCoil && ( loop = _heatingCoil->plantLoop() ) )
+    {
+      loop->removeDemandBranchWithComponent(*_heatingCoil);
     }
 
     return StraightComponent_Impl::remove();
-  } */
+  }
 
   bool AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl::isRemovable() const
   {

openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp

@@ -84,7 +84,7 @@ namespace detail {
 
     virtual ModelObject clone(Model model) const override;
 
-    // TODO: virtual std::vector<IdfObject> remove() override;
+    virtual std::vector<IdfObject> remove() override;
 
     virtual bool isRemovable() const override;
 

openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeInduction.cpp

@@ -257,8 +257,8 @@ namespace detail {
       }
     }
 
-    model().disconnect(getObject<ModelObject>(),inletPort());
-    model().disconnect(getObject<ModelObject>(),outletPort());
+    model().disconnect(thisObject, inletPort());
+    model().disconnect(thisObject, outletPort());
 
     if( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) )
     {

openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp

@@ -40,6 +40,7 @@
 #include "../AirLoopHVAC.hpp"
 #include "../ScheduleConstant.hpp"
 #include "../Node.hpp"
+#include "../ThermalZone.hpp"
 
 // Needed for getConcreteModelObjects
 #include "../CoilCoolingFourPipeBeam_Impl.hpp"
@@ -145,27 +146,131 @@ TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_CloneAndRem
 {
 
   Model m;
+  ThermalZone z(m);
+  AirLoopHVAC a(m);
+  PlantLoop hw_p(m);
+  PlantLoop chw_p(m);
+
   CoilCoolingFourPipeBeam cc(m);
   CoilHeatingFourPipeBeam hc(m);
   AirTerminalSingleDuctConstantVolumeFourPipeBeam atu(m, cc, hc);
 
+  EXPECT_TRUE(a.addBranchForZone(z, atu));
+  EXPECT_TRUE(chw_p.addDemandBranchForComponent(cc));
+  EXPECT_TRUE( hw_p.addDemandBranchForComponent(hc));
+
+  EXPECT_EQ(1u, z.equipment().size());
+  EXPECT_EQ(1u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented
+  // 7u = Node, Splitter, One branch with Node - Coil - Node, Mixer, Node
+  EXPECT_EQ(7u, chw_p.demandComponents().size());
+  EXPECT_EQ(7u,  hw_p.demandComponents().size());
+
+
+  // Now clone
   AirTerminalSingleDuctConstantVolumeFourPipeBeam atuClone = atu.clone(m).cast<AirTerminalSingleDuctConstantVolumeFourPipeBeam>();
 
   // I expect the cooling/heating coils to have been cloned too
   EXPECT_EQ(2u, m.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeFourPipeBeam>().size());
   EXPECT_EQ(2u, m.getConcreteModelObjects<CoilCoolingFourPipeBeam>().size());
   EXPECT_EQ(2u, m.getConcreteModelObjects<CoilHeatingFourPipeBeam>().size());
 
+
   // And they should have been reassigned to the clone too
-  ASSERT_TRUE(atuClone.coolingCoil());
-  ASSERT_TRUE(atuClone.heatingCoil());
+  boost::optional<HVACComponent> _ccClone = atuClone.coolingCoil();
+  boost::optional<HVACComponent> _hcClone = atuClone.heatingCoil();
+  ASSERT_TRUE(_ccClone);
+  ASSERT_TRUE(_hcClone);
+  EXPECT_NE(_ccClone->handle(), cc.handle());
+  EXPECT_NE(_hcClone->handle(), hc.handle());
+
+
+  // Shouldn't have been added to the Zone though
+  EXPECT_EQ(1u, z.equipment().size());
+  EXPECT_EQ(1u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented
+
+  // And coils shouldn't have been connected to the PlantLoops either (to match other objects)
+  // It's the AirLoopHVAC::addBranchForZone that will clone the last terminal used, and reconnect it like so
+  EXPECT_EQ(7u, chw_p.demandComponents().size());
+  EXPECT_EQ(7u,  hw_p.demandComponents().size());
+
 
   // test remove method
-  atuClone.remove();
+  atu.remove();
   EXPECT_EQ(1u, m.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeFourPipeBeam>().size());
   EXPECT_EQ(1u, m.getConcreteModelObjects<CoilCoolingFourPipeBeam>().size());
   EXPECT_EQ(1u, m.getConcreteModelObjects<CoilHeatingFourPipeBeam>().size());
 
-
+  EXPECT_EQ(0u, z.equipment().size());
+  EXPECT_EQ(0u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented
+  // 5u = node, splitter, branch with drop node, mixer, node
+  EXPECT_EQ(5u, chw_p.demandComponents().size());
+  EXPECT_EQ(5u,  hw_p.demandComponents().size());
 }
 
+
+TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_addBranchForZone)
+{
+
+  Model m;
+  ThermalZone z(m);
+  ThermalZone z2(m);
+
+  AirLoopHVAC a(m);
+  PlantLoop hw_p(m);
+  PlantLoop chw_p(m);
+
+  CoilCoolingFourPipeBeam cc(m);
+  CoilHeatingFourPipeBeam hc(m);
+  AirTerminalSingleDuctConstantVolumeFourPipeBeam atu(m, cc, hc);
+
+  EXPECT_TRUE(a.addBranchForZone(z, atu));
+  EXPECT_TRUE(chw_p.addDemandBranchForComponent(cc));
+  EXPECT_TRUE( hw_p.addDemandBranchForComponent(hc));
+
+  ASSERT_TRUE(atu.chilledWaterPlantLoop());
+  EXPECT_EQ(chw_p.handle(), atu.chilledWaterPlantLoop()->handle());
+  ASSERT_TRUE(atu.hotWaterPlantLoop());
+  EXPECT_EQ(hw_p.handle(), atu.hotWaterPlantLoop()->handle());
+
+  ASSERT_EQ(1u, z.equipment().size());
+  ASSERT_EQ(0u, z2.equipment().size());
+  EXPECT_EQ(1u, m.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeFourPipeBeam>().size());
+  EXPECT_EQ(1u, m.getConcreteModelObjects<CoilCoolingFourPipeBeam>().size());
+  EXPECT_EQ(1u, m.getConcreteModelObjects<CoilHeatingFourPipeBeam>().size());
+
+  // addBranchForZone(ThermalZone&) is the one used in OS App, and will clone the last terminal found
+  // **AND** reconnect the coils like the original
+
+  EXPECT_TRUE(a.addBranchForZone(z2));
+
+  ASSERT_EQ(1u, z.equipment().size());
+  ASSERT_EQ(1u, z2.equipment().size());
+
+  // I expect the cooling/heating coils to have been cloned too
+  EXPECT_EQ(2u, m.getConcreteModelObjects<AirTerminalSingleDuctConstantVolumeFourPipeBeam>().size());
+  EXPECT_EQ(2u, m.getConcreteModelObjects<CoilCoolingFourPipeBeam>().size());
+  EXPECT_EQ(2u, m.getConcreteModelObjects<CoilHeatingFourPipeBeam>().size());
+
+  boost::optional<AirTerminalSingleDuctConstantVolumeFourPipeBeam> _atuClone = z2.equipment()[0].optionalCast<AirTerminalSingleDuctConstantVolumeFourPipeBeam>();
+  ASSERT_TRUE(_atuClone);
+
+  // Coils should have been cloned and reassigned to the clone too
+  boost::optional<HVACComponent> _ccClone = _atuClone->coolingCoil();
+  boost::optional<HVACComponent> _hcClone = _atuClone->heatingCoil();
+  ASSERT_TRUE(_ccClone);
+  ASSERT_TRUE(_hcClone);
+  EXPECT_NE(_ccClone->handle(), cc.handle());
+  EXPECT_NE(_hcClone->handle(), hc.handle());
+
+  // They should have been reconnected like original one
+  ASSERT_TRUE(_atuClone->chilledWaterPlantLoop());
+  EXPECT_EQ(chw_p.handle(), _atuClone->chilledWaterPlantLoop()->handle());
+  ASSERT_TRUE(_atuClone->hotWaterPlantLoop());
+  EXPECT_EQ(hw_p.handle(), _atuClone->hotWaterPlantLoop()->handle());
+
+  // The original ones should be untouched too
+  ASSERT_TRUE(atu.chilledWaterPlantLoop());
+  EXPECT_EQ(chw_p.handle(), atu.chilledWaterPlantLoop()->handle());
+  ASSERT_TRUE(atu.hotWaterPlantLoop());
+  EXPECT_EQ(hw_p.handle(), atu.hotWaterPlantLoop()->handle());
+}