From ecebdb3dad61fe183f430eedda9bca4516a6ac9b Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Tue, 14 Jul 2026 11:42:35 +0200 Subject: [PATCH 1/7] feat: add hot-gas reheat unitary system with Coil:Heating:Desuperheater Adds 'Unitary - Single Speed DX cooling - Elec heat - CAV - Hotgas reheat' to the default HVAC library, using CoolReheat dehumidification control with a Coil:Heating:Desuperheater as the reheat coil. The desuperheater reclaims waste heat from the unitary system's own DX cooling coil rather than using resistance reheat, avoiding the extra energy cost of electric reheat. Also fixes AirLoopHVACUnitarySystem cloning (drag-from-library and "copy system"): ModelObject::clone() only remaps parent-child ownership, but CoilHeatingDesuperheater::heatingSource() is a lateral reference to a sibling coil, so it was left unset on the clone. fixupClonedDesuperheaterHeatingSource() re-links the cloned desuperheater coil to the cloned cooling coil. Co-Authored-By: Claude Sonnet 5 --- .../Resources/default/hvac_library.osm | 58 +++++++++++++++++++ src/openstudio_lib/HVACSystemsController.cpp | 36 ++++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/openstudio_app/Resources/default/hvac_library.osm b/src/openstudio_app/Resources/default/hvac_library.osm index 7758f37ac..23b666db2 100644 --- a/src/openstudio_app/Resources/default/hvac_library.osm +++ b/src/openstudio_app/Resources/default/hvac_library.osm @@ -6999,6 +6999,64 @@ OS:AirLoopHVAC:UnitarySystem, , !- Heat Recovery Water Outlet Node Name ; !- Design Specification Multispeed Object Name +OS:Coil:Heating:Desuperheater, + {bdb03443-be0a-4953-8eb4-e6014bbced83}, !- Handle + Desuperheater Reheat Coil, !- Name + {9f54092d-a4a8-41b8-a381-c4c332ecb843}, !- Availability Schedule Name + 0.25, !- Heat Reclaim Recovery Efficiency + , !- Air Inlet Node Name + , !- Air Outlet Node Name + {983b9c20-5a9e-49eb-9770-5c1545cc6b65}, !- Heating Source Name + 0; !- On Cycle Parasitic Electric Load {W} + +OS:AirLoopHVAC:UnitarySystem, + {dbc42aea-40d9-42a6-9016-87cc16813b11}, !- Handle + Unitary - Single Speed DX cooling - Elec heat - CAV - Hotgas reheat, !- Name + Load, !- Control Type + , !- Controlling Zone or Thermostat Location + CoolReheat, !- Dehumidification Control Type + {9f54092d-a4a8-41b8-a381-c4c332ecb843}, !- Availability Schedule Name + , !- Air Inlet Node Name + , !- Air Outlet Node Name + {0c016d27-85cb-4254-8b85-9a16c4586de5}, !- Supply Fan Name + BlowThrough, !- Fan Placement + {9f54092d-a4a8-41b8-a381-c4c332ecb843}, !- Supply Air Fan Operating Mode Schedule Name + {ca3b3fed-58b0-4f64-85f9-257f45d033a5}, !- Heating Coil Name + 1, !- DX Heating Coil Sizing Ratio + {983b9c20-5a9e-49eb-9770-5c1545cc6b65}, !- Cooling Coil Name + No, !- Use DOAS DX Cooling Coil + 2, !- DOAS DX Cooling Coil Leaving Minimum Air Temperature {C} + LatentOrSensibleLoadControl, !- Latent Load Control + {bdb03443-be0a-4953-8eb4-e6014bbced83}, !- Supplemental Heating Coil Name + SupplyAirFlowRate, !- Supply Air Flow Rate Method During Cooling Operation + autosize, !- Supply Air Flow Rate During Cooling Operation {m3/s} + , !- Supply Air Flow Rate Per Floor Area During Cooling Operation {m3/s-m2} + , !- Fraction of Autosized Design Cooling Supply Air Flow Rate + , !- Design Supply Air Flow Rate Per Unit of Capacity During Cooling Operation {m3/s-W} + SupplyAirFlowRate, !- Supply Air Flow Rate Method During Heating Operation + autosize, !- Supply Air Flow Rate During Heating Operation {m3/s} + , !- Supply Air Flow Rate Per Floor Area during Heating Operation {m3/s-m2} + , !- Fraction of Autosized Design Heating Supply Air Flow Rate + , !- Design Supply Air Flow Rate Per Unit of Capacity During Heating Operation {m3/s-W} + None, !- Supply Air Flow Rate Method When No Cooling or Heating is Required + , !- Supply Air Flow Rate When No Cooling or Heating is Required {m3/s} + , !- Supply Air Flow Rate Per Floor Area When No Cooling or Heating is Required {m3/s-m2} + , !- Fraction of Autosized Design Cooling Supply Air Flow Rate When No Cooling or Heating is Required + , !- Fraction of Autosized Design Heating Supply Air Flow Rate When No Cooling or Heating is Required + , !- Design Supply Air Flow Rate Per Unit of Capacity During Cooling Operation When No Cooling or Heating is Required {m3/s-W} + , !- Design Supply Air Flow Rate Per Unit of Capacity During Heating Operation When No Cooling or Heating is Required {m3/s-W} + Yes, !- No Load Supply Air Flow Rate Control Set To Low Speed + 80, !- Maximum Supply Air Temperature {C} + 21, !- Maximum Outdoor Dry-Bulb Temperature for Supplemental Heater Operation {C} + , !- Outdoor Dry-Bulb Temperature Sensor Node Name + 0, !- Ancilliary On-Cycle Electric Power {W} + 0, !- Ancilliary Off-Cycle Electric Power {W} + , !- Design Heat Recovery Water Flow Rate {m3/s} + 80, !- Maximum Temperature for Heat Recovery {C} + , !- Heat Recovery Water Inlet Node Name + , !- Heat Recovery Water Outlet Node Name + ; !- Design Specification Multispeed Object Name + OS:Fan:OnOff, {e803e540-d848-409c-9bad-0c0dc9c7b50a}, !- Handle Fan On Off 14, !- Name diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index 279399050..c02f91ec7 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -92,6 +92,12 @@ #include #include #include +#include +#include +#include +#include +#include +#include #include #include #include @@ -519,6 +525,34 @@ void HVACLayoutController::addLibraryObjectToTopLevel(const OSItemId& itemId) { message.exec(); } +namespace { + // ModelObject::clone() only remaps true parent-child ownership (children()), so it correctly + // duplicates a UnitarySystem's fan/coils. But CoilHeatingDesuperheater::heatingSource() is a + // lateral reference to a sibling coil rather than a child, so it is left unset by the clone. + // If that sibling is the unitary system's own cooling coil, re-link the clone to it. + // Walks the full cloned subtree since the UnitarySystem may be nested (e.g. cloning a whole + // AirLoopHVAC via "copy system") rather than being the clone root itself. + void fixupClonedDesuperheaterHeatingSource(model::ModelObject clonedObject) { + if (boost::optional unitary = clonedObject.optionalCast()) { + if (boost::optional supplementalCoil = unitary->supplementalHeatingCoil()) { + boost::optional desuperheater = supplementalCoil->optionalCast(); + if (desuperheater && !desuperheater->heatingSource()) { + if (boost::optional coolingCoil = unitary->coolingCoil()) { + desuperheater->setHeatingSource(coolingCoil.get()); + } + } + } + } + + // children() is declared on ParentObject, not the generic ModelObject, so cast before recursing. + if (boost::optional parent = clonedObject.optionalCast()) { + for (const model::ModelObject& child : parent->children()) { + fixupClonedDesuperheaterHeatingSource(child); + } + } + } +} // namespace + void HVACLayoutController::addLibraryObjectToModelNode(const OSItemId& itemId, model::HVACComponent& comp) { model::OptionalModelObject object; bool remove = false; @@ -528,6 +562,7 @@ void HVACLayoutController::addLibraryObjectToModelNode(const OSItemId& itemId, m if (object) { if (!doc->fromModel(itemId)) { object = object->clone(comp.model()); + fixupClonedDesuperheaterHeatingSource(object.get()); remove = true; } } @@ -893,6 +928,7 @@ void HVACSystemsController::onCopySystemClicked() { auto loop = currentLoop(); if (loop) { auto clone = loop->clone(loop->model()); + fixupClonedDesuperheaterHeatingSource(clone); setCurrentHandle(toQString(clone.handle())); } } From 4aa75f56795e9c3fbaebe69ef497a96237c3baae Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Tue, 14 Jul 2026 21:13:57 +0200 Subject: [PATCH 2/7] refactor: generalize clone lateral-reference fixup beyond desuperheater ModelObject::clone() clones each child individually and reattaches it via setParent(), so parent-child edges are correctly remapped, but lateral (sibling-to-sibling) object-list references within the cloned subtree are left pointing at the original objects. The previous fix special-cased just CoilHeatingDesuperheater::heatingSource(), but the same bug applies to any lateral reference (e.g. SetpointManager:MixedAir's node fields). Replace it with a generic fixup: build an old-handle -> clone map by walking the original and cloned subtrees in parallel, then use IddObject::objectLists()/WorkspaceObject::getTarget()/setPointer() to rewrite any object-list field in the clone that still points into the original subtree. This fixes the desuperheater case as before, and any future lateral-reference case, without needing a new special case per object type. Co-Authored-By: Claude Sonnet 5 --- src/openstudio_lib/HVACSystemsController.cpp | 78 ++++++++++++++------ 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index c02f91ec7..aa74a6bf7 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -92,10 +92,6 @@ #include #include #include -#include -#include -#include -#include #include #include #include @@ -132,6 +128,8 @@ #include #include +#include + #include #include #include @@ -526,31 +524,66 @@ void HVACLayoutController::addLibraryObjectToTopLevel(const OSItemId& itemId) { } namespace { - // ModelObject::clone() only remaps true parent-child ownership (children()), so it correctly - // duplicates a UnitarySystem's fan/coils. But CoilHeatingDesuperheater::heatingSource() is a - // lateral reference to a sibling coil rather than a child, so it is left unset by the clone. - // If that sibling is the unitary system's own cooling coil, re-link the clone to it. - // Walks the full cloned subtree since the UnitarySystem may be nested (e.g. cloning a whole - // AirLoopHVAC via "copy system") rather than being the clone root itself. - void fixupClonedDesuperheaterHeatingSource(model::ModelObject clonedObject) { - if (boost::optional unitary = clonedObject.optionalCast()) { - if (boost::optional supplementalCoil = unitary->supplementalHeatingCoil()) { - boost::optional desuperheater = supplementalCoil->optionalCast(); - if (desuperheater && !desuperheater->heatingSource()) { - if (boost::optional coolingCoil = unitary->coolingCoil()) { - desuperheater->setHeatingSource(coolingCoil.get()); - } + // ModelObject::clone() clones each child individually and reattaches it via setParent(), so + // true parent-child edges are correctly remapped to point within the new subtree. But children + // are cloned one at a time rather than as a batch sharing a single old->new handle table, so any + // *lateral* object-list reference between two siblings in the cloned subtree (e.g. + // CoilHeatingDesuperheater::heatingSource() pointing at a sibling cooling coil, or a + // SetpointManager's node references) is left pointing at the original objects instead of their + // clones. The functions below fix that up generically, for any object type, rather than special + // casing each lateral-reference field as it's discovered. + + // Walks `original` and `clone` in parallel -- children() order is deterministic and preserved by + // clone() -- building a map from each original object's handle to its corresponding clone. + void buildCloneHandleMap(const model::ModelObject& original, const model::ModelObject& clone, + std::map& handleMap) { + handleMap.emplace(original.handle(), clone); + + boost::optional originalParent = original.optionalCast(); + boost::optional cloneParent = clone.optionalCast(); + if (originalParent && cloneParent) { + std::vector originalChildren = originalParent->children(); + std::vector cloneChildren = cloneParent->children(); + if (originalChildren.size() == cloneChildren.size()) { + for (size_t i = 0; i < originalChildren.size(); ++i) { + buildCloneHandleMap(originalChildren[i], cloneChildren[i], handleMap); } } } + } + + // Rewrites every object-list field on clonedObject (and recursively its children) that still + // points at an object handleMap has a clone for, to point at that clone instead. + void remapLateralReferences(model::ModelObject clonedObject, const std::map& handleMap) { + IddObject iddObject = clonedObject.iddObject(); + for (unsigned index = 0; index < clonedObject.numFields(); ++index) { + if (iddObject.objectLists(index).empty()) { + continue; + } + boost::optional target = clonedObject.getTarget(index); + if (!target) { + continue; + } + auto it = handleMap.find(target->handle()); + if (it != handleMap.end() && it->second.handle() != target->handle()) { + clonedObject.setPointer(index, it->second.handle()); + } + } - // children() is declared on ParentObject, not the generic ModelObject, so cast before recursing. if (boost::optional parent = clonedObject.optionalCast()) { for (const model::ModelObject& child : parent->children()) { - fixupClonedDesuperheaterHeatingSource(child); + remapLateralReferences(child, handleMap); } } } + + // `original` is the subtree that was cloned; `clonedObject` is its clone. Re-links any lateral + // reference in the clone that still points into the original subtree. + void fixupClonedReferences(const model::ModelObject& original, model::ModelObject clonedObject) { + std::map handleMap; + buildCloneHandleMap(original, clonedObject, handleMap); + remapLateralReferences(clonedObject, handleMap); + } } // namespace void HVACLayoutController::addLibraryObjectToModelNode(const OSItemId& itemId, model::HVACComponent& comp) { @@ -561,8 +594,9 @@ void HVACLayoutController::addLibraryObjectToModelNode(const OSItemId& itemId, m object = doc->getModelObject(itemId); if (object) { if (!doc->fromModel(itemId)) { + model::ModelObject original = object.get(); object = object->clone(comp.model()); - fixupClonedDesuperheaterHeatingSource(object.get()); + fixupClonedReferences(original, object.get()); remove = true; } } @@ -928,7 +962,7 @@ void HVACSystemsController::onCopySystemClicked() { auto loop = currentLoop(); if (loop) { auto clone = loop->clone(loop->model()); - fixupClonedDesuperheaterHeatingSource(clone); + fixupClonedReferences(loop.get(), clone); setCurrentHandle(toQString(clone.handle())); } } From 8d82b3bf6f685cea4d43ced9727059bf9095bcc1 Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Wed, 15 Jul 2026 14:37:25 +0200 Subject: [PATCH 3/7] fix: correct clone fixup for fields clone() clears instead of staling Rename the library template to "Desuperheater reheat" for clarity. Testing the generalized fixup against the real SDK (loading hvac_library.osm and cloning the new UnitarySystem both within the same model and across models) showed remapLateralReferences was a no-op for CoilHeatingDesuperheater::heatingSource(): clone() doesn't leave that field pointing at the stale original object, it clears it outright. The previous implementation only rewrote fields that already had some target set on the clone, so it silently did nothing for this case -- reproduced live in the app as a fatal EnergyPlus error (missing heating_source_name / heating_source_object_type) when simulating the new system. Fixed by reading the field to remap from `original` instead of from the clone, and unconditionally forcing the clone's field to match whenever the original's target was itself part of the cloned subtree. Verified against the SDK directly (Ruby) for both same-model ("copy system") and cross-model (drag-from-library) clone scenarios, and confirmed the simulation now runs cleanly in the rebuilt app. Co-Authored-By: Claude Sonnet 5 --- .../Resources/default/hvac_library.osm | 2 +- src/openstudio_lib/HVACSystemsController.cpp | 53 ++++++++++++------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/openstudio_app/Resources/default/hvac_library.osm b/src/openstudio_app/Resources/default/hvac_library.osm index 23b666db2..c928b4305 100644 --- a/src/openstudio_app/Resources/default/hvac_library.osm +++ b/src/openstudio_app/Resources/default/hvac_library.osm @@ -7011,7 +7011,7 @@ OS:Coil:Heating:Desuperheater, OS:AirLoopHVAC:UnitarySystem, {dbc42aea-40d9-42a6-9016-87cc16813b11}, !- Handle - Unitary - Single Speed DX cooling - Elec heat - CAV - Hotgas reheat, !- Name + Unitary - Single Speed DX cooling - Elec heat - CAV - Desuperheater reheat, !- Name Load, !- Control Type , !- Controlling Zone or Thermostat Location CoolReheat, !- Dehumidification Control Type diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index aa74a6bf7..87fbe73cc 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -529,9 +529,10 @@ namespace { // are cloned one at a time rather than as a batch sharing a single old->new handle table, so any // *lateral* object-list reference between two siblings in the cloned subtree (e.g. // CoilHeatingDesuperheater::heatingSource() pointing at a sibling cooling coil, or a - // SetpointManager's node references) is left pointing at the original objects instead of their - // clones. The functions below fix that up generically, for any object type, rather than special - // casing each lateral-reference field as it's discovered. + // SetpointManager's node references) is left broken on the clone -- for some field/type + // combinations still pointing at the original object, for others (empirically, heatingSource()) + // cleared to empty outright. The functions below fix that up generically, for any object type, + // rather than special casing each lateral-reference field as it's discovered. // Walks `original` and `clone` in parallel -- children() order is deterministic and preserved by // clone() -- building a map from each original object's handle to its corresponding clone. @@ -552,37 +553,53 @@ namespace { } } - // Rewrites every object-list field on clonedObject (and recursively its children) that still - // points at an object handleMap has a clone for, to point at that clone instead. - void remapLateralReferences(model::ModelObject clonedObject, const std::map& handleMap) { - IddObject iddObject = clonedObject.iddObject(); - for (unsigned index = 0; index < clonedObject.numFields(); ++index) { + // For every lateral object-list field on `original` whose target was itself cloned (i.e. has an + // entry in handleMap), forces the corresponding field on `clonedObject` to point at that clone. + // Fields are read from `original`, not from `clonedObject`: clone() doesn't necessarily leave a + // lateral reference pointing at the stale original object -- for CoilHeatingDesuperheater's + // heatingSource() it clears the field outright -- so the only reliable source of "what this + // field is supposed to point at" is the original. + void remapLateralReferences(const model::ModelObject& original, model::ModelObject clonedObject, + const std::map& handleMap) { + IddObject iddObject = original.iddObject(); + for (unsigned index = 0; index < original.numFields(); ++index) { if (iddObject.objectLists(index).empty()) { continue; } - boost::optional target = clonedObject.getTarget(index); - if (!target) { + boost::optional originalTarget = original.getTarget(index); + if (!originalTarget) { continue; } - auto it = handleMap.find(target->handle()); - if (it != handleMap.end() && it->second.handle() != target->handle()) { - clonedObject.setPointer(index, it->second.handle()); + auto it = handleMap.find(originalTarget->handle()); + if (it == handleMap.end()) { + continue; + } + boost::optional clonedTarget = clonedObject.getTarget(index); + if (clonedTarget && clonedTarget->handle() == it->second.handle()) { + continue; } + clonedObject.setPointer(index, it->second.handle()); } - if (boost::optional parent = clonedObject.optionalCast()) { - for (const model::ModelObject& child : parent->children()) { - remapLateralReferences(child, handleMap); + boost::optional originalParent = original.optionalCast(); + boost::optional cloneParent = clonedObject.optionalCast(); + if (originalParent && cloneParent) { + std::vector originalChildren = originalParent->children(); + std::vector cloneChildren = cloneParent->children(); + if (originalChildren.size() == cloneChildren.size()) { + for (size_t i = 0; i < originalChildren.size(); ++i) { + remapLateralReferences(originalChildren[i], cloneChildren[i], handleMap); + } } } } // `original` is the subtree that was cloned; `clonedObject` is its clone. Re-links any lateral - // reference in the clone that still points into the original subtree. + // reference in the clone that still points into (or should point into) the original subtree. void fixupClonedReferences(const model::ModelObject& original, model::ModelObject clonedObject) { std::map handleMap; buildCloneHandleMap(original, clonedObject, handleMap); - remapLateralReferences(clonedObject, handleMap); + remapLateralReferences(original, clonedObject, handleMap); } } // namespace From fd5d26546b0b80d67feba341f47abe801e43c39c Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Wed, 15 Jul 2026 15:51:20 +0200 Subject: [PATCH 4/7] fix: reach loop-branch equipment when fixing up cloned references ParentObject::children() is true ownership (e.g. a UnitarySystem's own fan/coils) but does not include HVACComponents placed on a Loop's supply/demand branches. A UnitarySystem sitting on an AirLoopHVAC's supply branch is only reachable via supplyComponents(), not children(). Cloning a whole loop ("Copy System") therefore never visited the branch equipment at all -- including the desuperheater's heating source inside it -- because buildCloneHandleMap/remapLateralReferences only recursed via children(). Reproduced live: copying an existing (working) desuperheater airloop and reassigning zones still hit the same fatal EnergyPlus error, since the copy's desuperheater was never touched by the fixup. Fixed by walking children(), supplyComponents(), and demandComponents() as independently size-matched groups rather than one combined list -- demand-side counts legitimately differ between original and clone (connected thermal zones are never duplicated by clone(), by design), so that mismatch must not block fixing up the supply side, where the equipment lines up 1:1 with the original. Verified against the SDK directly (Ruby) by reproducing the actual AirLoopHVAC topology (OA system, unitary system, zone, terminal) and confirmed live in the app: both "drag from library" and "Copy System" on a whole airloop now correctly re-wire the desuperheater's heating source and a manually-added SetpointManager:MixedAir's node references. Co-Authored-By: Claude Sonnet 5 --- src/openstudio_lib/HVACSystemsController.cpp | 50 +++++++++++++++----- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index 87fbe73cc..4375762bc 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -534,17 +534,43 @@ namespace { // cleared to empty outright. The functions below fix that up generically, for any object type, // rather than special casing each lateral-reference field as it's discovered. - // Walks `original` and `clone` in parallel -- children() order is deterministic and preserved by - // clone() -- building a map from each original object's handle to its corresponding clone. + // ParentObject::children() is true ownership (e.g. a UnitarySystem's fan/coils) but does *not* + // include HVACComponents placed on a Loop's supply/demand branches -- a UnitarySystem sitting on + // an AirLoopHVAC's supply branch is reachable via supplyComponents(), not children(). Cloning a + // whole loop (the "copy system" toolbar action) needs both, or the branch equipment -- and + // anything lateral-referenced from inside it, like the desuperheater's heating source -- is + // never visited at all. + // + // Returned as separate groups (children / supplyComponents / demandComponents), each matched and + // recursed into independently: cloning a loop does not carry over the connected thermal zones on + // the demand side, so original vs. clone demandComponents() can legitimately have different + // sizes. Treating everything as one combined list would let a benign demand-side mismatch abort + // recursion into the supply side too, where the equipment (and lateral references like the + // desuperheater's heating source) *does* line up 1:1 with the original. + std::vector> childSubtreeObjectGroups(const model::ModelObject& object) { + std::vector> groups; + if (boost::optional loop = object.optionalCast()) { + groups.push_back(loop->supplyComponents()); + groups.push_back(loop->demandComponents()); + } + if (boost::optional parent = object.optionalCast()) { + groups.push_back(parent->children()); + } + return groups; + } + + // Walks `original` and `clone` in parallel -- childSubtreeObjectGroups() order is deterministic + // and preserved by clone() -- building a map from each original object's handle to its + // corresponding clone. void buildCloneHandleMap(const model::ModelObject& original, const model::ModelObject& clone, std::map& handleMap) { handleMap.emplace(original.handle(), clone); - boost::optional originalParent = original.optionalCast(); - boost::optional cloneParent = clone.optionalCast(); - if (originalParent && cloneParent) { - std::vector originalChildren = originalParent->children(); - std::vector cloneChildren = cloneParent->children(); + std::vector> originalGroups = childSubtreeObjectGroups(original); + std::vector> cloneGroups = childSubtreeObjectGroups(clone); + for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { + const std::vector& originalChildren = originalGroups[g]; + const std::vector& cloneChildren = cloneGroups[g]; if (originalChildren.size() == cloneChildren.size()) { for (size_t i = 0; i < originalChildren.size(); ++i) { buildCloneHandleMap(originalChildren[i], cloneChildren[i], handleMap); @@ -581,11 +607,11 @@ namespace { clonedObject.setPointer(index, it->second.handle()); } - boost::optional originalParent = original.optionalCast(); - boost::optional cloneParent = clonedObject.optionalCast(); - if (originalParent && cloneParent) { - std::vector originalChildren = originalParent->children(); - std::vector cloneChildren = cloneParent->children(); + std::vector> originalGroups = childSubtreeObjectGroups(original); + std::vector> cloneGroups = childSubtreeObjectGroups(clonedObject); + for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { + const std::vector& originalChildren = originalGroups[g]; + const std::vector& cloneChildren = cloneGroups[g]; if (originalChildren.size() == cloneChildren.size()) { for (size_t i = 0; i < originalChildren.size(); ++i) { remapLateralReferences(originalChildren[i], cloneChildren[i], handleMap); From e334fedbcbf866ec5c025b1411e9319a70492061 Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Wed, 15 Jul 2026 18:00:22 +0200 Subject: [PATCH 5/7] fix: reach outdoor-air/relief branch equipment in clone fixup too Same traversal gap as loop supply/demand branches, one level deeper: equipment placed on an AirLoopHVACOutdoorAirSystem's outdoor-air or relief branch (e.g. an evaporative cooler, or a SetpointManager:MixedAir sitting on one of those nodes) is reachable only via oaComponents()/reliefComponents(), not children() and not the loop's own supplyComponents()/demandComponents() (which only see the OA system as a single opaque object on the main branch). Reproduced with the stock File > Examples > Example Model: its airloop's outdoor air system has an evaporative cooler with a SetpointManager: MixedAir on it. Copying that airloop left the copy's setpoint manager's node fields empty, since the fixup never visited that branch at all. Fixed by adding oaComponents()/reliefComponents() as two more independently size-matched groups in childSubtreeObjectGroups(). Verified against the SDK directly (Ruby, using OpenStudio::Model.exampleModel) -- all 4 of the example model's SetpointManagerMixedAir objects correctly resolve their node references on the clone -- and confirmed live in the rebuilt app. Co-Authored-By: Claude Sonnet 5 --- src/openstudio_lib/HVACSystemsController.cpp | 25 ++++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index 4375762bc..976d7da26 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -536,23 +536,28 @@ namespace { // ParentObject::children() is true ownership (e.g. a UnitarySystem's fan/coils) but does *not* // include HVACComponents placed on a Loop's supply/demand branches -- a UnitarySystem sitting on - // an AirLoopHVAC's supply branch is reachable via supplyComponents(), not children(). Cloning a - // whole loop (the "copy system" toolbar action) needs both, or the branch equipment -- and - // anything lateral-referenced from inside it, like the desuperheater's heating source -- is - // never visited at all. + // an AirLoopHVAC's supply branch is reachable via supplyComponents(), not children(). The same + // gap exists one level deeper: equipment on an AirLoopHVACOutdoorAirSystem's outdoor-air/relief + // branches (e.g. an evaporative cooler, or a SetpointManager:MixedAir sitting on one of those + // nodes) is reachable only via oaComponents()/reliefComponents(), not children() either. Cloning + // a whole loop (the "copy system" toolbar action) needs all of these, or the branch equipment -- + // and anything lateral-referenced from inside it -- is never visited at all. // - // Returned as separate groups (children / supplyComponents / demandComponents), each matched and - // recursed into independently: cloning a loop does not carry over the connected thermal zones on - // the demand side, so original vs. clone demandComponents() can legitimately have different - // sizes. Treating everything as one combined list would let a benign demand-side mismatch abort - // recursion into the supply side too, where the equipment (and lateral references like the - // desuperheater's heating source) *does* line up 1:1 with the original. + // Returned as separate groups, each matched and recursed into independently: cloning a loop does + // not carry over the connected thermal zones on the demand side, so original vs. clone + // demandComponents() can legitimately have different sizes. Treating everything as one combined + // list would let a benign demand-side mismatch abort recursion into the other groups too, where + // the equipment (and any lateral references inside it) *does* line up 1:1 with the original. std::vector> childSubtreeObjectGroups(const model::ModelObject& object) { std::vector> groups; if (boost::optional loop = object.optionalCast()) { groups.push_back(loop->supplyComponents()); groups.push_back(loop->demandComponents()); } + if (boost::optional oaSystem = object.optionalCast()) { + groups.push_back(oaSystem->oaComponents()); + groups.push_back(oaSystem->reliefComponents()); + } if (boost::optional parent = object.optionalCast()) { groups.push_back(parent->children()); } From 3ca1f574d48a9988ea738c1e3e12fb9fd02e2578 Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Tue, 21 Jul 2026 09:38:09 +0300 Subject: [PATCH 6/7] style: fix clang-format indentation in HVACSystemsController.cpp CI flagged the anonymous namespace body as over-indented; clang-format doesn't indent namespace contents under this repo's style. --- src/openstudio_lib/HVACSystemsController.cpp | 195 +++++++++---------- 1 file changed, 97 insertions(+), 98 deletions(-) diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index 976d7da26..f36c9e73c 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -524,114 +524,113 @@ void HVACLayoutController::addLibraryObjectToTopLevel(const OSItemId& itemId) { } namespace { - // ModelObject::clone() clones each child individually and reattaches it via setParent(), so - // true parent-child edges are correctly remapped to point within the new subtree. But children - // are cloned one at a time rather than as a batch sharing a single old->new handle table, so any - // *lateral* object-list reference between two siblings in the cloned subtree (e.g. - // CoilHeatingDesuperheater::heatingSource() pointing at a sibling cooling coil, or a - // SetpointManager's node references) is left broken on the clone -- for some field/type - // combinations still pointing at the original object, for others (empirically, heatingSource()) - // cleared to empty outright. The functions below fix that up generically, for any object type, - // rather than special casing each lateral-reference field as it's discovered. - - // ParentObject::children() is true ownership (e.g. a UnitarySystem's fan/coils) but does *not* - // include HVACComponents placed on a Loop's supply/demand branches -- a UnitarySystem sitting on - // an AirLoopHVAC's supply branch is reachable via supplyComponents(), not children(). The same - // gap exists one level deeper: equipment on an AirLoopHVACOutdoorAirSystem's outdoor-air/relief - // branches (e.g. an evaporative cooler, or a SetpointManager:MixedAir sitting on one of those - // nodes) is reachable only via oaComponents()/reliefComponents(), not children() either. Cloning - // a whole loop (the "copy system" toolbar action) needs all of these, or the branch equipment -- - // and anything lateral-referenced from inside it -- is never visited at all. - // - // Returned as separate groups, each matched and recursed into independently: cloning a loop does - // not carry over the connected thermal zones on the demand side, so original vs. clone - // demandComponents() can legitimately have different sizes. Treating everything as one combined - // list would let a benign demand-side mismatch abort recursion into the other groups too, where - // the equipment (and any lateral references inside it) *does* line up 1:1 with the original. - std::vector> childSubtreeObjectGroups(const model::ModelObject& object) { - std::vector> groups; - if (boost::optional loop = object.optionalCast()) { - groups.push_back(loop->supplyComponents()); - groups.push_back(loop->demandComponents()); - } - if (boost::optional oaSystem = object.optionalCast()) { - groups.push_back(oaSystem->oaComponents()); - groups.push_back(oaSystem->reliefComponents()); - } - if (boost::optional parent = object.optionalCast()) { - groups.push_back(parent->children()); - } - return groups; - } - - // Walks `original` and `clone` in parallel -- childSubtreeObjectGroups() order is deterministic - // and preserved by clone() -- building a map from each original object's handle to its - // corresponding clone. - void buildCloneHandleMap(const model::ModelObject& original, const model::ModelObject& clone, - std::map& handleMap) { - handleMap.emplace(original.handle(), clone); - - std::vector> originalGroups = childSubtreeObjectGroups(original); - std::vector> cloneGroups = childSubtreeObjectGroups(clone); - for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { - const std::vector& originalChildren = originalGroups[g]; - const std::vector& cloneChildren = cloneGroups[g]; - if (originalChildren.size() == cloneChildren.size()) { - for (size_t i = 0; i < originalChildren.size(); ++i) { - buildCloneHandleMap(originalChildren[i], cloneChildren[i], handleMap); - } +// ModelObject::clone() clones each child individually and reattaches it via setParent(), so +// true parent-child edges are correctly remapped to point within the new subtree. But children +// are cloned one at a time rather than as a batch sharing a single old->new handle table, so any +// *lateral* object-list reference between two siblings in the cloned subtree (e.g. +// CoilHeatingDesuperheater::heatingSource() pointing at a sibling cooling coil, or a +// SetpointManager's node references) is left broken on the clone -- for some field/type +// combinations still pointing at the original object, for others (empirically, heatingSource()) +// cleared to empty outright. The functions below fix that up generically, for any object type, +// rather than special casing each lateral-reference field as it's discovered. + +// ParentObject::children() is true ownership (e.g. a UnitarySystem's fan/coils) but does *not* +// include HVACComponents placed on a Loop's supply/demand branches -- a UnitarySystem sitting on +// an AirLoopHVAC's supply branch is reachable via supplyComponents(), not children(). The same +// gap exists one level deeper: equipment on an AirLoopHVACOutdoorAirSystem's outdoor-air/relief +// branches (e.g. an evaporative cooler, or a SetpointManager:MixedAir sitting on one of those +// nodes) is reachable only via oaComponents()/reliefComponents(), not children() either. Cloning +// a whole loop (the "copy system" toolbar action) needs all of these, or the branch equipment -- +// and anything lateral-referenced from inside it -- is never visited at all. +// +// Returned as separate groups, each matched and recursed into independently: cloning a loop does +// not carry over the connected thermal zones on the demand side, so original vs. clone +// demandComponents() can legitimately have different sizes. Treating everything as one combined +// list would let a benign demand-side mismatch abort recursion into the other groups too, where +// the equipment (and any lateral references inside it) *does* line up 1:1 with the original. +std::vector> childSubtreeObjectGroups(const model::ModelObject& object) { + std::vector> groups; + if (boost::optional loop = object.optionalCast()) { + groups.push_back(loop->supplyComponents()); + groups.push_back(loop->demandComponents()); + } + if (boost::optional oaSystem = object.optionalCast()) { + groups.push_back(oaSystem->oaComponents()); + groups.push_back(oaSystem->reliefComponents()); + } + if (boost::optional parent = object.optionalCast()) { + groups.push_back(parent->children()); + } + return groups; +} + +// Walks `original` and `clone` in parallel -- childSubtreeObjectGroups() order is deterministic +// and preserved by clone() -- building a map from each original object's handle to its +// corresponding clone. +void buildCloneHandleMap(const model::ModelObject& original, const model::ModelObject& clone, std::map& handleMap) { + handleMap.emplace(original.handle(), clone); + + std::vector> originalGroups = childSubtreeObjectGroups(original); + std::vector> cloneGroups = childSubtreeObjectGroups(clone); + for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { + const std::vector& originalChildren = originalGroups[g]; + const std::vector& cloneChildren = cloneGroups[g]; + if (originalChildren.size() == cloneChildren.size()) { + for (size_t i = 0; i < originalChildren.size(); ++i) { + buildCloneHandleMap(originalChildren[i], cloneChildren[i], handleMap); } } } +} - // For every lateral object-list field on `original` whose target was itself cloned (i.e. has an - // entry in handleMap), forces the corresponding field on `clonedObject` to point at that clone. - // Fields are read from `original`, not from `clonedObject`: clone() doesn't necessarily leave a - // lateral reference pointing at the stale original object -- for CoilHeatingDesuperheater's - // heatingSource() it clears the field outright -- so the only reliable source of "what this - // field is supposed to point at" is the original. - void remapLateralReferences(const model::ModelObject& original, model::ModelObject clonedObject, - const std::map& handleMap) { - IddObject iddObject = original.iddObject(); - for (unsigned index = 0; index < original.numFields(); ++index) { - if (iddObject.objectLists(index).empty()) { - continue; - } - boost::optional originalTarget = original.getTarget(index); - if (!originalTarget) { - continue; - } - auto it = handleMap.find(originalTarget->handle()); - if (it == handleMap.end()) { - continue; - } - boost::optional clonedTarget = clonedObject.getTarget(index); - if (clonedTarget && clonedTarget->handle() == it->second.handle()) { - continue; - } - clonedObject.setPointer(index, it->second.handle()); +// For every lateral object-list field on `original` whose target was itself cloned (i.e. has an +// entry in handleMap), forces the corresponding field on `clonedObject` to point at that clone. +// Fields are read from `original`, not from `clonedObject`: clone() doesn't necessarily leave a +// lateral reference pointing at the stale original object -- for CoilHeatingDesuperheater's +// heatingSource() it clears the field outright -- so the only reliable source of "what this +// field is supposed to point at" is the original. +void remapLateralReferences(const model::ModelObject& original, model::ModelObject clonedObject, + const std::map& handleMap) { + IddObject iddObject = original.iddObject(); + for (unsigned index = 0; index < original.numFields(); ++index) { + if (iddObject.objectLists(index).empty()) { + continue; } + boost::optional originalTarget = original.getTarget(index); + if (!originalTarget) { + continue; + } + auto it = handleMap.find(originalTarget->handle()); + if (it == handleMap.end()) { + continue; + } + boost::optional clonedTarget = clonedObject.getTarget(index); + if (clonedTarget && clonedTarget->handle() == it->second.handle()) { + continue; + } + clonedObject.setPointer(index, it->second.handle()); + } - std::vector> originalGroups = childSubtreeObjectGroups(original); - std::vector> cloneGroups = childSubtreeObjectGroups(clonedObject); - for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { - const std::vector& originalChildren = originalGroups[g]; - const std::vector& cloneChildren = cloneGroups[g]; - if (originalChildren.size() == cloneChildren.size()) { - for (size_t i = 0; i < originalChildren.size(); ++i) { - remapLateralReferences(originalChildren[i], cloneChildren[i], handleMap); - } + std::vector> originalGroups = childSubtreeObjectGroups(original); + std::vector> cloneGroups = childSubtreeObjectGroups(clonedObject); + for (size_t g = 0; g < originalGroups.size() && g < cloneGroups.size(); ++g) { + const std::vector& originalChildren = originalGroups[g]; + const std::vector& cloneChildren = cloneGroups[g]; + if (originalChildren.size() == cloneChildren.size()) { + for (size_t i = 0; i < originalChildren.size(); ++i) { + remapLateralReferences(originalChildren[i], cloneChildren[i], handleMap); } } } +} - // `original` is the subtree that was cloned; `clonedObject` is its clone. Re-links any lateral - // reference in the clone that still points into (or should point into) the original subtree. - void fixupClonedReferences(const model::ModelObject& original, model::ModelObject clonedObject) { - std::map handleMap; - buildCloneHandleMap(original, clonedObject, handleMap); - remapLateralReferences(original, clonedObject, handleMap); - } +// `original` is the subtree that was cloned; `clonedObject` is its clone. Re-links any lateral +// reference in the clone that still points into (or should point into) the original subtree. +void fixupClonedReferences(const model::ModelObject& original, model::ModelObject clonedObject) { + std::map handleMap; + buildCloneHandleMap(original, clonedObject, handleMap); + remapLateralReferences(original, clonedObject, handleMap); +} } // namespace void HVACLayoutController::addLibraryObjectToModelNode(const OSItemId& itemId, model::HVACComponent& comp) { From efa9de1394f7b7a150b0d5fb15c5f564d1dc4c3c Mon Sep 17 00:00:00 2001 From: Ski90Moo Date: Tue, 21 Jul 2026 17:10:10 +0300 Subject: [PATCH 7/7] perf: pass clonedObject by const reference in fixupClonedReferences cppcheck flagged the unnecessary copy -- the parameter is only forwarded into buildCloneHandleMap/remapLateralReferences, both of which already accept it by const reference. --- src/openstudio_lib/HVACSystemsController.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openstudio_lib/HVACSystemsController.cpp b/src/openstudio_lib/HVACSystemsController.cpp index f36c9e73c..a4e94c40e 100644 --- a/src/openstudio_lib/HVACSystemsController.cpp +++ b/src/openstudio_lib/HVACSystemsController.cpp @@ -626,7 +626,7 @@ void remapLateralReferences(const model::ModelObject& original, model::ModelObje // `original` is the subtree that was cloned; `clonedObject` is its clone. Re-links any lateral // reference in the clone that still points into (or should point into) the original subtree. -void fixupClonedReferences(const model::ModelObject& original, model::ModelObject clonedObject) { +void fixupClonedReferences(const model::ModelObject& original, const model::ModelObject& clonedObject) { std::map handleMap; buildCloneHandleMap(original, clonedObject, handleMap); remapLateralReferences(original, clonedObject, handleMap);