Add hot-gas reheat unitary system (Coil:Heating:Desuperheater) by Ski90Moo · Pull Request #891 · openstudiocoalition/OpenStudioApplication

Ski90Moo · 2026-07-15T16:29:01Z

Summary

Adds "Unitary - Single Speed DX cooling - Elec heat - CAV - Desuperheater reheat" to the default HVAC library: the existing single-speed DX cooling coil paired with a new Coil:Heating:Desuperheater as the supplemental/reheat coil under CoolReheat dehumidification control, so reheat is partially sourced from the cooling coil's waste heat instead of just resistance heat.
Fixes AirLoopHVACUnitarySystem/loop cloning (both "drag from library" and "Copy System"): ModelObject::clone() only remaps true parent-child relationships. Lateral (sibling-to-sibling) object-list references — like CoilHeatingDesuperheater::heatingSource(), or a SetpointManager:MixedAir's node fields — are left broken on the clone, sometimes stale, sometimes cleared outright. Added a generic post-clone reference fixup (buildCloneHandleMap + remapLateralReferences in HVACSystemsController.cpp, using IddObject::objectLists()/WorkspaceObject::getTarget()/setPointer()) rather than a Desuperheater-specific special case.
The traversal walks children(), a loop's supplyComponents()/demandComponents(), and an outdoor air system's oaComponents()/reliefComponents() as independently size-matched groups, since branch equipment (units on a loop, or equipment like an evaporative cooler on an OA system's branches) isn't reachable via children() alone, and demand-side counts can legitimately differ between original and clone since connected thermal zones are never duplicated.

Context

Modeling dehumidification via hot-gas/desuperheater reheat is a recurring real-world need — see this UnmetHours thread, where CoilCoolingDXTwoStageWithHumidityControlMode proved awkward to control directly and commenters pointed toward Coil:Heating:Desuperheater as a workaround. Worth noting the thread's caveat: a desuperheater coil reclaims waste heat from the refrigerant's superheat and doesn't affect compressor performance, so it's a lower-fidelity approximation of "true" hot-gas reheat, not an exact substitute — but it's a standard, supported EnergyPlus object and a reasonable default-library addition for this use case.

Proof this was broken before this fix

You don't need the new desuperheater system to see the underlying bug — it reproduces with a completely stock model:

In the app, File > Examples > Example Model to generate a fresh example model.
Go to the HVAC Systems tab, select the one airloop, click "Copy System."
Select the copy and open its outdoor air system. It has an evaporative cooler with a SetpointManager:MixedAir attached — but on the copy, that setpoint manager's Reference Setpoint / Fan Inlet / Fan Outlet Node fields are all empty (shown as warning icons in the inspector), even though the original's are filled in correctly.
Run a simulation on the copy: EnergyPlus fails immediately with a fatal error (missing required properties on that object) before any calculation happens.

That's ModelObject::clone()'s lateral-reference bug on a totally unmodified example model — this PR's fixup resolves it for both this stock case and the new desuperheater system.

Test plan

cmake --build . --target openstudio_lib --config Release succeeds
Dragged the new system onto a zone in the app and ran the simulation successfully
Used "Copy System" on that airloop, reassigned zones on the copy, and ran the simulation successfully
Verified against the SDK directly (Ruby) for same-model/cross-model clone scenarios, and against OpenStudio::Model.exampleModel's stock outdoor-air-system evaporative cooler + SetpointManager:MixedAir
Confirmed live in the rebuilt app that "Copy System" correctly re-wires node references on that stock example airloop too

macumber · 2026-07-19T17:49:25Z

+  // 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


This makes sense to me but I wonder two things:

Why has this not been an issue before? Does the current workflow just make you have to go through the new airloop and reattach things to each other?

Should this be proposed to the SDK as an improvement to AirLoopHVAC::clone? Or maybe even ModelObject::clone?

Good questions.

It actually has been latent — clone() has never remapped lateral (sibling-to-sibling) object-list references, only true parent-child ownership edges. Both the desuperheater case and this OA-branch case fail loudly in E+ when hit. The difference is exposure: heatingSource() is core to every use of the new desuperheater template, so it's unmissable. The OA-branch gap (equipment on an AirLoopHVACOutdoorAirSystem's oa/relief branch, reachable only via oaComponents()/reliefComponents()) only fires for a narrower setup — an airloop whose OA system has its own branch equipment with a SetpointManager:MixedAir targeting those nodes, like the stock File > Examples > Example Model. Anyone who hit that fatal error after "Copy System" most likely just manually re-filled the blank node fields — indistinguishable from the zone-reassignment step users already expect to do after copying — rather than recognizing it as an app bug.

Agreed that fixing this inside ModelObject::clone()/AirLoopHVAC::clone() itself would be the more correct place — it'd benefit every SDK client, not just this app. But doing it "correctly" there is a bigger change: clone() currently clones children one at a time via setParent() rather than batch-cloning the whole subtree into one shared handle map first, so fixing it at that level means restructuring clone's own orchestration for every model object type, not just HVAC. That's more design/test surface than fits in this PR. I'd like to land this app-level fix now (it's scoped and tested, and it also fixes the example-model bug above), and open a tracking issue on the SDK repo proposing the real fix — I can file that and link it here if that sounds right.

Sounds good, please do open an SDK issue to track this. If the SDK fixes the issue we can remove this. But this fix will be useful for now.

Just add a few comments and suggestions to improve efficiency since there is a lot of looping. I'd move this to utilities since it is a candidate SDK method.

Filed NatLabRockies/OpenStudio#5636 to track the underlying clone() gap upstream.

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

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 <noreply@anthropic.com>

CI flagged the anonymous namespace body as over-indented; clang-format doesn't indent namespace contents under this repo's style.

cppcheck flagged the unnecessary copy -- the parameter is only forwarded into buildCloneHandleMap/remapLateralReferences, both of which already accept it by const reference.

macumber · 2026-07-22T03:32:02Z

    if (!doc->fromModel(itemId)) {
+      model::ModelObject original = object.get();
      object = object->clone(comp.model());
+      fixupClonedReferences(original, object.get());


I would suggest moving fixupClonedReferences to src/utilities and adding some unit tests

Done in 1256782 — moved fixupClonedReferences (and its now-private helpers) to src/utilities/CloneFixup.{hpp,cpp}, and added src/utilities/test/CloneFixup_GTest.cpp with a test covering the CoilHeatingDesuperheater::heatingSource() case plus a no-op sanity check.

macumber · 2026-07-22T03:35:26Z

+// 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,


clonedObject can be passed by reference

Done in 1256782 — remapLateralReferences now takes model::ModelObject& instead of by value.

macumber · 2026-07-22T03:36:41Z

+// `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, const model::ModelObject& clonedObject) {
+  std::map<Handle, model::ModelObject> handleMap;


Can use std::unordered_map since ordering isn't important

Done in 1256782 — switched to std::unordered_map<Handle, model::ModelObject, boost::hash<boost::uuids::uuid>> (matching the pattern already used for Handle keys in Workspace_Impl.hpp, since Handle has no std::hash specialization).

macumber · 2026-07-22T03:40:02Z

+void buildCloneHandleMap(const model::ModelObject& original, const model::ModelObject& clone, std::map<Handle, model::ModelObject>& handleMap) {
+  handleMap.emplace(original.handle(), clone);
+
+  std::vector<std::vector<model::ModelObject>> originalGroups = childSubtreeObjectGroups(original);


Can you move this recursion into childSubtreeObjectGroups so you just call childSubtreeObjectGroups once and it gets all the subtrees in one call?

Done in 1256782 — merged the duplicated tree-walk from buildCloneHandleMap/remapLateralReferences into a single recursive pass (collectClonedObjectPairs) that's called once and returns the flat list of matched original/clone pairs; both the handle-map build and the reference remap now just iterate that list.

Move fixupClonedReferences and its helpers from HVACSystemsController.cpp into src/utilities/CloneFixup.{hpp,cpp} and add unit tests covering the CoilHeatingDesuperheater::heatingSource() case. Also, per review: merge the duplicated tree-walk in buildCloneHandleMap/remapLateralReferences into a single recursive pass (collectClonedObjectPairs), pass clonedObject by reference instead of copying it down the recursion, and switch the handle map to std::unordered_map (ordering was never relied on). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

clonedObject is only ever forwarded into collectClonedObjectPairs's const-ref parameter here; the actual mutation happens later on the copy stored in the pairs vector, which is safe since ModelObject shares its underlying object via shared_ptr regardless of wrapper constness. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Ski90Moo mentioned this pull request Jul 15, 2026

Add hot-gas reheat unitary system (Coil:Heating:Desuperheater) Ski90Moo/OpenStudioApplication#3

Closed

5 tasks

macumber reviewed Jul 19, 2026

View reviewed changes

Comment thread .gitignore Outdated

macumber reviewed Jul 19, 2026

View reviewed changes

Comment thread .gitignore Outdated

macumber reviewed Jul 19, 2026

View reviewed changes

Ski90Moo mentioned this pull request Jul 21, 2026

Revert in-source build .gitignore additions Ski90Moo/OpenStudioApplication#4

Merged

Ski90Moo and others added 5 commits July 21, 2026 08:53

Ski90Moo force-pushed the feature/desuperheater-hotgas-reheat branch from 39e82f5 to e334fed Compare July 21, 2026 05:56

Ski90Moo added 2 commits July 21, 2026 09:38

style: fix clang-format indentation in HVACSystemsController.cpp

3ca1f57

CI flagged the anonymous namespace body as over-indented; clang-format doesn't indent namespace contents under this repo's style.

perf: pass clonedObject by const reference in fixupClonedReferences

efa9de1

cppcheck flagged the unnecessary copy -- the parameter is only forwarded into buildCloneHandleMap/remapLateralReferences, both of which already accept it by const reference.

macumber reviewed Jul 22, 2026

View reviewed changes

Ski90Moo mentioned this pull request Jul 23, 2026

ModelObject::clone() doesn't remap lateral (sibling-to-sibling) object-list references within the cloned subtree NatLabRockies/OpenStudio#5636

Open

Uh oh!

Conversation

Ski90Moo commented Jul 15, 2026

Summary

Context

Proof this was broken before this fix

Test plan

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

macumber Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

macumber Jul 22, 2026 •

edited

Loading