extract polygon holes when converting initial shape geometry #246
Conversation
mistafunk
force-pushed
the
shaegler/initial-shapes-with-holes
branch
2 times, most recently
from
8de5fc0 to
b999ced
Compare
mistafunk
force-pushed
the
shaegler/initial-shapes-with-holes
branch
from
b999ced to
b29a734
Compare
There was a problem hiding this comment.
Nice work!
I think we should generalize the code such that we can test it. There is also some room for improvement in naming/organization (some of it is opinionated and you can leave it if you feel strongly about some of the structure/names), but overall great stuff!
src/palladio/ShapeConverter.cpp
Outdated
| FaceWithHoles info(1); // insert the (yet empty) actual face | ||
|
|
||
| size_t ringIdx = 0; // stack-like indexing to jump into the holes and back out again | ||
| for (auto const& edge : edges) { |
There was a problem hiding this comment.
traversing like this will push the holes in the order houdini creates the bridge edges which is arbitrary. This is probably fine, but maybe the holes should be sorted in a particular order independent on where the bridge edges lie (maybe there is some heuristic with lowest/highest point index that would restore the original order? Maybe sidefx ppl know more..)
src/palladio/ShapeConverter.cpp
Outdated
| FaceWithHoles extractHoles(GEO_Face const& face) { | ||
| const auto [bridges, edges] = detectBridgeEdges(face); | ||
| if (bridges.empty()) | ||
| return {}; |
There was a problem hiding this comment.
Maybe we could simplify this by just returning traverseBridgeEdges...
We could also have a struct where face: vector<GA_Offset>, holes: vector<vector<GA_Offset>> are properly separated so we don't need to know implementation details when interacting with it.
I understand that in the no-bridge case the vertex indices are just ascending and storing that seems a bit like a waste.
But I still think It's a bit odd to either return face + holes or nothing at all. Maybe this could at least be documented why, if we leave it as-is? (That vertex order is expected to be 1,2,...,n in no-hole case, but not with holes)
I'll let you grok on it for a bit :)
No description provided.