Update ShadingControl - #4103
Conversation
joseph-robertson
added
component - Model
Pull Request - Ready for CI
Conflicts: src/energyplus/ForwardTranslator.cpp src/model/ShadingControl.cpp
| // Check if zone of subSurface not zone of existing subSurfaces | ||
| if (boost::optional<Space> space = subSurface.space()) { | ||
| if (boost::optional<ThermalZone> thermalZone = space->thermalZone()) { | ||
| for (auto& subSurface2 : subSurfaces()) { | ||
| if (boost::optional<Space> space2 = subSurface2.space()) { | ||
| if (boost::optional<ThermalZone> thermalZone2 = space2->thermalZone()) { | ||
| if (thermalZone->handle() != thermalZone2->handle()) { | ||
| LOG(Warn, briefDescription() << ": cannot add two surfaces that are in different Thermal Zones ('" << thermalZone->nameString() | ||
| << "' versus existing '" << thermalZone2->nameString() << "')"); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I thought @shorowit said that this is actually fine now to have a multiple zones referenced in the same ShadingControl? #4066 (comment)
There was a problem hiding this comment.
I'm not an expert on this object and there are so many ways to use it that it becomes very confusing. But as far as I know, it has always been fine to have subsurfaces of multiple zones referenced by a single ShadingControl, at least under some circumstances. When we did this, we used the "OnIfScheduleAllows" shading control type. If you instead used a control type that depends on the thermal zone specified, I don't know what would happen.
| auto it = zoneHandleToShadingControlVectorMap.find(zoneHandle); | ||
| if (it == zoneHandleToShadingControlVectorMap.end()) { | ||
| zoneHandleToShadingControlVectorMap.insert(std::make_pair(zoneHandle, std::vector<ShadingControl>())); | ||
| } | ||
| it = zoneHandleToShadingControlVectorMap.find(zoneHandle); | ||
| OS_ASSERT(it != zoneHandleToShadingControlVectorMap.end()); | ||
| it->second.push_back(shadingControl); | ||
| shadingControl.additionalProperties().setFeature("Shading Control Sequence Number", (int)it->second.size()); |
There was a problem hiding this comment.
The FT change seems to make sense though. What's weird and not handled (I have no clue how to properly handle that though) is the case where you have something like:
- Shading Control 1 controls Zone1-SurfaceA1
- Shading Control 2 controls Zone2-SurfaceA2 and Zone2-SurfaceA2
Then what happens to the Shading Control Sequence Number? Shading Control 2 would have a Shading Control Sequence Number of 2 (assuming it wasn't renamed, since the sort is done on name...), but Zone2-SurfaceA2 is referenced by only one shading control with a sequence number of 2. Does that make sense and is E+ ok with it? (I don't know).
@shorowit thoughts please? It's been a while I've used ShadingControls; and never since the E+ 9.4 changes, and I don't think the E+ API got any clearer in this specific case...
There was a problem hiding this comment.
I have no experience with the Shading Control Sequence Numbers, sorry. (I've never used the ShadingControl object in a way where the sequence numbers mattered.)
|
CI Results for aeaa599:
|
|
Closing for now. Issue captured in #4602. |
4 participants
Pull request overview
Please read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)src/openstudio_lib/library/OpenStudioPolicy.xml)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.