NatLabRockies / NatLabRockies/OpenStudio

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

Open
#5,636 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
646
Forks
237
Avg merge
3d 11h
Merged PRs (30d)
10

Description

Summary

ModelObject::clone() (and by extension Loop::clone() / AirLoopHVAC::clone()) clones each child individually and reattaches it via setParent(), so true parent-child ownership edges are correctly remapped to point within the new subtree. However, 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 is left broken on the clone.

Concretely:

  • CoilHeatingDesuperheater::heatingSource() — when the source is a sibling coil on the same loop, the field is cleared to empty outright on the clone, rather than pointing at the cloned sibling.
  • SetpointManager node references on an AirLoopHVACOutdoorAirSystem's outdoor-air/relief branch equipment — these still point at the original node objects rather than the clone's.

Both cause EnergyPlus to fail with a fatal error when the resulting model is simulated, since the referenced field is either blank or points at an object outside the new subtree.

To reproduce
  1. Build an AirLoopHVAC with a CoilCoolingDXSingleSpeed and a CoilHeatingDesuperheater whose heatingSource() is set to that cooling coil.
  2. Clone the loop: AirLoopHVAC newLoop = loop.clone(model).cast<AirLoopHVAC>();
  3. Find the cloned desuperheater coil and inspect heatingSource() — it will be empty (or, for other field/type combinations, still point at the original object) instead of pointing at the cloned cooling coil.
Suggested fix

clone() would need to batch-clone the whole subtree into one shared old->new handle map first (rather than cloning children one at a time via setParent()), then do a second pass remapping any object-list field whose target has an entry in that handle map. This is a bigger structural change to clone()'s orchestration, so it likely needs its own design discussion rather than a quick patch.

Context

We hit this in openstudiocoalition/OpenStudioApplication#891 (adding hot-gas-reheat desuperheater support) and worked around it at the application level with a generic post-clone fixup pass (see src/utilities/CloneFixup.cpp in that PR) rather than touching clone() itself. Filing this so the underlying SDK gap is tracked — the app-level fix could be removed if this is addressed upstream.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at ModelObject::clone() and trace how Loop::clone() and AirLoopHVAC::clone() clone and reattach children. Reproduce the sibling-reference case with the coils described in the issue, then compare the behavior with the application-level workaround in src/utilities/CloneFixup.cpp. Done means cloned sibling and node references resolve within the new subtree and the resulting model no longer fails simulation with these fatal errors.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.