Wrap ComfortViewFactorAngles by joseph-robertson · Pull Request #5650 · NatLabRockies/OpenStudio

joseph-robertson · 2026-09-08T17:33:06Z

Pull request overview

Closes Wrap ComfortViewFactorAngles #4412
This enables using "Mean Radiant Temperature Calculation Type" other than EnclosureAveraged

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Wrap ComfortViewFactorAngles OpenStudio-resources#233
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

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

joseph-robertson · 2026-09-08T22:32:26Z

Um hang on, why is \object-list ZoneNames in the Name field? And Name needs to be \required-field, right? Also, like ZoneMRTCalculation, in addAngleFactor we need to check that "all surfaces are in the same enclosure"? And A1 jumps to A3 ... ?

ComfortViewFactorAngles,
       \memo Used to specify radiant view factors for thermal comfort calculations.
       \memo Note that the following angle factor fractions must sum up to 1.0
       \memo The number of surfaces can be expanded beyond 100, if necessary, by adding more
       \memo groups to the end of the list.
       \extensible:2
  A1 , \field Name
       \type alpha
       \reference AllHeatTranAngFacNames
       \object-list ZoneNames <--------------------------- ?
  A3 , \field Surface 1 Name
       \type object-list
       \object-list AllHeatTranSurfNames
       \begin-extensible
  N1 ; \field Angle Factor 1
       \type real
       \minimum 0.0
       \maximum 1.0

Copilot

🟡 Changes recommended

Version translation loses existing targets, and the wrapper omits valid target types while permitting inconsistent MRT state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds model and EnergyPlus translation support for ComfortViewFactorAngles and related People MRT calculation modes.

Changes:

Adds the new model object and bindings.
Implements forward/reverse translation and tests.
Moves the MRT target field to PeopleDefinition with version translation.

File summaries

File	Description
`src/osversion/VersionTranslator.cpp`	Migrates People fields to 3.12.
`src/osversion/test/VersionTranslator_GTest.cpp`	Tests People migration.
`src/osversion/test/3_12_0/test_vt_People.rb`	Generates migration fixture.
`src/osversion/test/3_12_0/test_vt_People.osm`	Provides 3.11 fixture data.
`src/model/test/People_GTest.cpp`	Tests new PeopleDefinition API.
`src/model/test/ComfortViewFactorAngles_GTest.cpp`	Tests angle-factor behavior.
`src/model/PeopleDefinition.hpp`	Exposes MRT target API.
`src/model/PeopleDefinition.cpp`	Implements MRT target handling.
`src/model/PeopleDefinition_Impl.hpp`	Declares internal target methods.
`src/model/People_Impl.hpp`	Removes obsolete TODO.
`src/model/ModelGeometry.i`	Adds bindings.
`src/model/Model.cpp`	Registers model constructors.
`src/model/ConcreteModelObjects.hpp`	Includes new model types.
`src/model/ComfortViewFactorAngles.hpp`	Defines the public model API.
`src/model/ComfortViewFactorAngles.cpp`	Implements angle-factor logic.
`src/model/ComfortViewFactorAngles_Impl.hpp`	Defines internal implementation API.
`src/model/CMakeLists.txt`	Builds model sources and tests.
`src/energyplus/Test/People_GTest.cpp`	Tests People translation modes.
`src/energyplus/Test/ComfortViewFactorAngles_GTest.cpp`	Tests object translation.
`src/energyplus/ReverseTranslator/ReverseTranslatePeople.cpp`	Imports MRT targets.
`src/energyplus/ReverseTranslator/ReverseTranslateComfortViewFactorAngles.cpp`	Imports angle-factor lists.
`src/energyplus/ReverseTranslator.hpp`	Declares reverse translation.
`src/energyplus/ReverseTranslator.cpp`	Dispatches reverse translation.
`src/energyplus/ForwardTranslator/ForwardTranslatePeople.cpp`	Exports People MRT settings.
`src/energyplus/ForwardTranslator/ForwardTranslateComfortViewFactorAngles.cpp`	Exports angle-factor lists.
`src/energyplus/ForwardTranslator.hpp`	Declares forward translation.
`src/energyplus/ForwardTranslator.cpp`	Dispatches forward translation.
`src/energyplus/CMakeLists.txt`	Builds translator sources and tests.
`resources/model/OpenStudio.idd`	Adds and relocates model fields.
`resources/energyplus/ProposedEnergy+.idd`	Updates the EnergyPlus schema.

Review details

Suppressed comments (1)

src/model/PeopleDefinition.cpp:286

This rejects valid direct targets supported by EnergyPlus. Surface Name/Angle Factor List Name uses AllHeatTranAngFacNames, which includes fenestration and internal-mass objects as well as base surfaces and angle-factor lists (see resources/energyplus/ProposedEnergy+.idd:7191-7193,8022-8023,11971-11973). Consequently reverse translation silently loses a valid SubSurface or InternalMass target. Support the full target set rather than only Surface and ComfortViewFactorAngles.

      if (modelObject.optionalCast<Surface>()) {
        mrtType = "SurfaceWeighted";
      } else if (modelObject.optionalCast<ComfortViewFactorAngles>()) {
        mrtType = "AngleFactor";
      } else {
        LOG(Error, "Surface Name/Angle Factor List Name must reference a Surface or ComfortViewFactorAngles object.");

Files reviewed: 30/30 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

joseph-robertson · 2026-09-09T16:31:54Z

+            } else if (i > 6) {
+              newObject.setString(i - 1, value.get());


VT shouldn't happen by keeping it on people. Also, this mitigation is uneeded, the field was never connected to anything so we don't care if something "wrote" something manually here

joseph-robertson · 2026-09-09T16:43:56Z

+       \reference AllHeatTranAngFacNames
+  A3,  \field Surface 1 Name
+       \type object-list
+       \object-list SurfaceNames


joseph-robertson · 2026-09-09T16:31:37Z

+      if (result && istringEqual(mrtType, "EnclosureAveraged")) {
+        resetSurfaceNameAngleFactorListName();


jmarrec · 2026-09-09T20:18:59Z

Moves (previously unused) "Surface Name/Angle Factor List Name" from People to People:Definition

Why? I think the specific surfaces are instance specific rather than definition-level, don't you?

(Note: I didn't review the changes yet as I'm on a mobile device)

As far as the IDD inconsistencies, that should be fixed upstream on EnergyPlus with a quick PR

…ld indices

Require a name, declare the minimum field count, and document the enclosure requirement for referenced surfaces.

Validate surface space and thermal-zone assignments before adding angle factors, reject cross-zone lists, and align extensible-group update handling with ZoneMRTCalculation.

Revalidate every referenced surface's thermal zone before generating the EnergyPlus object and cover stale surface assignments in the forward-translator tests.

Reject non-finite angle factors and clear the MRT target when resetting the calculation type, with model regression coverage.

Move unambiguous 3.11 People MRT targets to PeopleDefinition during version translation, report shared-definition conflicts, and cover the migrated surface target.

Accept Surface, SubSurface, and InternalMass targets from AllHeatTranSurfNames, preserve same-zone validation, and cover model and EnergyPlus translation behavior.

github-actions · 2026-09-10T08:45:37Z

🧪 Test Results Dashboard

Summary

Metric	Value
Total Tests	0
Passed	0
Failed	0
Errors	0
Skipped	0
Success Rate	0.0%
Generated	2026-09-15 17:05:04 UTC

❌ Significant Test Failures

📊 Test Run Information

Run	XML File	Status
-	No XML files found	❌ Missing

jmarrec · 2026-09-11T08:20:37Z

+  A6, \field Surface Name/Angle Factor List Name
+       \type object-list
+       \object-list AllHeatTranAngFacNames


Ok, I confirmed what I said/asked before: why move this to People:Definition?!

This is an instance-specific thing. In fact, when I wrapped People:Instance in E+, that's where I put the field too.

I was moving it to People:Definition so it would be alongside Mean Radiant Temperature Calculation Type. Does that not matter?

jmarrec · 2026-09-11T08:28:03Z


    bool isMeanRadiantTemperatureCalculationTypeDefaulted() const;

+    boost::optional<ModelObject> surfaceNameAngleFactorListName() const;


Ok, I'm going to review assuming that this gets moved back to People, not PeopleDefinition.

The only type it will accept / return a ComfortViewFactorAngles, so I don't think there's a reason to use the generic ModelObject here.

Edit: Ok, I read the I/O reference guide: this can also be just a Surface for "SurfaceWeighted". my bad.

jmarrec · 2026-09-11T08:43:00Z

+            } else if (i > 6) {
+              newObject.setString(i - 1, value.get());


VT shouldn't happen by keeping it on people. Also, this mitigation is uneeded, the field was never connected to anything so we don't care if something "wrote" something manually here

jmarrec · 2026-09-11T08:47:26Z

+    boost::optional<AngleFactor> getAngleFactor(unsigned groupIndex) const;
+
+    bool addAngleFactor(const AngleFactor& angleFactor);
+    bool addAngleFactor(const ModelObject& surface, double angleFactor);


technically I think it can only be a Surface or a SubSurface. So PlanarSurface would work... or two overloads?

Ok, InternalMass is possible too. I guess ModelObject is fine then

jmarrec · 2026-09-11T08:53:17Z

+  AngleFactor::AngleFactor(const ModelObject& surface, double angleFactor) : m_surface(surface), m_angleFactor(angleFactor) {
+    if (!((m_angleFactor >= 0.0) && (m_angleFactor <= 1.0))) {
+      LOG_AND_THROW("Angle Factor must be between 0 and 1.");
+    }
+  }


If you end up using the generic ModelObject, then you must validate it's the right type.
Otherwise use three overloads (Surface, SubSurface, InternalMass)

You end up delaying calling isHeatTransferSurface when ComfortViewFactorAngles::addAngleFactor(const AngleFactor& angleFactor) is called, but I think you should throw in the AngleFactor::AngleFactorctor instead

jmarrec · 2026-09-11T08:58:34Z

+      if (auto planarSurface = surface.optionalCast<PlanarSurface>()) {
+        return planarSurface->space();
+      }


(Note: without validation, a ShadingSurface would qualify)

jmarrec · 2026-09-11T09:08:41Z

+      bool surfaceSet = group.setPointer(OS_ComfortViewFactorAnglesExtensibleFields::SurfaceName, surface.handle(), false);
+      if (!surfaceSet) {
+        LOG(Error, "Unable to add AngleFactor which has an incompatible Surface object to " << briefDescription());
+        OS_ASSERT(false);
+      }


If we validate the AngleFactor, this is unecessary and could just be an OS_ASSERT

jmarrec · 2026-09-11T09:14:43Z

+    unsigned int ComfortViewFactorAngles_Impl::numberofAngleFactors() const {
+      return numExtensibleGroups();
+    }


I know we use that in other classes, I'm just thinking about what happens if I delete a Surface after it's been added here... I have an invalid extensible group, and numExtensibleGroups is "wrong"

Yeah, I think for ZoneMRCalculation we handled this -- updated People remove to call removeZoneMRTCalculationReferences. I'll add something similar for Surface, SubSurface, and InternalMass. Your call whether we want to do similar updates for ZonePropertyUserViewFactorsBySurfaceName.

jmarrec · 2026-09-11T12:32:45Z

+%extend openstudio::model::AngleFactor {
+  // Use the overloaded operator<< for string representation
+  std::string __str__() {
+    std::ostringstream os;
+    os << *$self;
+    return os.str();
+  }
+};


Nice, and ModelGeometry.i is the right file to add it to (needs Surface)

jmarrec · 2026-09-11T12:37:14Z

+    void PeopleDefinition_Impl::resetSurfaceNameAngleFactorListName() {
+      bool result = setString(OS_People_DefinitionFields::SurfaceName_AngleFactorListName, "");
+      OS_ASSERT(result);


This is moot if not consolidating MRT Calculation Type and Surface Name/Angle Facor List on a single object.

But anyways: given the enforcing/switching the mrt calculation type you do, here you would need to reset to EnclosureAveraged. and the setMeanRadiantTemperature should be removed from the public interface.

(The enforcing/switching is probably the reason you thought about moving the AngleFactorList to the PeopleDefinition object... If we really think we should move something, I think it'd be the MRT Calculation Type moving to the Instance though)

While on the topic, the MRT Calculation Type and Surface Name/Angle Facor List are ONLY read by E+ if using thermal comfort models: Fanger, Pierce, KSU, CoolingEffectASH55 or AnkleDraftASH55

https://github.com/NatLabRockies/EnergyPlus/blob/f7c103167afdd833d3c5f97e4d42f5a2f9d7a18c/src/EnergyPlus/InternalHeatGains.cc#L608-L663

OK, so basically the two options are (?):

List on People and CalcType on PeopleDefinition; resetList shouldn't change CalcType, or

both List/CalcType on People; resetList should change CalcType

jmarrec · 2026-09-11T12:48:29Z

+    std::string mrtType = definition.meanRadiantTemperatureCalculationType();
+    if (auto target = definition.surfaceNameAngleFactorListName()) {
+      if (target->optionalCast<Surface>()) {
+        if (auto idfTarget = translateAndMapModelObject(*target)) {
+          mrtType = "SurfaceWeighted";
+          idfObject.setString(PeopleFields::SurfaceName_AngleFactorListName, idfTarget->nameString());
+        } else {
+          mrtType = "EnclosureAveraged";
+        }
+      } else if (target->optionalCast<ComfortViewFactorAngles>()) {
+        if (auto idfTarget = translateAndMapModelObject(*target)) {
+          mrtType = "AngleFactor";
+          idfObject.setString(PeopleFields::SurfaceName_AngleFactorListName, idfTarget->nameString());
+        } else {
+          mrtType = "EnclosureAveraged";
+        }
+      } else {
+        mrtType = "EnclosureAveraged";
+      }
+    } else if (!istringEqual(mrtType, "EnclosureAveraged")) {
+      mrtType = "EnclosureAveraged";


If this is enforced at model time, there's no need to recheck for it.
If we do check/recheck for it in FT, we should Warn for inconsistencies.

jmarrec · 2026-09-11T12:59:48Z

There will be a slight conflict with ForwardTranslate to new E+ SpaceLoad Instances + Definitions (opt-out possible) #5651
- I add a path where it will translate to People:Instance + People:Definition, and that'll need the Surface Name/Angle List Name field translated in there too

joseph-robertson · 2026-09-11T14:47:54Z

There will be a slight conflict with ForwardTranslate to new E+ SpaceLoad Instances + Definitions (opt-out possible) #5651

I add a path where it will translate to People:Instance + People:Definition, and that'll need the Surface Name/Angle List Name field translated in there too

Does this mean we should square away this PR, and then move on to #5651?

jmarrec · 2026-09-14T10:22:42Z

  The following tests FAILED:
  	3373 - EnergyPlusFixture.ForwardTranslator_People_MRTTypeWithoutTargetDefaultsToEnclosureAveraged (Failed)
  	3374 - EnergyPlusFixture.ForwardTranslator_People_UntranslatedMRTTargetDefaultsToEnclosureAveraged (Failed)

jmarrec · 2026-09-14T10:25:19Z

Does this mean we should square away this PR, and then move on to #5651?

One or the other first. I think #5651 is ready to merge though.

joseph-robertson self-assigned this Sep 8, 2026

joseph-robertson added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Sep 8, 2026

joseph-robertson mentioned this pull request Sep 8, 2026

Wrap ZoneMRTCalculation NatLabRockies/OpenStudio-resources#232

Merged

42 tasks

joseph-robertson marked this pull request as ready for review September 8, 2026 22:21

joseph-robertson requested a review from jmarrec September 8, 2026 22:21

joseph-robertson requested a balanced review from Copilot September 9, 2026 15:30

Copilot started reviewing on behalf of joseph-robertson September 9, 2026 15:31 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Base automatically changed from v26.2.0-IOFreeze-ZoneMRTCalc to develop September 10, 2026 08:44

joseph-robertson added 18 commits September 10, 2026 10:45

Add ComfortViewFactorAngles model schema

848afbb

Add ComfortViewFactorAngles model object

6c9beaa

Translate ComfortViewFactorAngles for People

2fe1fb5

Import ComfortViewFactorAngles from EnergyPlus

f674777

Translate People MRT target schema

54530d7

Test ComfortViewFactorAngles translation

1c97302

Register ComfortViewFactorAngles translation

eb7fe45

Clean up angle factor name.

96adc29

Update ComfortViewFactorAngles schema

d536120

Refine ComfortViewFactorAngles model API

75bd4dd

Refine ComfortViewFactorAngles translation

cef6abb

Fix ComfortViewFactorAngles model integration

9a9d173

Add direct dependencies to ComfortViewFactorAngles translator tests

25b1e1a

Correct ComfortViewFactorAngles translation test paths and People fie…

85e090a

…ld indices

Update ComfortViewFactorAngles EnergyPlus IDD metadata

b0f4b5c

Require a name, declare the minimum field count, and document the enclosure requirement for referenced surfaces.

Require ComfortViewFactorAngles surfaces in one thermal zone

2a92c96

Validate surface space and thermal-zone assignments before adding angle factors, reject cross-zone lists, and align extensible-group update handling with ZoneMRTCalculation.

Reject cross-zone ComfortViewFactorAngles during translation

25785c8

Revalidate every referenced surface's thermal zone before generating the EnergyPlus object and cover stale surface assignments in the forward-translator tests.

Harden ComfortViewFactorAngles and MRT target validation

b857c7b

Reject non-finite angle factors and clear the MRT target when resetting the calculation type, with model regression coverage.

joseph-robertson added 2 commits September 10, 2026 10:45

Migrate legacy People MRT targets to definitions

24d1ad7

Move unambiguous 3.11 People MRT targets to PeopleDefinition during version translation, report shared-definition conflicts, and cover the migrated surface target.

Support all heat-transfer surfaces in ComfortViewFactorAngles

f940f2e

Accept Surface, SubSurface, and InternalMass targets from AllHeatTranSurfNames, preserve same-zone validation, and cover model and EnergyPlus translation behavior.

jmarrec force-pushed the comfort-view-factor-angles branch from 0b23e76 to f940f2e Compare September 10, 2026 08:45

jmarrec mentioned this pull request Sep 10, 2026

Wrap ComfortViewFactorAngles NatLabRockies/OpenStudio-resources#233

Open

26 tasks

jmarrec requested changes Sep 11, 2026

View reviewed changes

jmarrec reviewed Sep 11, 2026

View reviewed changes

joseph-robertson added 9 commits September 11, 2026 11:07

Move People MRT target API to People

04c15df

Simplify AngleFactor validation and access

0a80b4d

Clean angle factors when surfaces are removed

8b3e3bc

Update People MRT translation and warnings

6b03103

Remove obsolete People MRT version translation

91e212a

Fix model cleanup dependencies

136a495

Expand ComfortViewFactorAngles test coverage

14dfde9

Align version translation test field indices

02b8055

Clarify People thermal comfort field notes

8b6b47f

joseph-robertson added 6 commits September 14, 2026 11:00

Add people to space for new ft tests.

0bd2301

Merge branch 'develop' into comfort-view-factor-angles

733f5f6

Merge branch 'develop' into comfort-view-factor-angles

a9133a1

Use setExcludeSpaceLoadInstances true for legacy tests.

01c9d11

Translate People instance MRT targets

8f1ed58

Test People instance MRT target override

5802a09

joseph-robertson requested a review from jmarrec September 15, 2026 22:52

Merge branch 'develop' into comfort-view-factor-angles

c8c9e17

		if (result && istringEqual(mrtType, "EnclosureAveraged")) {
		resetSurfaceNameAngleFactorListName();


		bool isMeanRadiantTemperatureCalculationTypeDefaulted() const;

		boost::optional<ModelObject> surfaceNameAngleFactorListName() const;

Conversation

joseph-robertson commented Sep 8, 2026 • edited 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!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 11, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Sep 9, 2026

Uh oh!

github-actions Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 Test Results Dashboard

Summary

❌ Significant Test Failures

📊 Test Run Information

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!

jmarrec Sep 11, 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!

joseph-robertson Sep 11, 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

joseph-robertson commented Sep 8, 2026 •

edited

Loading

joseph-robertson commented Sep 8, 2026 •

edited

Loading

jmarrec Sep 11, 2026 •

edited

Loading

github-actions Bot commented Sep 10, 2026 •

edited

Loading

jmarrec Sep 11, 2026 •

edited

Loading

joseph-robertson Sep 11, 2026 •

edited

Loading