Add GenerateBVHSubModel variants

[nicovanduijn]

Identical to the GenerateBVHModel, but without ending the model, such that
more geometric primitives can be added. This is useful if you want to
create a BVHModel containing multiple geometric primitives.

---

This change is 

[sherm1]

This looks useful. I took a look and made some comments. However, it would be good for someone else to look at this to verify there are no unexpected consequences.

Currently all changes to FCL should be made very cautiously because there are very few unit tests ensuring that the functionality is as-specified. It's very important to include unit tests with all new code so that we don't compound this problem.

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed, 4 unresolved discussions (waiting on @nicovanduijn)

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 58 at r1 (raw file):

/// @brief Generate BVH model from box
template<typename BV>
void generateBVHModel(BVHModel<BV>& model, const Box<typename BV::S>& shape, const Transform3<typename BV::S>& pose, bool submodel = false);

The meaning of the name submodel seems unclear to me. Consider calling this something that captures its functionality better, like dont_end_model_yet or more_shapes_to_come or something like that.

Also, the new parameter needs to be documented in the Doxygen docs for each API method (or you could put them in a Doxygen group and provide group documentation).

---

test/test_fcl_broadphase_distance.cpp, line 266 at r1 (raw file):

    Sphere<S> sphere(single_size / 2);
    BVHModel<OBBRSS<S>>* model = new BVHModel<OBBRSS<S>>();
    generateBVHModel(*model, sphere, Transform3<S>::Identity(), 16, 16, false);

Since the original behavior is the default you don't need to add these repetitive false parameters to all these test. You can just leave them as they were before so the changes don't need to be reviewed and so this PR has fewer changes.

On the other hand, I didn't see a unit test of the new functionality anywhere. Please add tests that show proper functioning of the new code that will ensure the code continues to function as you intended when other programmers make changes later, perhaps not understanding your intent.

---

test/test_fcl_shape_mesh_consistency.cpp, line 67 at r1 (raw file):

  generateBVHModel(s1_rss, s1, Transform3<S>::Identity(), 16, 16, false);
  generateBVHModel(s2_rss, s2, Transform3<S>::Identity(), 16, 16, false);

Ditto -- no need for these false arguments, but a definite need for some true arguments!

---

test/test_fcl_utility.h, line 597 at r1 (raw file):

  {
    BVHModel<OBBRSS<S>>* model = new BVHModel<OBBRSS<S>>();
    generateBVHModel(*model, sphere, Transform3<S>::Identity(), 16, 16, false);

Ditto: no need for false here. Do you need a true test here as well?

---

Comments from Reviewable

[nicovanduijn]

Thanks for the review @sherm1

Regarding your comments:

• Indeed my goal was to break as little pre-existing code as possible, thus I made the boolean flag optional. However, the calls

    generateBVHModel(*model, sphere, Transform3<S>::Identity(), 16, 16, false);

Definitely needed the "false" flag because otherwise the compiler doesn't know whether to cast the second "16" to boolean and call the overload with num_faces instead. This might be solved by replacing 16 with 16U everywhere, but might not be the safest either.

The best I could come up with that is safe, breaks little existing code and avoids lots of code duplication was introducing an enum class to replace the boolean.

I added comments and some unit tests. I'm not very experienced in writing unit tests, so let me know if this is appropriate.

Any reason the tests test_fcl_capsule_box_2 fails on my machine, while it works on CI? And the other way around for one of the other unit tests which I never touched either.

[sherm1]

Your unit tests look great, thanks! I'm not sure about the test failure on your machine, but the CI failure is one we know about and @SeanCurtis-TRI is struggling to fix -- it only occurs on the particular Mac setup in CI; no one has been able to duplicate it locally :( .

Nico, some review tips:

• If you click on the purple Reviewable button in GitHub for your PR, you will be in the very nice Reviewable UI where you can respond directly to the reviewers comments.
• The response field recognizes magic keywords like "Done" that may clear away reviewer comments (depending on the keywords the reviewer used to indicate how strict they want to be on a particular point).
• In any case you should respond to each comment individually so that the reviewer knows you've seen them and considered the reviewer's comment (you might reject it, fix it, counterpropose or whatever).
• The "checks" circle at the top of the Reviewable page can tell you what is pending, how many comments are unresolved, who you are waiting for an "lgtm" from, etc.

Sean, do you want to give this a final review? It is short and to the point.
+@SeanCurtis-TRI

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nicovanduijn)

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 56 at r2 (raw file):

{
/**
* \defgroup generateBVHModel

Nit: use @defgroup (rather than backslash) for consistency with the other doxygen commands here.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 61 at r2 (raw file):

* @param[in]   pose a const reference to the pose of the geometric object
* @param[in]   finalize_model a FinalizeModel enum indicating whether the model is final or more submodels can be added later
* @{

This is a nice way to document these! I generated the doxygen output and it looks good. A few minor comments:

• I like the idea of documenting the shared parameters just once. But it leaves the signature-specific parameters undocumented. I think you could add just the specific ones to each signature.
• You do not need the * at the beginnings of these lines. The lines would be shorter and easier to read without them. Every modern code viewer colorizes comments anyway so the initial punctuation doesn't add anything. Consider just /** at the beginning and */ at the end with the rest free-form.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 75 at r2 (raw file):

/// @brief Generate BVH model from box
template<typename BV>
void generateBVHModel(BVHModel<BV>& model, const Box<typename BV::S>& shape, const Transform3<typename BV::S>& pose, FinalizeModel finalize_model = FinalizeModel::DO_FINALIZE);

Nice! I like that solution -- very clear.

[sherm1]

BTW I approved this by typing :lgtm: above, but that's pending a few minor comments below which you can clear yourself by saying "Done" after they are addressed.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nicovanduijn and @SeanCurtis-TRI)

[nicovanduijn]

Thanks for the pointers with reviewable. Still getting used to it

Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @sherm1 and @SeanCurtis-TRI)

[SeanCurtis-TRI]

Hi Nico. Thanks for contributing and being so understanding in our somewhat pedantic ways. We definitely want to push FCL into a more mature state and some of the comments provided have that end goal in mind.

If I prepend a comment with "BTW" or "FYI" it is meant to be informational. It's a suggestion that isn't strictly required, but I wanted to plant a thought in your mind and if you agree, you can modify the code. If I prepend "nit" or "minor", it's a requested change that is a minor, simple change that I feel should definitely happen but, most likely, does not require discussion. All other comments are either larger in impact or might require discussion.

Let me know if you have any questions or my comments are unclear. And, again, thanks for your willingness to contribute.

Reviewed 1 of 7 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nicovanduijn)

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 56 at r3 (raw file):

{
/**
@defgroup generateBVHModel

nit: Probably worth adding a description of the function (e.g., @brief) -- something more than just the well-documented parameters.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 64 at r3 (raw file):

*/

/**

nit You've injected this enum between the subsequent function and its doxygen documentation above.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 69 at r3 (raw file):

*/
enum class FinalizeModel{
	DO_FINALIZE,

BTW You might consider shortening these to DO and DONT. Because you've made it a scoped enumeration, you will always have the prefix FinalizeModel:: so, in that case, finishing with FINALIZE as well might seem redundant. Whereas:

FinalizeModel::Do;
FinalizeModel::Dont;

look reasonable to me.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 79 at r3 (raw file):

/**
@brief Generate BVH model from sphere
@param[in]   seg an unsigned integer defining the number of segments along longitude

BTW A couple of thoughts about doxygen pracitcies:

1. The old comment with a single @brief comment can be sufficient. If a single sentence is enough to communicate the semantics of the inputs and return value then it isn't strictly necessary to enumerate the parameters.
2. Generally you don't have to state the type of the parameter -- the declaration and doxygen rendering will make that clear. Your comments can focus on understanding the interpretation of the parameter leaving the compiler and doxygen to cover the simple syntax details.

---

include/fcl/geometry/geometric_shape_to_BVH_model-inl.h, line 84 at r3 (raw file):

  }

  if(model.build_state == BVH_BUILD_STATE_EMPTY){

This new pattern happens five times now. That is sufficient repetition that it's worth refactoring into its own method. Something like:

void AddTriangles(BVHModel<BV>& model,
                  const std::vector<Vector3<S>>& points,
                  const std::vector<Triangle> tri_indices,
                  FinalizeModel finalize_model) {
  // Copy these nine lines here.
}

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

#include <iostream>

using namespace fcl;

Several thoughts on this test.

1. You've added for loops to create variations of geometries (e.g., different box dimensions). I would suggest that that is unnecessary work. These test are focused on the ability to update a non-finalized model. As such, it's enough to add one primitive, and then to add another. One would assume that the dimensions of the geometry don't play a role in that.

2. Each of these tests calls the same validation tests (# vertices, # triangles, and model state). You could/should put those tests into a refactored test where each test location merely passes those three expected values into an invocation.

3. You should document clearly what is being tested -- that you're testing the ability to add a model to a non-finalized BVH. You are not testing that the BVH is correct or efficient or anything else. In fact, it might be best if the name of this file reflected that.

4. You should also test what happens if someone attempts to add to the model that has already been finalized -- the goal would be to fully characterize the semantics of the function and we would expect failure and should confirm that.

I'll let you think about these and make modifications before I look at the rest of the test in detail.

---

test/test_fcl_generate_bvh_model.cpp, line 52 at r3 (raw file):

  // Test various box sizes
  for(S a = 0.5; a <= 2.1; a += 0.8){

BTW This might be more readable as:

for (S a : {0.5, 1.3, 2.1}) {
...
}

You're obviously targeting specific (albeit, arbitrary) values. The range iterator over an initializer list makes that abundantly clear and makes it easy to edit later if it turns out there are other magical values worth trying.

[nicovanduijn]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nicovanduijn)

---

include/fcl/geometry/geometric_shape_to_BVH_model-inl.h, line 84 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This new pattern happens five times now. That is sufficient repetition that it's worth refactoring into its own method. Something like:

void AddTriangles(BVHModel<BV>& model,
                  const std::vector<Vector3<S>>& points,
                  const std::vector<Triangle> tri_indices,
                  FinalizeModel finalize_model) {
  // Copy these nine lines here.
}

Fair enough. It is quite a thin wrapper around the model's method "addSubModel(points, tri_indices)", but depending on your preferences just enough to warrant a new method

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Several thoughts on this test.

1. You've added for loops to create variations of geometries (e.g., different box dimensions). I would suggest that that is unnecessary work. These test are focused on the ability to update a non-finalized model. As such, it's enough to add one primitive, and then to add another. One would assume that the dimensions of the geometry don't play a role in that.

2. Each of these tests calls the same validation tests (# vertices, # triangles, and model state). You could/should put those tests into a refactored test where each test location merely passes those three expected values into an invocation.

3. You should document clearly what is being tested -- that you're testing the ability to add a model to a non-finalized BVH. You are not testing that the BVH is correct or efficient or anything else. In fact, it might be best if the name of this file reflected that.

4. You should also test what happens if someone attempts to add to the model that has already been finalized -- the goal would be to fully characterize the semantics of the function and we would expect failure and should confirm that.

I'll let you think about these and make modifications before I look at the rest of the test in detail.

Generally, I agree. If you look at my commits, you might see how this evolved into what it is right now, but briefly explained this is how it came to be:

Originally started just testing unit-sized-everythings. But once I realized that those are actually all corner cases that I'm testing, I wanted to be a bit more general. E.g. an ellipsoid with radii (1.0, 1.0, 1.0) is really just a sphere.

Then I started thinking what other corner cases might arrive. Maybe adding another model would fail if vertices coincide? I don't know. So I added loops to test a whole bunch of sizes. Not the best way to test for these cases either, I know.

This quickly blows up, especially for ellipsoids since there are so many variables. So I reduced the for loops to test just a few values each. You are of course entirely correct in calling them arbitrary choices.

I like your suggestion of the range based loops, as it still tests various sizes and gives a future programmer the option to easily insert a corner case he may have found.

To conclude:

1. I kind-of disagree. I would like to test various sizes to make the test more robust and customizable
2. Agree. Will refactor
3. Agree. Will do, except the file name. I think it makes sense for it to be called like the very functions it is testing, no? I see that it does not test proper functionality of the generation of the model from primitives, but that should be added by the author of this logic, as I'm not familiar enough with it.
4. Agree. Might take more work to do, but I'll give it a shot. Might need to add error handling of some sort

---

test/test_fcl_generate_bvh_model.cpp, line 52 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This might be more readable as:

for (S a : {0.5, 1.3, 2.1}) {
...
}

You're obviously targeting specific (albeit, arbitrary) values. The range iterator over an initializer list makes that abundantly clear and makes it easy to edit later if it turns out there are other magical values worth trying.

done

[nicovanduijn]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI)

---

include/fcl/geometry/geometric_shape_to_BVH_model-inl.h, line 84 at r3 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Fair enough. It is quite a thin wrapper around the model's method "addSubModel(points, tri_indices)", but depending on your preferences just enough to warrant a new method

Done.

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Generally, I agree. If you look at my commits, you might see how this evolved into what it is right now, but briefly explained this is how it came to be:

Originally started just testing unit-sized-everythings. But once I realized that those are actually all corner cases that I'm testing, I wanted to be a bit more general. E.g. an ellipsoid with radii (1.0, 1.0, 1.0) is really just a sphere.

Then I started thinking what other corner cases might arrive. Maybe adding another model would fail if vertices coincide? I don't know. So I added loops to test a whole bunch of sizes. Not the best way to test for these cases either, I know.

This quickly blows up, especially for ellipsoids since there are so many variables. So I reduced the for loops to test just a few values each. You are of course entirely correct in calling them arbitrary choices.

I like your suggestion of the range based loops, as it still tests various sizes and gives a future programmer the option to easily insert a corner case he may have found.

To conclude:

1. I kind-of disagree. I would like to test various sizes to make the test more robust and customizable
2. Agree. Will refactor
3. Agree. Will do, except the file name. I think it makes sense for it to be called like the very functions it is testing, no? I see that it does not test proper functionality of the generation of the model from primitives, but that should be added by the author of this logic, as I'm not familiar enough with it.
4. Agree. Might take more work to do, but I'll give it a shot. Might need to add error handling of some sort

Done.

[SeanCurtis-TRI]

Second pass done

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @SeanCurtis-TRI and @nicovanduijn)

a discussion (no related file):
One big question on this PR -- the BVH has no ability to distinguish which triangle belongs to which geometry. All triangles are considered to be one homogeneous soup. Is this the intent? In the original state (with one geometry per model), the geometry it came from was implicit. Did you have any expectation that you could know which geometry any particular triangle came from?

If the former, that should be clearly documented in the @defgroup documentation -- that leaving the model unfinalized allows the definition of a model that is the union of all the shapes -- but the original shapes cannot be recovered. If the latter, than clearly, we have a bug.

---

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 71 at r4 (raw file):
This doxygen seem s a little counter-intuitive. Some doxygen flags clearly belong to the @group (e.g., @defgroup, @brief, and @details). However, the @param list (at first) doesn't seem to belong. Perhaps it would be good to more explicitly tie them together. Something along the lines of:

All functions in this group have a common sub-set of parameters (listed below). In addition, each has unique parameters related to the geometry type being added and how it should be triangulated. These additional parameters are documented with their corresponding functions.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 88 at r4 (raw file):

**/
template<typename BV>
int generateBVHModel(BVHModel<BV>& model, const Sphere<typename BV::S>& shape, const Transform3<typename BV::S>& pose, unsigned int seg, unsigned int ring, FinalizeModel finalize_model = FinalizeModel::DO);

You've added a return value; its interpretation should be documented. Probably above in the @group definition.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 153 at r4 (raw file):

/**
@brief          AddTriangles to a BVHModel

BTW This didn't necessarily need to be defined in the header file. You could (maybe should) make it a purely local helper function in the .cc file. Also, as is, it is being included in the generateBVHModel doxygen group. So, moving it into the .cc file addresses that as well.

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Done.

Just to continue the conversation. There are a couple of points here. Test focus and targeting.

Focus: The more focused a test is, the better. Its passage/failure should indicate something very specific. This PR is about adding the FinalizeModel parameter. The tests in this PR should support that very specific feature: does it leave the door open with FinalizeModel::DONT and then does it successfully close the door with FinalizeModel::DO? There should be tests that explicitly test that very simple concept -- they can assume friendly conditions (avoiding all the corner cases).

Now, the point that I believe you're also making is that the generateBVHModel algorithms were designed as a one-shot thing; one BVH generated for one geometry. It is possible that they are making assumptions which would be invalidated for multiple geometries. The key phrase that jumps out to me is "Maybe adding another model would fail if vertices coincide? I don't know." This "I don't know" is the problem I have. If your suspicions are correct, then there should be tests that actively exercise that corner. And they should be different tests than the ones that make sure the FinalizeModel switch works at all. Even if your suspicion is correct and such a bug is lurking in the code, I don't believe that these particular values necessarily expose it.

Targeting: Currently, I suspect (from what you've written), that you don't actually know that the extra complexity in the tests contributes anything. However, if we merge the complexity into master, future developers will assume they are significant. And that's something we want to avoid.

Perhaps an appropriate intermediate solution is to simplify these tests into just making sure the switch works and then add a TODO to investigate other failure conditions (documenting that these tests only confirm the viability of the switch but haven't determined if there are potential conflicts between subsequent geometries.)

FTR I've delved a bit more into the code and I don't believe that the variations in geometry actually uniquely exercise any interesting code in this regard. Ultimately, the BVH just operates on a triangle soup and has no awareness of redundant vertices or triangles.

Miscellaneous:
File name: the file name should reflect the code structure it is testing (as you say). However, you're not testing the whole function (for example, you're not testing any of the tesselation functionality that is part of the generateBVHModel() functions). By giving this test the most universal name, it suggests that you're fully testing the file. So, what you could do is add a suffix to the file name suggesting you're not testing everything (e.g., test_fcl_generate_bvh_model_deferred_finalize.cc. Alternatively, you put a big note at the top for future developers so that they can know what is and what isn't covered in this test.

---

test/test_fcl_generate_bvh_model.cpp, line 49 at r4 (raw file):
Add

As a side effect, the provided model will always be finalized.

---

test/test_fcl_generate_bvh_model.cpp, line 63 at r4 (raw file):

  // Add another instance of the shape and make sure it was added to the model by counting vertices and tris
  generateBVHModel(model, shape, Transform3<S>(Translation3<S>(Vector3<S>(2.0, 2.0, 2.0))), n);

I'd advocate explicitly passing FinalizeModel::DO. This guarantees the test will still pass even if the API changes (i.e., such as no longer making this parameter optional or changing the default value.) Same applies to all invocations in this test file.

---

test/test_fcl_generate_bvh_model.cpp, line 91 at r4 (raw file):

**/
template<typename BV, typename ShapeType>
void checkAddToFinishedModel(BVHModel<BV>& model, const ShapeType& shape, uint8_t n)

FYI Excellent! These tests are exactly what I would hope for (and +1 for checking your pre-condition).

---

test/test_fcl_generate_bvh_model.cpp, line 94 at r4 (raw file):

{
  using S = typename BV::S;
  EXPECT_EQ(model.build_state, BVH_BUILD_STATE_PROCESSED);

You should make this GTEST_ASSERT_EQ -- after all, if this test fails, there's no point moving on. (Same for the Box version.)

---

test/test_fcl_generate_bvh_model.cpp, line 122 at r4 (raw file):

        checkNumVerticesAndTris(*model, box, 8, 12);
        checkAddToFinishedModel(*model, box);

Could you add a comment on this line (and all of the subsequent invocations) that indicates that this invocation relies on checkNumVerticesAndTris() finalizing the model? It'll make reading these tests easier for future developers.

---

test/test_fcl_generate_bvh_model.cpp, line 228 at r4 (raw file):

}

template<typename BV>

FYI While this makes for a very compact expression of the tests, it leads to single, gargantuan test. If, for any shape or BV type there is a failure, the whole test fails. I'm not going to push for a refactoring or parameterization of the gtest. I just wanted to point that out to you.

[nicovanduijn]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @SeanCurtis-TRI and @nicovanduijn)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

One big question on this PR -- the BVH has no ability to distinguish which triangle belongs to which geometry. All triangles are considered to be one homogeneous soup. Is this the intent? In the original state (with one geometry per model), the geometry it came from was implicit. Did you have any expectation that you could know which geometry any particular triangle came from?

If the former, that should be clearly documented in the @defgroup documentation -- that leaving the model unfinalized allows the definition of a model that is the union of all the shapes -- but the original shapes cannot be recovered. If the latter, than clearly, we have a bug.

Good to point this out. Personally I am aware that once it's a BVHModel, there's no way of "going back", e.g. to find out which primitive a certain vertex used to belong to. To me, that's not a problem, I almost think of it as a "convert to mesh" type of operation on a CAD program that cannot be undone. I'll add a @warning to the @defgroup

---

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 71 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This doxygen seem s a little counter-intuitive. Some doxygen flags clearly belong to the @group (e.g., @defgroup, @brief, and @details). However, the @param list (at first) doesn't seem to belong. Perhaps it would be good to more explicitly tie them together. Something along the lines of:

All functions in this group have a common sub-set of parameters (listed below). In addition, each has unique parameters related to the geometry type being added and how it should be triangulated. These additional parameters are documented with their corresponding functions.

Done.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 88 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You've added a return value; its interpretation should be documented. Probably above in the @group definition.

Done.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 153 at r4 (raw file):

ely local helper function in the .cc file. Also, as i
I agree it should not be in the doxygen group.
But I'm not sure which .cc file you're talking about. Did you mean the "geometric_shape_to_BVH_model-inl.h"?
I find the function could be generally useful, as an alternative to/wrapper around the BVHModel method "addSubModel", so I put it in here. Now that I think about it, maybe that's not the best, I should either make it a local like you suggested or add a corresponding method to the BVHModel class.

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Just to continue the conversation. There are a couple of points here. Test focus and targeting.

Focus: The more focused a test is, the better. Its passage/failure should indicate something very specific. This PR is about adding the FinalizeModel parameter. The tests in this PR should support that very specific feature: does it leave the door open with FinalizeModel::DONT and then does it successfully close the door with FinalizeModel::DO? There should be tests that explicitly test that very simple concept -- they can assume friendly conditions (avoiding all the corner cases).

Now, the point that I believe you're also making is that the generateBVHModel algorithms were designed as a one-shot thing; one BVH generated for one geometry. It is possible that they are making assumptions which would be invalidated for multiple geometries. The key phrase that jumps out to me is "Maybe adding another model would fail if vertices coincide? I don't know." This "I don't know" is the problem I have. If your suspicions are correct, then there should be tests that actively exercise that corner. And they should be different tests than the ones that make sure the FinalizeModel switch works at all. Even if your suspicion is correct and such a bug is lurking in the code, I don't believe that these particular values necessarily expose it.

Targeting: Currently, I suspect (from what you've written), that you don't actually know that the extra complexity in the tests contributes anything. However, if we merge the complexity into master, future developers will assume they are significant. And that's something we want to avoid.

Perhaps an appropriate intermediate solution is to simplify these tests into just making sure the switch works and then add a TODO to investigate other failure conditions (documenting that these tests only confirm the viability of the switch but haven't determined if there are potential conflicts between subsequent geometries.)

FTR I've delved a bit more into the code and I don't believe that the variations in geometry actually uniquely exercise any interesting code in this regard. Ultimately, the BVH just operates on a triangle soup and has no awareness of redundant vertices or triangles.

Miscellaneous:
File name: the file name should reflect the code structure it is testing (as you say). However, you're not testing the whole function (for example, you're not testing any of the tesselation functionality that is part of the generateBVHModel() functions). By giving this test the most universal name, it suggests that you're fully testing the file. So, what you could do is add a suffix to the file name suggesting you're not testing everything (e.g., test_fcl_generate_bvh_model_deferred_finalize.cc. Alternatively, you put a big note at the top for future developers so that they can know what is and what isn't covered in this test.

Ok, I see that a "Unit" test is supposed to be only testing one specific thing. And you are right in suspecting that I don't understand how my added complexity in the unit test may or may not test for special cases. This "shot in the dark and hope to maybe catch some bugs" is quite naive and frankly dumb.

However, I am not the right person to write a unit test which properly tests those things, and I don't see a unit test that currently does so. Again, I'm not knowledgeable enough to do so, I think.

I now made two tests:

• One testing only the functionality I personally implemented: test_fcl_generate_bvh_model_deferred_finalize.cpp (It's getting a bit wordy now...)
• One testing the more basic functionality of creating BVHModels from primitives: test_fcl_generate_bvh_model_primitives.cpp. This one is to be considered the "bare bones"
  of what it should one day contain. I indicated this clearly in the comments.

I hope this addresses your concerns

---

test/test_fcl_generate_bvh_model.cpp, line 49 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Add

As a side effect, the provided model will always be finalized.

Done.

---

test/test_fcl_generate_bvh_model.cpp, line 63 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd advocate explicitly passing FinalizeModel::DO. This guarantees the test will still pass even if the API changes (i.e., such as no longer making this parameter optional or changing the default value.) Same applies to all invocations in this test file.

Done.

---

test/test_fcl_generate_bvh_model.cpp, line 94 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You should make this GTEST_ASSERT_EQ -- after all, if this test fails, there's no point moving on. (Same for the Box version.)

Done.

---

test/test_fcl_generate_bvh_model.cpp, line 122 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Could you add a comment on this line (and all of the subsequent invocations) that indicates that this invocation relies on checkNumVerticesAndTris() finalizing the model? It'll make reading these tests easier for future developers.

Done.

[SeanCurtis-TRI]

This looks great. We're almost there (just a last couple of quibbles).

Reviewed 5 of 5 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Good to point this out. Personally I am aware that once it's a BVHModel, there's no way of "going back", e.g. to find out which primitive a certain vertex used to belong to. To me, that's not a problem, I almost think of it as a "convert to mesh" type of operation on a CAD program that cannot be undone. I'll add a @warning to the @defgroup

Excellent.

---

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 153 at r4 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

ely local helper function in the .cc file. Also, as i
I agree it should not be in the doxygen group.
But I'm not sure which .cc file you're talking about. Did you mean the "geometric_shape_to_BVH_model-inl.h"?
I find the function could be generally useful, as an alternative to/wrapper around the BVHModel method "addSubModel", so I put it in here. Now that I think about it, maybe that's not the best, I should either make it a local like you suggested or add a corresponding method to the BVHModel class.

Oops...right, not the ".cc" file, but the '-inl" file. You clearly understood my intent, even when I failed at communicating it.

---

include/fcl/geometry/geometric_shape_to_BVH_model.h, line 80 at r5 (raw file):
FYI It seems odd to document this as a BVHReturnCode but have it declared as an int. But I won't defect this due to the implicit conversion. Perhaps:

Return code (as defined by BVHReturnCode) indicating the success of the operation.

---

test/test_fcl_generate_bvh_model_deferred_finalize.cpp, line 148 at r5 (raw file):

  ret = generateBVHModel(model, shape, Transform3<S>(Translation3<S>(Vector3<S>(2.0, 2.0, 2.0))), FinalizeModel::DONT);
  GTEST_ASSERT_EQ(ret, BVH_OK);
  EXPECT_GT(model.num_vertices, num_vertices);

FYI You could make this test more specific. In the cases of boxes, you know exactly how many more vertices and triangles have been added. These could become EXPECT_EQ making it a stronger test. Same applies to all box specializations.

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Ok, I see that a "Unit" test is supposed to be only testing one specific thing. And you are right in suspecting that I don't understand how my added complexity in the unit test may or may not test for special cases. This "shot in the dark and hope to maybe catch some bugs" is quite naive and frankly dumb.

However, I am not the right person to write a unit test which properly tests those things, and I don't see a unit test that currently does so. Again, I'm not knowledgeable enough to do so, I think.

I now made two tests:

• One testing only the functionality I personally implemented: test_fcl_generate_bvh_model_deferred_finalize.cpp (It's getting a bit wordy now...)
• One testing the more basic functionality of creating BVHModels from primitives: test_fcl_generate_bvh_model_primitives.cpp. This one is to be considered the "bare bones"
  of what it should one day contain. I indicated this clearly in the comments.

I hope this addresses your concerns

This is marvelous. I'm sorry if I implied that you should write those other tests; that was not my intent. You are right that the other tests are missing. I also agree that this is not the time nor place to add them. Someone is really going to have to delve into that code to supply the missing unit tests. That's a task for later.

I would advocate removing this file, however, and capture this as an issue. It's much easier to track as work to be done as an issue than as a comment hiding in some file.

I realize this will mean throwing out some code and documentation you've written. But as we're unsure if these are the right tests for this function, I'd prefer not to give the illusion of such until we know.

[nicovanduijn]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

---

test/test_fcl_generate_bvh_model_deferred_finalize.cpp, line 148 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI You could make this test more specific. In the cases of boxes, you know exactly how many more vertices and triangles have been added. These could become EXPECT_EQ making it a stronger test. Same applies to all box specializations.

Well, true, but then we'd technically be mixing things again... Strictly speaking that would be testing whether the generateBVHModel creates a "correct" box with 8 vertices and 12 triangles. But if it makes you happy I can hard-code this to 8 and 12.

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This is marvelous. I'm sorry if I implied that you should write those other tests; that was not my intent. You are right that the other tests are missing. I also agree that this is not the time nor place to add them. Someone is really going to have to delve into that code to supply the missing unit tests. That's a task for later.

I would advocate removing this file, however, and capture this as an issue. It's much easier to track as work to be done as an issue than as a comment hiding in some file.

I realize this will mean throwing out some code and documentation you've written. But as we're unsure if these are the right tests for this function, I'd prefer not to give the illusion of such until we know.

You know how hard it is to let go of code you've written.. sigh
To ease my pain and frustration I'll put that file on a new branch with WIP in the signature and create an issue. Maybe someone will pick it up from there

[SeanCurtis-TRI]

@sherm1 This is ready for merge; the only CI failure is the known mac-release box-box failure.

Reviewed 4 of 4 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

---

test/test_fcl_generate_bvh_model_deferred_finalize.cpp, line 148 at r5 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

Well, true, but then we'd technically be mixing things again... Strictly speaking that would be testing whether the generateBVHModel creates a "correct" box with 8 vertices and 12 triangles. But if it makes you happy I can hard-code this to 8 and 12.

Thanks

---

test/test_fcl_generate_bvh_model.cpp, line 44 at r3 (raw file):

Previously, nicovanduijn (Nico van Duijn) wrote…

You know how hard it is to let go of code you've written.. sigh
To ease my pain and frustration I'll put that file on a new branch with WIP in the signature and create an issue. Maybe someone will pick it up from there

You have my deepest sympathies and thanks.

[SeanCurtis-TRI]

Nico, thanks for the PR. Sorry to make the experience a bit like pulling teeth and I'm particularly grateful for how willing you were to play along.