-
Notifications
You must be signed in to change notification settings - Fork 192
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
Allow edges to have same surface on both sides #5003
Conversation
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.
Just going to make a dumb change to retrigger a CI pass
@macumber thanks for the fix. I wonder if we shouldn't just erase these edges in the polyhedron instead of retaining them and putting them being shared twice to the same surface. Have you tried it by any chance? Edit: I toyed with the idea of removing them directly in Surface3d 's ctor, but that doesn't work because the Surface3d isn't convex in that case Tried implementation, at end of current Ctor: // #5002 - special case to find and remove edges that are used to "cut" in to a surface to remove an interior hole
const bool keep_edge = true;
std::vector<bool> mask = std::vector<bool>(edges.size(), keep_edge);
for (size_t i = 0; i < edges.size() - 1; ++i) {
for (size_t j = i + 1; j < edges.size(); ++j) {
if (edges[i].reverseEqual(edges[j])) {
mask.at(i) = false;
mask.at(j) = false;
}
}
}
edges.erase(std::remove_if(edges.begin(), edges.end(), [this, &mask](const auto& edge) { return mask.at(&edge - edges.data()); }), edges.end()); |
for (size_t i = 0; i < edges.size(); ++i) { | ||
for (size_t j = 0; j < edges.size(); ++j) { |
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.
I know I'm guilty here because I've done it a couple of lines above when looping on surfaces, but we can avoid traversing that much by just doing combinations (instead of permutations)
for (size_t i = 0; i < edges.size(); ++i) { | |
for (size_t j = 0; j < edges.size(); ++j) { | |
for (size_t i = 0; i < edges.size() - 1; ++i) { | |
for (size_t j = i + 1; j < edges.size(); ++j) { |
…ces/edges Testbed / demo: https://godbolt.org/z/E7n88WP34
I guess I botched something in f2c2537 since tests are failing now, but I can't see what I did wrong right now... |
Ok I see. OpenStudio/src/gbxml/Test/ForwardTranslator_GTest.cpp Lines 999 to 1002 in 6fb3ded
This has zero surfaces. So the loop goes: |
static_cast<size_t>(-1) is a very large unsigned number. We'll just be iterating an integer once too many time, which is totally unoticable
CI Results for d746a33:
|
Pull request overview
Supports polygons with interior holes cut out with reverse winding, this fixes a regression from 3.6.0 that was discovered in 3.7.0-rc1.
Pull Request Author
No API or IDD changes needed
Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.