-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pulls OSut v0.6.0 + SEB fixes #118
Conversation
brgix
commented
Oct 8, 2024
•
edited
Loading
edited
- Patch - no API changes
- Brings in OSut v0.6.0 (sky branch), recently released
- Harmonizes SEB fixes (as per OSut & TBD Tests)
- Attempts to circumvent (often numerous) Ruby warnings related to duplication of constants
|
||
# The v1.11.5 (2016) seb.osm, shipped with OpenStudio, holds (what would now | ||
# be considered as deprecated) a definition of plenum floors (i.e. ceiling | ||
# tiles) generating several warnings with more recent OpenStudio versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Harmonized with (more detailed) fixes introduced in TBD Tests.
expect(transitions.size).to eq(5) | ||
expect(roof_edges.size).to eq(parapets.size + transitions.size) | ||
|
||
roof_edges.each { |edg| expect(edg[:surfaces].size).to eq(2) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Control tests (to compare against when adding a skylight well, ~Line 2860).
# - 4x new "skylightjamb" edges | ||
# - 4x new "transition" edges around well | ||
# - 1x "transition" edge along leader line, required for well cutout. | ||
sky_jambs = io[:edges].select { |ed| ed[:surfaces].include?(sky_id) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t2x = transitions.select { |edg| edg[:surfaces].size == 2 } | ||
t4x = transitions.select { |edg| edg[:surfaces].size == 4 } | ||
expect(t1x.size).to eq(1) # leader line | ||
expect(t2x.size).to eq(5) # see "can process JSON surface KHI entries" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(~Line 200).
end | ||
|
||
roof_edges = io[:edges].select { |ed| ed[:surfaces].include?(roof_id) } | ||
parapets = roof_edges.select { |ed| ed[:type] == :parapetconvex } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial tests revealed one of the parapet edges was tagged as :parapet (and no longer :parapetconvex). This would normally be inconsequential (they typically inherit the same PSI factor), yet the change didn't pass the smell test. The issue is linked to a (bad) presumption on Topolys wires and vertex management. See psi.rb changes.
inverted = true unless i_terminal == 0 | ||
else | ||
inverted = true unless i_terminal - i_origin == 1 | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A skylight well cutout (or any other cutout) + leader lines combo breaks any expectation that Topolys wire vertex indices follow sequentially, e.g. [0, 1, 2, 3, 4]. With cutouts/leader lines (which need to backtrack), one could encounter e.g. [0, 1, 2, 3, 4, 2, 3, etc.]. For some edges, this would unfortunately break the inverted test, which would subsequently screw up polar vectors and subsequent angle calculations.
All things considered, easier to scrap the test altogether than fix it. In fact the test is no longer warranted given the polygon checks upstream.