Multiple shading controls referenced by a single subsurface by joseph-robertson · Pull Request #4066 · NatLabRockies/OpenStudio

joseph-robertson · 2020-09-04T18:45:13Z

Pull request overview

SubSurface

boost::optional<ShadingControl> shadingControl() const (OS_DEPRECATED; only returns if there is 1 shading control)
std::vector<ShadingControl> shadingControls() const (NEW)
unsigned int numberofShadingControls() const (NEW)
bool setShadingControl(const ShadingControl& shadingControl) (OS_DEPRECATED; will remove any existing shading controls and then add the one shading control)
void resetShadingControl() (OS_DEPRECATED; will remove any existing shading controls)
bool addShadingControl(const ShadingControl& shadingControl) (NEW)
bool addShadingControls(const std::vector<ShadingControl> &shadingControls) (NEW)
void removeShadingControl(const ShadingControl& shadingControl) (NEW)
void removeAllShadingControls() (NEW)

ShadingControl

unsigned int numberofSubSurfaces() const (NEW)
bool addSubSurface(const SubSurface& subSurface) (NEW)
bool addSubSurfaces(const std::vector<SubSurface> &subSurfaces) (NEW)
void removeSubSurface(const SubSurface& subSurface) (NEW)
void removeAllSubSurfaces() (NEW)
bool glareControlIsActivate() const (NEW)
std::string typeofSlatAngleControlforBlinds() const (NEW)
bool isTypeofSlatAngleControlforBlindsDefault() const (NEW)
boost::optional<Schedule> slatAngleSchedule() const (NEW)
std::string multipleSurfaceControlType() const (NEW)
bool setGlareControlIsActive(bool glareControlIsActive) (NEW)
void resetGlareControlIsActive() (NEW)
bool setTypeofSlatAngleControlforBlinds(const std::string& typeofSlatAngleControlforBlinds) (NEW)
void resetTypeofSlatAngleControlforBlinds() (NEW)
bool setSlatAngleSchedule(const Schedule& slatAngleSchedule) (NEW)
void resetSlatAngleSchedule() (NEW)
bool setMultipleSurfaceControlType(const std::string& multipleSurfaceControlType) (NEW)

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.

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…surface.

jmarrec · 2020-09-04T20:15:22Z

@joseph-robertson I'll take a deeper look on monday. Mostly I think I need to look up how the E+ objects interact exactly... but I have a couple of naive comments that perhaps you can clear out today:

seems like you are providing add/remove API from both SubSurface and ShadingControl, which looks unusual. Having getters as convenience on both objects isn't unusual, but setters on both is. Typically the the one to many relationship would be that the one who has the referencing field would have the setter. As in shadingControl.setSurbSurface. And you would have SubSurface::shadingControls() getter or something.
You'll need a serious VersioNTranslation for this change probably.
I would reckon you may have to break API in this... I haven't looked in detail but it's a possibility that breaking API could provide a cleaner API.

joseph-robertson · 2020-09-04T21:20:10Z

I think we're setting up a many-to-many relationship. You can have:

a sub surface with multiple shading controls
a shading control that is shared across multiple sub surfaces

The second bullet was already supported. I'm thinking we wouldn't have to break API to support the first bullet. subSurface.setShadingControl(shadingControl) would just call shadingControl.addSubSurface(subSurface). Version translation would just be: take the shading control object referenced on the sub surface object, and do shadingControl.addSubSurface(subSurface).

jmarrec

@joseph-robertson I missed the many-to-many relationship aspect, this makes sense now, thanks for the clarification!

A couple more comments for discussion. The relationship between these two objects in E+ is quite awkward, and I'm still unsure how we would deal with the "Shading Control Sequence Number" in particular.

jmarrec · 2020-09-07T08:56:00Z


+    std::vector<ShadingControl> shadingControls();
+
+    ShadingControl getShadingControl(unsigned shadingControlIndex);


That may be a bit weird. The order here is kinda undefined isn't it? Unlike ShadingControl which has an ordered list via its extensible groups, here it will depend on the order of the objects in the model (the shadingControls() is fine though, as this doesn't make any promises in terms of order)

jmarrec · 2020-09-07T09:00:55Z


+    bool setShadingControls(const std::vector<ShadingControl> &shadingControls);
+
+    bool insertShadingControl(unsigned shadingControlIndex, const ShadingControl &shadingControl);


Same comment about order as above. I am not sure it's appropriate or even wanted?

Is the idea to tie into the field "Shading Control Sequence Number" on the ShadingControl object? That field is a bit weird...

https://github.com/NREL/OpenStudio/blob/fa9a77bf173556a2a044f64789a7f4f01b0df076/src/energyplus/ForwardTranslator/ForwardTranslateShadingControl.cpp#L114-L119

jmarrec · 2020-09-09T12:39:49Z

  boost::optional<ShadingControl> SubSurface_Impl::shadingControl() const
  {
-    return getObject<SubSurface>().getModelObjectTarget<ShadingControl>(OS_SubSurfaceFields::ShadingControlName);
+    if (numberofShadingControls() > 1) {


I would warn that there are more than 1, then return the first found.

You cna also do that on the public side only, and remove the ShadingControl() method from Impl altogether.

jmarrec · 2020-09-09T12:47:16Z

+  EXPECT_TRUE(shadingControl.addSubSurfaces(subSurfaces));
+  EXPECT_EQ(2, shadingControl.numberofSubSurfaces());
+  shadingControl.removeAllSubSurfaces();
+  EXPECT_EQ(0, shadingControl.numberofSubSurfaces());


Expand the test. Assign subSurface1 and subSurface2, then subSurface2.remove() => what happens to the numberOfSurbSurfaces.

Add a clone test too: are the extensible groups cleared? (do we want them to be?)

@jmarrec I added tests for these two scenarios. They both fail. For the first, shadingControl.numberofSubSurfaces remains at what it was before. For the second, cloning the shadingControl object throws an error.

…hen we are mutating it. m

…eph-robertson

…eph-robertson

@joseph-robertson

…ing a crude change for build to keep going @joseph-robertson

jmarrec · 2020-09-15T12:44:45Z

For #4074, fix is here: d6041d8
Can someone please make sure I interpreted the E+ docs correctly and didn't mess up something there?

Edit: Follow up in 361e509

@kbenne @asparke2 @joseph-robertson

…dule all the time. Left A TODO

Found it while investigating #4062

…o into multiple-shading-controls

…ontrols

ci-commercialbuildings · 2020-09-17T13:04:24Z

CI Results for e454bfa:

jmarrec · 2020-09-17T14:21:11Z

@joseph-robertson did we restrict the SubSurfaces to be part of the same ThermalZOne? I think we perhaps should?

shorowit · 2020-09-17T14:52:33Z

@joseph-robertson did we restrict the SubSurfaces to be part of the same ThermalZOne? I think we perhaps should?

I don't agree with this. See #3380 for example.

EDIT: So note that there is a warning for this case.

joseph-robertson · 2020-09-17T14:57:08Z

@joseph-robertson did we restrict the SubSurfaces to be part of the same ThermalZOne? I think we perhaps should?

@jmarrec Why do you think we should?

jmarrec · 2020-09-17T15:00:11Z

I think I misread the doc for the field https://bigladdersoftware.com/epx/docs/9-3/input-output-reference/group-thermal-zone-description-geometry.html#shading-control-sequence-number (I read that they should be in the same zone) as I had just read the comment in the ForwardTranslator just before it. Now I see it's not a problem to span across zones

If multiple WindowShadingControl objects are used in the same zone, then the order that they deploy the window shades can be set with this field. The first WindowShadingControl should be 1 and subsequent WindowShadingControl should 2, 3, etc. This is usually used when the Multiple Surface Control Type field is set to Group to control groups of windows in a certain order.

Well then it goes back to the action item I added: Does the logic in the ForwardTrnaslator.cpp need to be updated?

https://github.com/NREL/OpenStudio/blob/aa61fc98fbf85cc7feb3cc84a4c228e6fe840c64/src/energyplus/ForwardTranslator.cpp#L448-L450

kbenne · 2020-09-18T15:45:50Z

I have merged this into #4073 and will go ahead and merge the combined work which will close #4066 and #4073. Lets just please pay close attention to the regression tests, and @joseph-robertson consider adding a regression test to demonstrate the multiple shading surfaces.

joseph-robertson · 2021-06-25T18:38:32Z

I think I misread the doc for the field https://bigladdersoftware.com/epx/docs/9-3/input-output-reference/group-thermal-zone-description-geometry.html#shading-control-sequence-number (I read that they should be in the same zone) as I had just read the comment in the ForwardTranslator just before it. Now I see it's not a problem to span across zones

If multiple WindowShadingControl objects are used in the same zone, then the order that they deploy the window shades can be set with this field. The first WindowShadingControl should be 1 and subsequent WindowShadingControl should 2, 3, etc. This is usually used when the Multiple Surface Control Type field is set to Group to control groups of windows in a certain order.

Well then it goes back to the action item I added: Does the logic in the ForwardTrnaslator.cpp need to be updated?

https://github.com/NREL/OpenStudio/blob/aa61fc98fbf85cc7feb3cc84a4c228e6fe840c64/src/energyplus/ForwardTranslator.cpp#L448-L450

@jmarrec @shorowit

Here is a new test (shadingcontrol_multizone.rb) related to this: NatLabRockies/OpenStudio-resources#147.

From what I can tell, the idf contains shading controls with sub surfaces from different zones. However, the "Window Control" table in eplustbl.htm shows only a single shading control object for each sub surface. In fact, when I look at eplustbl.htm for shadingcontrol_singlezone.rb I see a similar thing. So I'm not sure if this is working. Shouldn't we see multiple shading control objects for the same sub surface?

joseph-robertson added 2 commits September 4, 2020 11:42

Store subsurface on shading control instead of shading control on sub…

691f6a7

…surface.

Stub new methods for subsurface and shading control.

71318ce

joseph-robertson added component - Model IDDChange labels Sep 4, 2020

joseph-robertson added this to the OpenStudio SDK 3.1.0 milestone Sep 4, 2020

joseph-robertson self-assigned this Sep 4, 2020

jmarrec reviewed Sep 7, 2020

View reviewed changes

joseph-robertson added 7 commits September 8, 2020 12:29

Start filling out methods for shading control.

f9df752

Start filling out methods for sub surface.

4c453f5

Continue to fill out sub surface methods.

92fdd7b

Add new model test for sub surface.

f84e710

Add new model test for shading control.

b282d03

Clean up some model and test files.

b14c8f7

Start a vt for sub surface and shading control.

afda1a1

jmarrec reviewed Sep 10, 2020

View reviewed changes

joseph-robertson added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Sep 10, 2020

joseph-robertson marked this pull request as ready for review September 10, 2020 16:55

joseph-robertson and others added 7 commits September 10, 2020 10:04

Updates to source files after review.

4ab9318

Make some changes to ShadingControl.

8b1664b

Remove the deprecated functions from the Impl API to fix compiler error

3e236f1

const-correctness; ,no ABI break: cannot pass cosnt ShadingCOntrol& w…

2f008b3

…hen we are mutating it. m

ABI break of now-deprecated function

ff629b3

Update description

373d248

Update FT to avoid error using deprecated. is this still needed? @jos…

87820ee

…eph-robertson

jmarrec force-pushed the multiple-shading-controls branch from 8902811 to 3118db0 Compare September 11, 2020 10:54

jmarrec added 2 commits September 11, 2020 12:58

Implement new MultipleSurfaceControlType field in Shading Control

5dd85a1

The Radiance Forward Translator also needs to be update. I'm just mak…

6619cbf

…ing a crude change for build to keep going @joseph-robertson

jmarrec force-pushed the multiple-shading-controls branch from 3118db0 to 6619cbf Compare September 11, 2020 10:59

Add some release notes for this PR

b06fd15

jmarrec reviewed Sep 15, 2020

View reviewed changes

Comment thread developer/doc/ReleaseNotes/OpenStudio_Release_Notes_3_1_0.md Outdated

Missing an override keyword

9d5b2ac

jmarrec and others added 7 commits September 15, 2020 15:05

#4074 Followup: add test for logic, and avoid clearing Setpoint2/Sche…

361e509

…dule all the time. Left A TODO

Typo in C# partial class

33df578

Unrelated C# fixed while I'm at it.

b751a34

Unrelated issue in ModelObject::getAutosizedValue

bec2af2

Found it while investigating #4062

Include glare and slat angle control in api.

64f1a6d

Merge branch 'multiple-shading-controls' of github.com:NREL/OpenStudi…

abe551f

…o into multiple-shading-controls

Another typo in getAutosizedValue

7bea01b

jmarrec force-pushed the multiple-shading-controls branch from 3448990 to 7bea01b Compare September 16, 2020 09:22

Updates to release notes draft.

81be659

jmarrec mentioned this pull request Sep 17, 2020

Update to OS SDK 3.1.0 openstudiocoalition/OpenStudioApplication#235

Closed

1 task

jmarrec added 2 commits September 17, 2020 14:47

Add static typeofSlatAngleControlforBlindsValues

142330d

Merge remote-tracking branch 'origin/develop' into multiple-shading-c…

e454bfa

…ontrols

jmarrec reviewed Sep 17, 2020

View reviewed changes

Comment thread src/model/ShadingControl.hpp

Comment thread src/model/ShadingControl.hpp

Comment thread src/model/ShadingControl.hpp

kbenne merged commit d32be89 into develop Sep 18, 2020

kbenne deleted the multiple-shading-controls branch September 18, 2020 15:46

macumber mentioned this pull request Sep 19, 2020

ShadingControl API breakage #4084

Closed

jmarrec mentioned this pull request Sep 21, 2020

Fix #4084 - Maintain backward compatibility in SubSurface::setShadingControl including with C++ interfaces #4086

Merged

20 tasks

jmarrec mentioned this pull request Mar 16, 2021

Update ShadingControl #4103

Closed

20 tasks

joseph-robertson mentioned this pull request Jun 6, 2022

Multiple shading controls for a single subsurface #4602

Open


		std::vector<ShadingControl> shadingControls();

		ShadingControl getShadingControl(unsigned shadingControlIndex);


		bool setShadingControls(const std::vector<ShadingControl> &shadingControls);

		bool insertShadingControl(unsigned shadingControlIndex, const ShadingControl &shadingControl);

Uh oh!

Conversation

joseph-robertson commented Sep 4, 2020 • edited by jmarrec Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

jmarrec commented Sep 4, 2020

Uh oh!

joseph-robertson commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarrec Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarrec Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarrec Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarrec Sep 9, 2020

Choose a reason for hiding this comment

Uh oh!

joseph-robertson Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarrec commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ci-commercialbuildings commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarrec commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shorowit commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joseph-robertson commented Sep 17, 2020

Uh oh!

jmarrec commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbenne commented Sep 18, 2020

Uh oh!

joseph-robertson commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

joseph-robertson commented Sep 4, 2020 •

edited by jmarrec

Loading

joseph-robertson commented Sep 4, 2020 •

edited

Loading

jmarrec left a comment •

edited

Loading

jmarrec commented Sep 15, 2020 •

edited

Loading

ci-commercialbuildings commented Sep 17, 2020 •

edited

Loading

jmarrec commented Sep 17, 2020 •

edited

Loading

shorowit commented Sep 17, 2020 •

edited

Loading

jmarrec commented Sep 17, 2020 •

edited

Loading