NREL / OpenStudio

OpenStudio is a cross-platform collection of software tools to support whole building energy modeling using EnergyPlus and advanced daylight analysis using Radiance.
https://www.openstudio.net/
Other
501 stars 191 forks source link

Allow edges to have same surface on both sides #5003

Closed macumber closed 11 months ago

macumber commented 12 months ago

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:

Review Checklist

This will not be exhaustively relevant to every PR.

jmarrec commented 11 months ago

@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());
jmarrec commented 11 months ago

I guess I botched something in f2c25372e25533c98c8015cb874769600bf779e1 since tests are failing now, but I can't see what I did wrong right now...

jmarrec commented 11 months ago

Ok I see. https://github.com/NREL/OpenStudio/blob/6fb3dedab53cc873f2b9a53417cbc361af335015/src/gbxml/Test/ForwardTranslator_GTest.cpp#L999-L1002

This has zero surfaces.

So the loop goes: for (size_t i = 0; i < -1; ++i), size_t is an unsigned and static_cast<size_t>(-1) ==. 18446744073709551615

ci-commercialbuildings commented 11 months ago

CI Results for d746a33953ef84ac6cfca16c4f5581e663ab56ce: