Skip to content
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

removed RotateUp and uint #1000

Merged
merged 8 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions include/manifold/manifold.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,4 +323,50 @@ class Manifold {
CsgLeafNode& GetCsgLeafNode() const;
};
/** @} */

/** @defgroup Debug
* @brief Debugging features
*
* The features require compiler flags to be enabled. Assertions are enabled
* with the MANIFOLD_DEBUG flag and then controlled with ExecutionParams.
* @{
*/
#ifdef MANIFOLD_DEBUG
inline std::string ToString(const Manifold::Error& error) {
switch (error) {
case Manifold::Error::NoError:
return "No Error";
case Manifold::Error::NonFiniteVertex:
return "Non Finite Vertex";
case Manifold::Error::NotManifold:
return "Not Manifold";
case Manifold::Error::VertexOutOfBounds:
return "Vertex Out Of Bounds";
case Manifold::Error::PropertiesWrongLength:
return "Properties Wrong Length";
case Manifold::Error::MissingPositionProperties:
return "Missing Position Properties";
case Manifold::Error::MergeVectorsDifferentLengths:
return "Merge Vectors Different Lengths";
case Manifold::Error::MergeIndexOutOfBounds:
return "Merge Index Out Of Bounds";
case Manifold::Error::TransformWrongLength:
return "Transform Wrong Length";
case Manifold::Error::RunIndexWrongLength:
return "Run Index Wrong Length";
case Manifold::Error::FaceIDWrongLength:
return "Face ID Wrong Length";
case Manifold::Error::InvalidConstruction:
return "Invalid Construction";
default:
return "Unkown Error";
};
}

inline std::ostream& operator<<(std::ostream& stream,
const Manifold::Error& error) {
return stream << ToString(error);
}
#endif
/** @} */
} // namespace manifold
22 changes: 11 additions & 11 deletions src/boolean_result.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -669,23 +669,23 @@ Manifold::Impl Boolean3::Result(OpType op) const {
const int c2 = op == OpType::Add ? 1 : 0;
const int c3 = op == OpType::Intersect ? 1 : -1;

if (inP_.status_ != Manifold::Error::NoError) {
auto impl = Manifold::Impl();
impl.status_ = inP_.status_;
return impl;
}
if (inQ_.status_ != Manifold::Error::NoError) {
auto impl = Manifold::Impl();
impl.status_ = inQ_.status_;
return impl;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to pass through the input error instead of changing it, which I think makes it a be easier to deduce what went wrong first.

}

if (inP_.IsEmpty()) {
if (inP_.status_ != Manifold::Error::NoError ||
inQ_.status_ != Manifold::Error::NoError) {
auto impl = Manifold::Impl();
impl.status_ = Manifold::Error::InvalidConstruction;
return impl;
}
if (!inQ_.IsEmpty() && op == OpType::Add) {
return inQ_;
}
return Manifold::Impl();
} else if (inQ_.IsEmpty()) {
if (inQ_.status_ != Manifold::Error::NoError) {
auto impl = Manifold::Impl();
impl.status_ = Manifold::Error::InvalidConstruction;
return impl;
}
if (op == OpType::Intersect) {
return Manifold::Impl();
}
Expand Down
2 changes: 1 addition & 1 deletion src/csg_tree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ Manifold::Impl CsgLeafNode::Compose(
for (auto &node : nodes) {
if (node->pImpl_->status_ != Manifold::Error::NoError) {
Manifold::Impl impl;
impl.status_ = Manifold::Error::InvalidConstruction;
impl.status_ = node->pImpl_->status_;
return impl;
}
double nodeOldScale = node->pImpl_->bBox_.Scale();
Expand Down
4 changes: 4 additions & 0 deletions src/impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,10 @@ Manifold::Impl Manifold::Impl::Transform(const mat3x4& transform_) const {
result.status_ = status_;
return result;
}
if (!all(la::isfinite(transform_))) {
result.MarkFailure(Error::NonFiniteVertex);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to catch any kind of bad transform.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently we need to make it explicit, i.e. all(la::isfinite(transform_)) to make msvc happy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, in BRL-CAD when we use C++ we generally are explicit with the namespaces to avoid lookup surprises. It's definitely a readability tradeoff though.

return result;
}
result.collider_ = collider_;
result.meshRelation_ = meshRelation_;
result.precision_ = precision_;
Expand Down
4 changes: 2 additions & 2 deletions src/meshIO/meshIO.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ aiScene* CreateScene(const ExportOptions& options) {

scene->mRootNode = new aiNode();
scene->mRootNode->mNumMeshes = 1;
scene->mRootNode->mMeshes = new uint[scene->mRootNode->mNumMeshes];
scene->mRootNode->mMeshes = new uint32_t[scene->mRootNode->mNumMeshes];
scene->mRootNode->mMeshes[0] = 0;

scene->mMeshes[0]->mPrimitiveTypes = aiPrimitiveType_TRIANGLE;
Expand Down Expand Up @@ -251,7 +251,7 @@ void ExportMesh(const std::string& filename, const MeshGL& mesh,
for (size_t i = 0; i < mesh_out->mNumFaces; ++i) {
aiFace& face = mesh_out->mFaces[i];
face.mNumIndices = 3;
face.mIndices = new uint[face.mNumIndices];
face.mIndices = new uint32_t[face.mNumIndices];
for (int j : {0, 1, 2}) face.mIndices[j] = mesh.triVerts[3 * i + j];
}

Expand Down
6 changes: 1 addition & 5 deletions src/properties.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,16 +126,12 @@ struct UpdateProperties {

struct CheckHalfedges {
VecView<const Halfedge> halfedges;
VecView<const vec3> vertPos;

bool operator()(size_t edge) const {
const Halfedge halfedge = halfedges[edge];
if (halfedge.startVert == -1 || halfedge.endVert == -1) return true;
if (halfedge.pairedHalfedge == -1) return false;

if (!std::isfinite(vertPos[halfedge.startVert][0])) return false;
if (!std::isfinite(vertPos[halfedge.endVert][0])) return false;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure when this showed up, but we have a dedicated IsFinite method which was being elided by this throwing a NonManifold error, which was pretty unclear.


const Halfedge paired = halfedges[halfedge.pairedHalfedge];
bool good = true;
good &= paired.pairedHalfedge == static_cast<int>(edge);
Expand Down Expand Up @@ -199,7 +195,7 @@ namespace manifold {
bool Manifold::Impl::IsManifold() const {
if (halfedge_.size() == 0) return true;
return all_of(countAt(0_uz), countAt(halfedge_.size()),
CheckHalfedges({halfedge_, vertPos_}));
CheckHalfedges({halfedge_}));
}

/**
Expand Down
15 changes: 0 additions & 15 deletions src/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -230,21 +230,6 @@ inline int CCW(vec2 p0, vec2 p1, vec2 p2, double tol) {
return area > 0 ? 1 : -1;
}

/**
* This 4x3 matrix can be used as an input to Manifold.Transform() to turn an
* object. Turns along the shortest path from given up-vector to (0, 0, 1).
*
* @param up The vector to be turned to point upwards. Length does not matter.
*/
inline mat3x4 RotateUp(vec3 up) {
up = la::normalize(up);
const vec3 axis = la::cross(up, {0, 0, 1});
double angle = la::asin(la::length(axis));
if (la::dot(up, {0, 0, 1}) < 0) angle = kPi - angle;
const quat q = la::rotation_quat(la::normalize(axis), angle);
return mat3x4(la::qmat(q), vec3());
}

inline mat4 Mat4(mat3x4 a) {
return mat4({a[0], 0}, {a[1], 0}, {a[2], 0}, {a[3], 1});
}
Expand Down
22 changes: 4 additions & 18 deletions test/boolean_complex_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#ifdef MANIFOLD_CROSS_SECTION
#include "manifold/cross_section.h"
#endif
#include "../src/utils.h" // For RotateUp
#include "manifold/manifold.h"
#include "manifold/polygon.h"
#include "test.h"
Expand Down Expand Up @@ -1053,13 +1052,8 @@ TEST(BooleanComplex, SimpleOffset) {
vec3 vpos(seeds.vertProperties[3 * i + 0], seeds.vertProperties[3 * i + 1],
seeds.vertProperties[3 * i + 2]);
Manifold vsph = sph.Translate(vpos);
if (!vsph.NumTri()) continue;
c += vsph;
// See above discussion
EXPECT_EQ(c.Status(), Manifold::Error::NoError);
}
// See above discussion
// EXPECT_EQ(c.Status(), Manifold::Error::NoError);
// Edge Cylinders
for (size_t i = 0; i < edges.size(); i++) {
vec3 ev1 = vec3(seeds.vertProperties[3 * edges[i].first + 0],
Expand All @@ -1071,20 +1065,12 @@ TEST(BooleanComplex, SimpleOffset) {
vec3 edge = ev2 - ev1;
double len = la::length(edge);
if (len < std::numeric_limits<float>::min()) continue;
// TODO - workaround, shouldn't be necessary
if (len < 0.03) continue;
manifold::Manifold origin_cyl = manifold::Manifold::Cylinder(len, 1, 1, 8);
vec3 evec(-1 * edge.x, -1 * edge.y, edge.z);
manifold::Manifold rotated_cyl =
origin_cyl.Transform(manifold::RotateUp(evec));
manifold::Manifold right = rotated_cyl.Translate(ev1);
if (!right.NumTri()) continue;
quat q = rotation_quat(normalize(evec), vec3(0, 0, 1));
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pca006132 I was a little suprised I didn't need la::rotation_quat or manifold::la::rotation_quat. I recall you made a comment about my namespacing - did I do it wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a weird thing called Argument-dependent lookup for functions, which basically adds the argument type namespaces into the lookup search path. I don't think we have anything wrong with our namespacing.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's cool! Thanks for the explanation.

manifold::Manifold right = origin_cyl.Transform({la::qmat(q), ev1});
c += right;
// See above discussion
EXPECT_EQ(c.Status(), Manifold::Error::NoError);
}
// See above discussion
// EXPECT_EQ(c.Status(), Manifold::Error::NoError);
// Triangle Volumes
for (size_t i = 0; i < seeds.NumTri(); i++) {
int eind[3];
Expand All @@ -1098,6 +1084,7 @@ TEST(BooleanComplex, SimpleOffset) {
vec3 a = ev[0] - ev[2];
vec3 b = ev[1] - ev[2];
vec3 n = la::normalize(la::cross(a, b));
if (!all(isfinite(n))) continue;
// Extrude the points above and below the plane of the triangle
vec3 pnts[6];
for (int j = 0; j < 3; j++) pnts[j] = ev[j] + n;
Expand All @@ -1123,13 +1110,12 @@ TEST(BooleanComplex, SimpleOffset) {
for (int j = 0; j < 24; j++)
tri_m.triVerts.insert(tri_m.triVerts.end(), faces[j]);
manifold::Manifold right(tri_m);
if (!right.NumTri()) continue;
c += right;
// See above discussion
EXPECT_EQ(c.Status(), Manifold::Error::NoError);
}
// See above discussion
// EXPECT_EQ(c.Status(), Manifold::Error::NoError);
EXPECT_EQ(c.Status(), Manifold::Error::NoError);
}

#endif
4 changes: 2 additions & 2 deletions test/samples_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#ifdef MANIFOLD_CROSS_SECTION
#include "manifold/cross_section.h"
#endif
#include "../src/utils.h"
#include "manifold/polygon.h"
#include "test.h"

Expand Down Expand Up @@ -147,7 +146,8 @@ TEST(Samples, TetPuzzle) {

Manifold puzzle2 = puzzle.Rotate(0, 0, 180);
EXPECT_TRUE((puzzle ^ puzzle2).IsEmpty());
puzzle = puzzle.Transform(RotateUp({1, -1, -1}));
quat q = rotation_quat(normalize(vec3(1, -1, -1)), vec3(0, 0, 1));
puzzle = puzzle.Transform({la::qmat(q), vec3()});
#ifdef MANIFOLD_EXPORT
if (options.exportModels) ExportMesh("tetPuzzle.glb", puzzle.GetMeshGL(), {});
#endif
Expand Down
4 changes: 1 addition & 3 deletions test/smooth_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

#include <algorithm>

#include "../src/utils.h"
#ifdef MANIFOLD_CROSS_SECTION
#include "manifold/cross_section.h"
#endif
Expand Down Expand Up @@ -324,8 +323,7 @@ TEST(Smooth, Torus) {
vec3 tan(v.y, -v.x, 0);
tan *= la::dot(tan, edge) < 0 ? -1.0 : 1.0;
tangent = CircularTangent(tan, edge);
} else if (std::abs(la::determinant(mat2(vec2(v), vec2(edge)))) <
kTolerance) {
} else if (std::abs(la::determinant(mat2(vec2(v), vec2(edge)))) < 1e-5) {
const double theta = std::asin(v.z);
vec2 xy(v);
const double r = la::length(xy);
Expand Down
Loading