Adds a Quadrilateral primitive #1181

adayton1 · 2023-09-08T17:10:11Z

Summary

This PR is a feature
It does the following:
- Adds Quadrilateral primitive
- Adds BoundingBox constructor that takes an initializer list
- Adds compute_bounding_box operator for Quadrilateral

adayton1 · 2023-09-12T00:34:49Z

Is there a command I can use to do automatic formatting since the style check is failing?

kennyweiss · 2023-09-12T00:53:15Z

Is there a command I can use to do automatic formatting since the style check is failing?

Thanks @adayton1 -- make style from the build directory should do the trick.

adayton1 · 2023-09-12T16:48:23Z

Can someone tell me why the ci/gitlab/lc.llnl.gov pipeline failed? I don't have access to view that page.

bmhan12 · 2023-09-12T16:53:51Z

Can someone tell me why the ci/gitlab/lc.llnl.gov pipeline failed? I don't have access to view that page.

This was the error for jobs with CUDA enabled:
/usr/WS1/atk/gitlab-runner/builds/zkQzXxmM/002/gitlab/axom/axom/src/axom/spin/tests/spin_bvh.cpp(1868): error: copy-list-initialization cannot use a constructor marked "explicit"

Error message in HIP enabled jobs:

/usr/WS1/atk/gitlab-runner/builds/9YsVm9VV/001/gitlab/axom/axom/src/axom/spin/tests/spin_bvh.cpp:1868:20: error: chosen constructor is explicit in copy-initialization
      boxes[idx] = {PointType(0.), PointType(1.)};
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/WS1/atk/gitlab-runner/builds/9YsVm9VV/001/gitlab/axom/axom/src/axom/core/../../axom/primal/geometry/BoundingBox.hpp:101:12: note: explicit constructor declared here
  explicit BoundingBox(std::initializer_list<PointType> pts);
           ^
/usr/WS1/atk/gitlab-runner/builds/9YsVm9VV/001/gitlab/axom/axom/src/axom/core/../../axom/primal/geometry/BoundingBox.hpp:67:7: note: passing argument to parameter here
class BoundingBox
      ^

kennyweiss · 2023-09-12T16:58:13Z

Can someone tell me why the ci/gitlab/lc.llnl.gov pipeline failed? I don't have access to view that page.

Looks like with your new initializer list constructor, it needs:

  // single bounding box in [0,1] x [0,1] x [0,1]
  BoxType* boxes = axom::allocate<BoxType>(1, allocID);
  axom::for_all<exec>(
    0,
    1,
    AXOM_LAMBDA(axom::IndexType idx) {
-      boxes[idx] = {PointType(0.), PointType(1.)};
+      boxes[idx] = BoxType{PointType(0.), PointType(1.)};
    });

kennyweiss

Thanks for this contribution @adayton1

Overall, it looks great, but it would be nice to have a few more unit tests for your expected use case of intersecting 2D and 3D bounding boxes.

It would also be nice to have a unit test for a 3D quadrilateral, and its associated area, but that could wait until we have a direct need.

src/axom/primal/geometry/BoundingBox.hpp

src/axom/primal/operators/compute_bounding_box.hpp

src/axom/primal/tests/primal_compute_bounding_box.cpp

src/axom/primal/geometry/Quadrilateral.hpp

src/axom/primal/tests/primal_quadrilateral.cpp

cyrush · 2023-09-12T17:42:11Z

@kennyweiss can we get @adayton1 access to CZ Gitlab?

kennyweiss · 2023-09-12T18:03:15Z

@kennyweiss can we get @adayton1 access to CZ Gitlab?

Good call! Done.

src/axom/primal/geometry/Quadrilateral.hpp

adayton1 · 2023-09-12T19:46:32Z

Can someone tell me why the ci/gitlab/lc.llnl.gov pipeline failed? I don't have access to view that page.

Looks like with your new initializer list constructor, it needs:
  // single bounding box in [0,1] x [0,1] x [0,1]
  BoxType* boxes = axom::allocate<BoxType>(1, allocID);
  axom::for_all<exec>(
    0,
    1,
    AXOM_LAMBDA(axom::IndexType idx) {
-      boxes[idx] = {PointType(0.), PointType(1.)};
+      boxes[idx] = BoxType{PointType(0.), PointType(1.)};
    });

Or should I make the initializer list constructor implicit?

adayton1 · 2023-09-12T19:55:29Z

Thanks for this contribution @adayton1

Overall, it looks great, but it would be nice to have a few more unit tests for your expected use case of intersecting 2D and 3D bounding boxes.

It would also be nice to have a unit test for a 3D quadrilateral, and its associated area, but that could wait until we have a direct need.

I will add a unit test for intersecting 2D and 3D bounding boxes. I have not implemented the area calculation for a 3D quadrilateral since I haven't been able to find a universal algorithm that works with skewed (non-planar) quadrilaterals.

kennyweiss · 2023-09-12T19:59:15Z

I will add a unit test for intersecting 2D and 3D bounding boxes. I have not implemented the area calculation for a 3D quadrilateral since I haven't been able to find a universal algorithm that works with skewed (non-planar) quadrilaterals.

Thanks. I agree let's hold off on the 3D area until we need it.
2D/3D BBox intersections for quads would be useful though since that's needed in your application.

rhornung67

Thank you @adayton1

…uadrilateral

adayton1 · 2023-09-14T18:07:00Z

src/axom/primal/geometry/BoundingBox.hpp

+   * \param [in] pts an initializer list containing points
+   */
+  AXOM_HOST_DEVICE
+  explicit BoundingBox(std::initializer_list<PointType> pts)


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}}

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.

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.

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

Would it be better to have a make_bounding_box free function?

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);

@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??

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.

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{});

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.

adayton1 · 2023-09-28T21:25:12Z

@kennyweiss, @rhornung67 do you have any more comments?

kennyweiss · 2023-09-28T21:53:52Z

@kennyweiss, @rhornung67 do you have any more comments?

Could you please add a line to the RELEASE_NOTES about the new primitive?

Looks good to me once it passes all the CI. I'm not sure why the Azure jobs are failing.

adayton1 · 2023-09-28T22:06:38Z

@kennyweiss, @rhornung67 do you have any more comments?

Could you please add a line to the RELEASE_NOTES about the new primitive?

Looks good to me once it passes all the CI. I'm not sure why the Azure jobs are failing.

I updated the release notes. Yeah, something weird was going on in the CI. Hopefully restarting it will do the trick.

Add Quadrilateral primitive, operators, and tests

246b066

adayton1 mentioned this pull request Sep 8, 2023

Adds a Quadrilateral primitive #1179

Closed

adayton1 requested review from kennyweiss, bmhan12 and rhornung67 September 8, 2023 17:13

adayton1 added 4 commits September 11, 2023 15:10

Merge branch 'develop' into feature/dayton8/primal_quadrilateral

1c4d0e8

Use right hand coordinate system for quadrilateral

1d1ed9b

Fix some documentation

f941938

Fix build issues

e72cfdf

Clang format fixes

26d8f58

kennyweiss approved these changes Sep 12, 2023

View reviewed changes

kennyweiss reviewed Sep 12, 2023

View reviewed changes

src/axom/primal/geometry/Quadrilateral.hpp Show resolved Hide resolved

kennyweiss added the Primal Issues related to Axom's 'primal component label Sep 12, 2023

Update doxygen for BoundingBox::intersectsWith

620a9c0

rhornung67 approved these changes Sep 12, 2023

View reviewed changes

adayton1 added 5 commits September 12, 2023 14:47

Add test cases

3a07c13

Undo changes to BoundingBox

29063e7

Remove constructor taking raw array

a1e130a

Simplify initialization

8e64ec4

Simplify compute_bounding_box operators

27b1bc7

adayton1 and others added 12 commits September 12, 2023 15:53

Add compute_bounding_box test for 3D quad

7e6bcd4

Add tests for quadrilateral constructors

b36e7e9

Use correct gtest macro

dda0249

Formatting

e517fa0

Fix up default constructor

50aab4e

Formatting

2ece781

Add note about investigating other algorithms for computing area of q…

40e7e55

…uadrilateral

Fix warnings and build errors

7fcf84e

Fix syntax errors

a8519b1

Use explicit constructor

aa6c669

Use member initializer and compiler generated default constructor

170a3c5

Merge branch 'develop' into feature/dayton8/primal_quadrilateral

d70bd56

adayton1 commented Sep 14, 2023

View reviewed changes

Merge branch 'develop' into feature/dayton8/primal_quadrilateral

2babdfa

Updates the release notes

928f7bb

adayton1 merged commit cf01b04 into develop Sep 28, 2023
12 checks passed

adayton1 deleted the feature/dayton8/primal_quadrilateral branch September 28, 2023 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a Quadrilateral primitive #1181

Adds a Quadrilateral primitive #1181

adayton1 commented Sep 8, 2023 •

edited

Loading

adayton1 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

adayton1 commented Sep 12, 2023

bmhan12 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

kennyweiss left a comment

cyrush commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

adayton1 commented Sep 12, 2023

adayton1 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023 •

edited

Loading

rhornung67 left a comment

adayton1 Sep 14, 2023

rhornung67 Sep 14, 2023

rhornung67 Sep 14, 2023 •

edited

Loading

adayton1 Sep 14, 2023 •

edited

Loading

adayton1 Sep 14, 2023

kennyweiss Sep 14, 2023

rhornung67 Sep 14, 2023

adayton1 Sep 15, 2023

kennyweiss Sep 15, 2023

rhornung67 Sep 15, 2023

adayton1 commented Sep 28, 2023

kennyweiss commented Sep 28, 2023

adayton1 commented Sep 28, 2023

Adds a Quadrilateral primitive #1181

Adds a Quadrilateral primitive #1181

Conversation

adayton1 commented Sep 8, 2023 • edited Loading

Summary

adayton1 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

adayton1 commented Sep 12, 2023

bmhan12 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

kennyweiss left a comment

Choose a reason for hiding this comment

cyrush commented Sep 12, 2023

kennyweiss commented Sep 12, 2023

adayton1 commented Sep 12, 2023

adayton1 commented Sep 12, 2023

kennyweiss commented Sep 12, 2023 • edited Loading

rhornung67 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhornung67 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

adayton1 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adayton1 commented Sep 28, 2023

kennyweiss commented Sep 28, 2023

adayton1 commented Sep 28, 2023

adayton1 commented Sep 8, 2023 •

edited

Loading

kennyweiss commented Sep 12, 2023 •

edited

Loading

rhornung67 Sep 14, 2023 •

edited

Loading

adayton1 Sep 14, 2023 •

edited

Loading