Preserve SubSurface Outside Boundary Condition Object in ModelMerger #5154
Conversation
jmarrec
added
Pull Request - Ready for CI
src/model/ModelMerger.cpp
Outdated
| // match new to cloned subsurfaces | ||
| for (const auto& newSubSurface : newSurface.subSurfaces()) { | ||
| m_newMergedHandles.insert(newSubSurface.handle()); | ||
| for (auto& cloneSubSurface : clone.subSurfaces()) { | ||
| if (circularEqual(newSubSurface.vertices(), cloneSubSurface.vertices(), 0.01)) { | ||
| m_currentToNewHandleMapping[cloneSubSurface.handle()] = newSubSurface.handle(); | ||
| m_newToCurrentHandleMapping[newSubSurface.handle()] = cloneSubSurface.handle(); | ||
|
|
||
| boost::optional<SubSurface> newAdjacentSubSurface = newSubSurface.adjacentSubSurface(); | ||
| if (newAdjacentSubSurface) { | ||
| boost::optional<UUID> currentAdjacentSubSurfaceHandle = getCurrentModelHandle(newAdjacentSubSurface->handle()); | ||
| if (currentAdjacentSubSurfaceHandle) { | ||
| boost::optional<SubSurface> currentAdjacentSubSurface = m_currentModel.getModelObject<SubSurface>(*currentAdjacentSubSurfaceHandle); | ||
| if (currentAdjacentSubSurface) { | ||
| cloneSubSurface.setAdjacentSubSurface(*currentAdjacentSubSurface); | ||
| } | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ok, so:
- We always emplace subSurfaces in m_newMergedHandles
- we do resolve m_currentToNewHandleMapping and m_newToCurrentHandleMapping
- Only if newAdjacentSubSurface then we try to setAdjacentSubSurface
Do we need 1 & 2 if newAdjacentSubSurface is empty?
I can see it follows the intent, but matching via circularEqual is expensive and it could be a big slow down on large models with lots of surfaces & more importantly lots of subsurfaces on each surface.
There was a problem hiding this comment.
The issue was that clone clones all the children without any support for the handle mapping so I tried to find a way to find the same sub surfaces in both models after the clone. I thought about matching sub surfaces based on names but there are some degenerate cases that would fail on (e.g. merge model {Space1 with SubSurface1, Space2 with SubSurface2} with model {Space1 with SubSurface2, Space2 with SubSurface1} might change object names based on the order spaces are processed).
My thought was to add all sub surfaces to the handle mappings for completeness, but we don't 100% need it. I would be ok to only do the checks if adjacentSubSurface is not empty. I'll update this now.
Changes to clone to support the handle mapping would be possible, and might be generally useful, just a bigger change.
There was a problem hiding this comment.
I made the change and tested it locally. I updated my branch and pushed the change however I can no longer build after the branch was updated to latest develop, I don't have my environment set up for 3.8.0 yet. Is there a wiki for setting up the new environment?
There was a problem hiding this comment.
There was a problem hiding this comment.
Basically you want python (ideally 3.8), and probably set a system ruby to 3.2.2 (though you can build without, but some tests won't run).
Install ninja too.
https://github.com/NREL/OpenStudio/blob/develop/BUILDING.md#full-example
pip install 'conan>2'
git clone git@github.com/NREL/OpenStudio.git
cd OpenStudio
conan install . --output-folder=../OS-build-release --build=missing -c tools.cmake.cmaketoolchain:generator=Ninja -s compiler.cppstd=20 -s build_type=Release
cmake --preset conan-release
cmake --build --preset conan-release
|
Ok build is good, I agree with the code changes, and tests are ok. Let it fly |
Pull request overview
Fixes #5153
Preserve SubSurface Outside Boundary Condition Object in ModelMerger
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.