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

Adds a Quadrilateral primitive #1181

Merged
merged 26 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
246b066
Add Quadrilateral primitive, operators, and tests
adayton1 Sep 8, 2023
1c4d0e8
Merge branch 'develop' into feature/dayton8/primal_quadrilateral
adayton1 Sep 11, 2023
1d1ed9b
Use right hand coordinate system for quadrilateral
adayton1 Sep 11, 2023
f941938
Fix some documentation
adayton1 Sep 11, 2023
e72cfdf
Fix build issues
adayton1 Sep 11, 2023
26d8f58
Clang format fixes
adayton1 Sep 12, 2023
620a9c0
Update doxygen for BoundingBox::intersectsWith
adayton1 Sep 12, 2023
3a07c13
Add test cases
adayton1 Sep 12, 2023
29063e7
Undo changes to BoundingBox
adayton1 Sep 12, 2023
a1e130a
Remove constructor taking raw array
adayton1 Sep 12, 2023
8e64ec4
Simplify initialization
adayton1 Sep 12, 2023
27b1bc7
Simplify compute_bounding_box operators
adayton1 Sep 12, 2023
7e6bcd4
Add compute_bounding_box test for 3D quad
adayton1 Sep 12, 2023
b36e7e9
Add tests for quadrilateral constructors
adayton1 Sep 12, 2023
dda0249
Use correct gtest macro
adayton1 Sep 12, 2023
e517fa0
Formatting
adayton1 Sep 12, 2023
50aab4e
Fix up default constructor
adayton1 Sep 12, 2023
2ece781
Formatting
adayton1 Sep 12, 2023
40e7e55
Add note about investigating other algorithms for computing area of q…
adayton1 Sep 12, 2023
7fcf84e
Fix warnings and build errors
adayton1 Sep 13, 2023
a8519b1
Fix syntax errors
adayton1 Sep 13, 2023
aa6c669
Use explicit constructor
adayton1 Sep 13, 2023
170a3c5
Use member initializer and compiler generated default constructor
adayton1 Sep 14, 2023
d70bd56
Merge branch 'develop' into feature/dayton8/primal_quadrilateral
adayton1 Sep 14, 2023
2babdfa
Merge branch 'develop' into feature/dayton8/primal_quadrilateral
adayton1 Sep 28, 2023
928f7bb
Updates the release notes
adayton1 Sep 28, 2023
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
3 changes: 3 additions & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ The Axom project release numbers follow [Semantic Versioning](http://semver.org/
returns the signed volume.
- Primal: `intersection_volume()` operators changed from returning a signed
volume to an unsigned volume.
- Primal: Adds a `Quadrilateral` primitive
- Primal: Adds a `compute_bounding_box()` operator for computing the bounding
box of a `Quadrilateral`

### Fixed
- quest's `SamplingShaper` now properly handles material names containing underscores
Expand Down
1 change: 1 addition & 0 deletions src/axom/primal/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ set( primal_headers
geometry/Tetrahedron.hpp
geometry/Octahedron.hpp
geometry/Triangle.hpp
geometry/Quadrilateral.hpp
geometry/Vector.hpp

## operators
Expand Down
2 changes: 2 additions & 0 deletions src/axom/primal/docs/sphinx/primitive.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ Primal includes the following primitives:
- Point
- Segment, Ray, Vector
- Plane, Triangle, Polygon
- Quadrilateral
- Sphere
- Tetrahedron
- Hexahedron
- BoundingBox, OrientedBoundingBox

Primal also provides the NumericArray class, which implements arithmetic
Expand Down
23 changes: 17 additions & 6 deletions src/axom/primal/geometry/BoundingBox.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,17 @@ class BoundingBox
AXOM_HOST_DEVICE
BoundingBox(const PointType* pts, int n);

/*!
* \brief Constructor. Creates a bounding box containing the
* initializer list of points.
*
* \param [in] pts an initializer list containing points
*/
AXOM_HOST_DEVICE
explicit BoundingBox(std::initializer_list<PointType> pts)
Copy link
Member Author

Choose a reason for hiding this comment

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

Design question: Will this constructor get confused with the constructor that takes a min and max point? BoundingBox<double, 1> {Point<double, 1> {1.0}, Point<double, 1> {0.0}}

Copy link
Member

Choose a reason for hiding this comment

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

since you use 'explicit', I don't believe so. I may be wrong but I believe the compiler interprets and initializer_list as a single object while the ctor taking two points is considered a different overload. I recommend trying a simple example with several different compilers to be sure.

Copy link
Member

@rhornung67 rhornung67 Sep 14, 2023

Choose a reason for hiding this comment

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

This may provide some insight: https://en.cppreference.com/w/cpp/language/converting_constructor

In particular, there is a distinction between copy-list initialization and direct-list-initialization, which implies the behavior is influenced by how the users calls the ctor.

Copy link
Member Author

@adayton1 adayton1 Sep 14, 2023

Choose a reason for hiding this comment

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

It could be a little confusing and it does lead to a subtle change in behavior.

BoundingBox{Point {}, Point {}}; // Previously called two point constructor, now calls initializer_list constructor 
BoundingBox(Point {}, Point {}); // Calls two point constructor

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be better to have a make_bounding_box free function?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose the constructor from two points can be more computationally efficient when you know that one point is the min and the other is the max, and you explicitly use the third parameter (shouldFixBounds=false) -- but obviously this is explicit.
Otherwise, we should get the same result and I also wouldn't expect a meaningful performance difference (although, I suppose we should benchmark/profile it if it matters):

// 1: explicit .ctor -- explicitly set min and max, but then check and fix
BoundingBoxType bbox (point1, point2, /* shouldFixBounds=*/ true);
// 2: explicit .ctor -- most efficient, but possibly unsafe
BoundingBoxType bbox (point1, point2, /* shouldFixBounds=*/ false);
// 3: initializer-list -- will ensure that bounds are correct by inserting points one at a time
BoundingBoxType bbox {point1, point2}; 
// 4: .ctor -- implicitly sets shouldFixBounds to true, 
// performance diff to initializer list unknown, but likely negligible; should be measured if it matters
BoundingBoxType bbox (point1, point2); 

Copy link
Member

Choose a reason for hiding this comment

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

@adayton1 thanks for confirming. That's what I would expect to happen. What do you get if you do this?

BoundingBox foo = {Point{}, Point{}}

Maybe it is sufficient to note the difference in the comments??

Copy link
Member Author

Choose a reason for hiding this comment

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

BoundingBoxType foo = {Point{}, Point{}} won't compile since both constructors are explicit.
BoundingBoxType foo = BoundingBoxType{Point{}, Point{}} will use initializer_list constructor.
BoundingBoxType foo = BoundingBoxType(Point{}, Point{}) will use two point constructor.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @adayton1 -- that looks good to me.

Presumably, this would be the same as your first line?:

BoundingBoxType foo {Point{}, Point{}};

... but this would still work?

BoundingBoxType foo (Point{}, Point{});

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming @adayton1 That's exactly what I would expect.

@kennyweiss they are different. Alan's first line (that doesn't compile) is copy-initialization, where as yours is direct-initialization. The first uses a copy/move ctor. The second uses a regular ctor.

: BoundingBox {pts.begin(), static_cast<int>(pts.size())}
{ }

/*!
* \brief Constructor. Creates a bounding box with a given min and max point
* The code ensures that the bounds are valid, if shouldFixBounds is true.
Expand Down Expand Up @@ -129,14 +140,14 @@ class BoundingBox
* \return const reference to the min corner of the bounding box.
*/
AXOM_HOST_DEVICE
const PointType& getMin() const { return m_min; };
const PointType& getMin() const { return m_min; }

/*!
* \brief Returns const reference to the max corner of the bounding box.
* \return const reference to the max corner of the bounding box.
*/
AXOM_HOST_DEVICE
const PointType& getMax() const { return m_max; };
const PointType& getMax() const { return m_max; }

/*!
* \brief Returns the centroid (midpoint) of the bounding box.
Expand All @@ -150,7 +161,7 @@ class BoundingBox
* \return Vector from min point to max point of bounding box.
*/
AXOM_HOST_DEVICE
VectorType range() const { return VectorType(m_min, m_max); };
VectorType range() const { return VectorType(m_min, m_max); }

/*!
* \brief Updates bounds to include the provided point.
Expand All @@ -173,7 +184,7 @@ class BoundingBox
* \post d >= 1.
*/
AXOM_HOST_DEVICE
int dimension() const { return NDIMS; };
int dimension() const { return NDIMS; }

/*!
* \brief Finds the longest dimension of the bounding box
Expand Down Expand Up @@ -344,7 +355,7 @@ class BoundingBox
AXOM_HOST_DEVICE inline void setMin(const PointType& newMin)
{
m_min = newMin;
};
}

/*!
* \brief Sets the max point for this bounding box instance.
Expand All @@ -353,7 +364,7 @@ class BoundingBox
AXOM_HOST_DEVICE inline void setMax(const PointType& newMax)
{
m_max = newMax;
};
}

/*!
* \brief Ensures that the bounds are valid.
Expand Down
249 changes: 249 additions & 0 deletions src/axom/primal/geometry/Quadrilateral.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
// Copyright (c) 2017-2023, Lawrence Livermore National Security, LLC and
// other Axom Project Developers. See the top-level LICENSE file for details.
//
// SPDX-License-Identifier: (BSD-3-Clause)

#ifndef AXOM_PRIMAL_QUADRILATERAL_HPP_
#define AXOM_PRIMAL_QUADRILATERAL_HPP_

#include "axom/config.hpp"
#include "axom/core.hpp"
#include "axom/slic/interface/slic.hpp"
#include "axom/fmt.hpp"

#include "axom/primal/constants.hpp"
#include "axom/primal/geometry/Point.hpp"
#include "axom/primal/geometry/Triangle.hpp"

#include <cmath>
#include <ostream>

namespace axom
{
namespace primal
{
// Forward declare the templated classes and operator functions
template <typename T, int NDIMS>
class Quadrilateral;

/**
* \brief Overloaded output operator for quadrilaterals
*/
template <typename T, int NDIMS>
std::ostream& operator<<(std::ostream& os, const Quadrilateral<T, NDIMS>& quad);

/*!
* \accelerated
* \class Quadrilateral
*
* \brief Represents a quadrilateral geometric shape defined by four points.
*
* \accelerated
*
* \tparam T the coordinate type, e.g., double, float, etc.
* \tparam NDIMS the number of dimensions
*
* There are four vertices in the quadrilateral, labeled A through D as the
* constructor's arguments. They are accessible using the square-brackets
* operator, with A being index 0, B index 1, C index 2, and D index 3.
*
* Here's a diagram showing a square with the labeled vertices.
*
* \verbatim
*
* D +---------+ C +y
* | |
* | | ^
* | | |
* | | |
* A +---------+ B -----> +x
*
* \endverbatim
adayton1 marked this conversation as resolved.
Show resolved Hide resolved
*/
template <typename T, int NDIMS>
class Quadrilateral
{
public:
using PointType = Point<T, NDIMS>;
using TriangleType = Triangle<T, NDIMS>;

static constexpr int DIM = NDIMS;
static constexpr int NUM_QUAD_VERTS = 4;

public:
/// \brief Default constructor. Creates a degenerate quadrilateral.
Quadrilateral() = default;
adayton1 marked this conversation as resolved.
Show resolved Hide resolved

/*!
* \brief Custom Constructor. Creates a quadrilateral from the 4 points A, B, C, and D.
* \param [in] A point corresponding to vertex A of the quadrilateral.
* \param [in] B point corresponding to vertex B of the quadrilateral.
* \param [in] C point corresponding to vertex C of the quadrilateral.
* \param [in] D point corresponding to vertex D of the quadrilateral.
*/
AXOM_HOST_DEVICE
Quadrilateral(const PointType& A,
const PointType& B,
const PointType& C,
const PointType& D)
: m_points {A, B, C, D}
{ }

/*!
* \brief Quadrilateral constructor from an initializer list of Points
*
* \param [in] pts an initializer list containing 4 Points
*/
AXOM_HOST_DEVICE
explicit Quadrilateral(std::initializer_list<PointType> pts)
{
SLIC_ASSERT(pts.size() == NUM_QUAD_VERTS);

int i = 0;
for(const auto& pt : pts)
{
m_points[i] = pt;
i++;
}
}

/*!
* \brief Quadrilateral constructor from an Array of Points.
*
* \param [in] pts An ArrayView containing 4 Points.
*/
AXOM_HOST_DEVICE
explicit Quadrilateral(const axom::ArrayView<PointType> pts)
{
SLIC_ASSERT(pts.size() == NUM_QUAD_VERTS);

for(int i = 0; i < NUM_QUAD_VERTS; i++)
{
m_points[i] = pts[i];
}
}

/*!
* \brief Index operator to get the i^th vertex
* \param idx The index of the desired vertex
* \pre idx is 0, 1, 2, or 3
*/
AXOM_HOST_DEVICE
PointType& operator[](int idx)
{
SLIC_ASSERT(idx >= 0 && idx < NUM_QUAD_VERTS);
return m_points[idx];
}

/*!
* \brief Index operator to get the i^th vertex
* \param idx The index of the desired vertex
* \pre idx is 0, 1, 2, or 3
*/
AXOM_HOST_DEVICE
const PointType& operator[](int idx) const
{
SLIC_ASSERT(idx >= 0 && idx < NUM_QUAD_VERTS);
return m_points[idx];
}

/// \brief Returns the area of the quadrilateral (2D specialization)
template <int TDIM = NDIMS>
AXOM_HOST_DEVICE typename std::enable_if<TDIM == 2, double>::type area() const
{
return axom::utilities::abs(signedArea());
}

/**
* \brief Returns the signed area of a 2D quadrilateral
*
* The area is positive when the vertices are oriented counter-clockwise.
* \note This function is only available for quadrilaterals in 2D since
* signed areas don't make sense for 3D quadrilaterals
*
* Compute the signed area by dividing the quadrilateral into two triangles
* as shown below and summing their signed area
*
* \verbatim
*
* D +----+ C +y
* | /|
* | / | ^
* | / | |
* |/ | |
* A +----+ B -----> +x
*
* \endverbatim
*/
template <int TDIM = NDIMS>
AXOM_HOST_DEVICE typename std::enable_if<TDIM == 2, double>::type signedArea() const
{
// TODO: Investigate other algorithms for computing the area
// https://artofproblemsolving.com/wiki/index.php/Shoelace_Theorem
const PointType& A = m_points[0];
const PointType& B = m_points[1];
const PointType& C = m_points[2];
const PointType& D = m_points[3];

// clang-format off
return TriangleType{A, B, C}.signedArea() +
TriangleType{C, D, A}.signedArea();
adayton1 marked this conversation as resolved.
Show resolved Hide resolved
// clang-format on
}

/*!
* \brief Returns the volume of the quadrilateral (synonym for area())
* \sa area()
*/
AXOM_HOST_DEVICE double volume() const { return area(); }

/*!
* \brief Returns the signed volume of a 2D quadrilateral (synonym for signedArea())
* \sa signedArea()
*/
template <int TDIM = NDIMS>
AXOM_HOST_DEVICE typename std::enable_if<TDIM == 2, double>::type signedVolume() const
{
return signedArea();
}

/*!
* \brief Simple formatted print of a quadrilateral instance
* \param os The output stream to write to
* \return A reference to the modified ostream
*/
std::ostream& print(std::ostream& os) const
{
os << "{" << m_points[0] << " " << m_points[1] << " " << m_points[2] << " "
<< m_points[3] << "}";

return os;
}

private:
PointType m_points[NUM_QUAD_VERTS] {PointType {},
PointType {},
PointType {},
PointType {}};
};

//------------------------------------------------------------------------------
/// Free functions implementing Quadrilateral's operators
//------------------------------------------------------------------------------
template <typename T, int NDIMS>
std::ostream& operator<<(std::ostream& os, const Quadrilateral<T, NDIMS>& quad)
{
quad.print(os);
return os;
}

} // namespace primal
} // namespace axom

/// Overload to format a primal::Quadrilateral using fmt
template <typename T, int NDIMS>
struct axom::fmt::formatter<axom::primal::Quadrilateral<T, NDIMS>>
: ostream_formatter
{ };

#endif // AXOM_PRIMAL_QUADRILATERAL_HPP_
Loading