[geometry] ComputeSignedDistanceToPoint for meshes #21471

DamrongGuoy · 2024-05-22T20:58:14Z

We need two tasks:

[geometry] Calculate signed distance from point to closed meshes #21566
ComputeSignedDistanceToPoint from meshes in ProximityEngine (draft branch)

This change is

jwnimmer-tri · 2024-05-23T16:51:51Z

I'll also mark it +(status: do not review) since we're not seeking the official code review now, we're just pinging specific people to check the overall idea. This placates the https://drake.mit.edu/platform_reviewer_checklist.html needs-attention report.

jwnimmer-tri

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @DamrongGuoy)

geometry/query_object.h line 747 at r1 (raw file):

       is to the high curvature area, the bigger the effect. It is not
       immediately clear how much worse the answer will get.
   - ᶜ Support only compliant hydroelastic tetrahedral meshes (VTK files).

The details of (c) are unclear in several ways:

(1) When the file does not meet this criterion, what happens? Does it throw, or ignore like note (a)?

(2) For supported meshes, is the distance (and grad) measured to the convex hull of the geometry (like point contact), or does it allow for non-convexity (like a hydro surface)?

(3) It says "compliant hydro tet mesh". Are we trying to exclude rigid hydro tet meshes? I'm not sure why the hydroelastic details should come into play here. My intuition is that "vtk tet mesh" should be the only criterion? Why does the compliance matter for a signed-distance?

DamrongGuoy

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge"

geometry/query_object.h line 747 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The details of (c) are unclear in several ways:

(1) When the file does not meet this criterion, what happens? Does it throw, or ignore like note (a)?

(2) For supported meshes, is the distance (and grad) measured to the convex hull of the geometry (like point contact), or does it allow for non-convexity (like a hydro surface)?

(3) It says "compliant hydro tet mesh". Are we trying to exclude rigid hydro tet meshes? I'm not sure why the hydroelastic details should come into play here. My intuition is that "vtk tet mesh" should be the only criterion? Why does the compliance matter for a signed-distance?

Thanks for asking. I hope it's clearer now.

I agree that it's independent of hydroelastics.

jwnimmer-tri

This seems aimed in the right direction to me.

\CC @SeanCurtis-TRI in case you want to weigh in, or check with me on any details.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @DamrongGuoy)

geometry/query_object.h line 747 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Thanks for asking. I hope it's clearer now.

I agree that it's independent of hydroelastics.

Thanks!

SeanCurtis-TRI

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @DamrongGuoy)

geometry/query_object.h line 747 at r2 (raw file):

       is to the high curvature area, the bigger the effect. It is not
       immediately clear how much worse the answer will get.
   - ᶜ Support only tetrahedral meshes in VTK files. Unsupported meshes are

Grammar

Suggestion:

Only tetrahedral meshes in VTK files are supported.

geometry/query_object.h line 748 at r2 (raw file):

       immediately clear how much worse the answer will get.
   - ᶜ Support only tetrahedral meshes in VTK files. Unsupported meshes are
       simply ignored; no results are reported for that geometry. The signed

BTW Given that the mesh has the potential (possibly even high probability) of being non-convex, it feels like the documentation, overall, should emphasize that in these cases, not only can the gradient be discontinuous, but the witness point can also be discontinuous (a property that has not hitherto been at risk with the previous convex shapes).

DamrongGuoy

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

geometry/query_object.h line 747 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Grammar

Done below.

geometry/query_object.h line 748 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Given that the mesh has the potential (possibly even high probability) of being non-convex, it feels like the documentation, overall, should emphasize that in these cases, not only can the gradient be discontinuous, but the witness point can also be discontinuous (a property that has not hitherto been at risk with the previous convex shapes).

Done.

(Side note: Convex() can also have discontinuous witness points and gradients. For example, the center of a regular polyhedron has multiple witness points and gradients. All Shape subclasses, except HalfSpace, have discontinuous witness points at their centers or medial axes.)

DamrongGuoy

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

a discussion (no related file):
@calderpg-tri could you please confirm this feature is what you'd like? The actual computations are in other PRs that I'm still working on. This PR is only documentation.

calderpg-tri

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @DamrongGuoy)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

@calderpg-tri could you please confirm this feature is what you'd like? The actual computations are in other PRs that I'm still working on. This PR is only documentation.

Yes, signed distance queries against convex hull of Convex shapes and surface of Mesh shapes (with appropriate mesh files) is what I'm looking for.

DamrongGuoy

I moved code from my draft branch into this PR.

This PR fixes #21323.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

DamrongGuoy

@SeanCurtis-TRI I still have status:do not review, and the PR is still a draft. For now, I'd like your opinion on the general design. If you could browse the code by Monday, we can talk in person on Tuesday. Your insight on the design would be greatly appreciated.

Please ignore the unit tests for now.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

SeanCurtis-TRI

You bet. I'll take a look at the core meat of this between now and Tuesday.

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

SeanCurtis-TRI

So, I think this is basically, structurally there. There's some small stuff that would come up in normal review. I apologize if I've tagged anything that is still a WIP but as things occurred to me, I felt I should note them in case I missed them later.

Reviewed 16 of 17 files at r5, all commit messages.
Reviewable status: 36 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

a discussion (no related file):
Something just occurred to me. Is there a reason that CalcSignedDistnaceToSurfaceMesh is double-valued and not T-valued? I.e., in principle, it could take a Vector3<T> p_MQ. We should make it clear why we've excluded T or put in a TODO for what it would take to have it.

geometry/BUILD.bazel line 905 at r5 (raw file):

#  prototype quickly.
drake_cc_googletest(
    name = "proximity_engine_temporary_test",

BTW Wise. :)

geometry/proximity_engine.cc line 396 at r5 (raw file):

        geometry.shape(), id, new_properties, X_WG);

    // TODO(DamrongGuoy): Update mesh_sdf_data_ for new properties.

nit: I'm not clear what that would entail. Surely the mesh_sdf_data_ doesn't depend on any ProximityProperties.

geometry/proximity_engine.cc line 556 at r5 (raw file):

  void ImplementGeometry(const Mesh& mesh, void* user_data) override {
    // Need this for non-ComputeSignedDistanceToPoint queries.

nit: Actually, we even need it for ComputeSignedDistanceToPoint queries as all non-convex meshes are represented by their convex hulls in the BVH. Perhaps the following instead:

// We currently represent Mesh shapes with their convex hulls in fcl.

geometry/proximity_engine.cc line 1052 at r5 (raw file):

                                 mesh.source(), mesh.scale())));
    }
    // Unsupported meshes will be ignored in point_distance::Callback().

BTW Let's make this more precise:

    // Meshes are unsupported if we cannot compute a VolumeMeshBoundary.
    // point_distance::Callback() skips every Mesh that doesn't have an entry
    // in mesh_sdf_data_.

This makes it clear why file types are unsupported and suggests that this is the logic that defines whether a Mesh is supported or not and that point_distance::Callback simply keys on the signal defined here.

geometry/proximity_engine.cc line 1154 at r5 (raw file):

  // geometry could be a tetrahedral mesh (Mesh(.vtk)), a triangle mesh
  // (Mesh(.obj)), or a convex hull (Convex(.obj) and Convex(.vtk)). Their
  // common requirement is to be water-tight.

BTW I'd leave off all of the elaboration. It's documented elsewhere. And just gives us one more site to keep in sync.

Suggestion:

  // Data for ComputeSignedDistanceToPoint from meshes.

geometry/query_object.h line 748 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Done.

(Side note: Convex() can also have discontinuous witness points and gradients. For example, the center of a regular polyhedron has multiple witness points and gradients. All Shape subclasses, except HalfSpace, have discontinuous witness points at their centers or medial axes.)

Touche. I was insufficiently imprecise. I'd been thinking of discontinuities when on the exterior of the geometry. That's what's new.

geometry/query_object.h line 735 at r5 (raw file):

   |   Scalar   |   %Box  | %Capsule | %Convex | %Cylinder | %Ellipsoid | %HalfSpace |  %Mesh  | %Sphere |
   | :--------: | :-----: | :------: | :-----: | :-------: | :--------: | :--------: | :-----: | :-----: |
   |   double   |  2e-15  |   4e-15  |  1e-13ᶜ |   3e-15   |    3e-5ᵇ   |    5e-15   | 1e-13ᵈ  |  4e-15  |

nit: Where does this number come from? It should be supported in the characterization test. And yet, distance_to_point_characterize_test.cc still reports Convex as being ignored.

This also has made the MeshIsConvex() test (in charcterization_utilities.h invalid. And it probably was always incorrect because it doesn't allow for a Mesh having multiple representations based on the query. It was a laudable goal, but, nevertheless it proves to be more wrong than right.

geometry/query_object.h line 749 at r5 (raw file):

       is to the high curvature area, the bigger the effect. It is not
       immediately clear how much worse the answer will get.
   - ᶜ Convex hulls of meshes are supported. The signed distance, the

Nit: this footnote isn't needed at all. Convex is already defined as a convex hull (as specified by a mesh). And a reported value here already implies normal support. No further statement isn't required.

geometry/query_object.h line 751 at r5 (raw file):

   - ᶜ Convex hulls of meshes are supported. The signed distance, the
       nearest point, and the gradient are computed from the convex hull.
   - ᵈ Tetrahedral meshes in VTK files and triangle meshes in OBJ files are

nit: No need to specify "triangle meshes" in OBJ.

geometry/query_object.h line 753 at r5 (raw file):

   - ᵈ Tetrahedral meshes in VTK files and triangle meshes in OBJ files are
       supported. Unsupported meshes are simply ignored; no results are
       reported for that geometry. The signed distance, the nearest point,

BTW I see the language here borrows from the previous version. In its old version, it may have been slightly redundant. But, at the very least, it was trying to make something clear about the relationship between a tetrahedral mesh and the signed distance. With broader support of more mesh types, it's less targeted and feels far more redundant. I'd suggest simply:

   - ᶜ Only supports OBJ and tetrahedral VTK meshes. Unsupported meshes are
       simply ignored; no results are reported for that geometry. For OBJ meshes
       the surface mesh must satisfy specific requirements (see below). Unlike
       the other Shapes, witness points and gradients can be discontinuous on a
       mesh's exterior if it is non-convex.

geometry/query_object.h line 759 at r5 (raw file):

   @pre The %Mesh of a triangular surface mesh must be a closed manifold
   without duplicate vertices or self-intersection, and every triangle's face
   winding gives an outward-pointing face normal. Non-compliant meshes will

BTW This query suffers from the same issues that computation of a mesh's spatial inertia does. It has some requirements on the mesh that aren't validated but necessary for the final result to be valid. I'd propose an extra phrase:

   @pre The %Mesh of a triangular surface mesh must be a closed manifold
   without duplicate vertices or self-intersection, and every triangle's face
   winding gives an outward-pointing face normal. Drake does not currently
   validate the input mesh with respect to these properties. Instead, it does a
   good-faith computation assuming the properties, possibly returning incorrect
   results. Non-compliant meshes will ...

The same applies to tet meshes (see my note below). It probably would be better saying that this query has certain requirements for the meshes, indicate what the requirements are, and then state one time about it being untested and the possible ramifications of the requirements not being met.

geometry/query_object.h line 771 at r5 (raw file):

   (https://en.wikipedia.org/wiki/Simplicial_complex) whose tetrahedra have
   positive volumes (or positive orientation). Therefore, it has no duplicate
   vertices, no self-intersection, and any two tetrahedra intersect in a common

While assigning the name 'simplicial complex' to an acceptable mesh is nice, I don't know how much value it adds. You provide the label and then define it. Reference to terms like that are best if the term replaces the definition. A label and definition only makes sense if you plan on using the label moving forward. That doesn't seem to be the case here.

Instead, we should unify our discussion of volume mesh with surface mesh. We have requirements on the meshes for this algorithm to produce meaningful results. So, let's enumerate the requirements for each kind of mesh and then discuss the possible issues that might/will arise if they aren't met.

Code quote:

no self-intersection

geometry/shape_specification.h line 470 at r5 (raw file):

   - Computing signed distances from the %Mesh to query points (when it
     references a triangle .obj file or a tetrahedral .vtk file).

nit: Don't say "triangle .obj". We automatically triangulate it when we parse .obj file. Although, "tetrahedral .vtk" is, of course, quite appropriate.

geometry/proximity/BUILD.bazel line 274 at r5 (raw file):

    name = "distance_to_point_callback",
    srcs = [
        "calc_signed_distance_to_surface_mesh.cc",

BTW It's not clear to me why the calc_signed_distance_to_surface_mesh build target got rolled into here. Can you explain why? (This is a request for a change to the PR, just an explanation in this discussion of what I'm missing.)

geometry/proximity/calc_signed_distance_to_surface_mesh.h line 53 at r5 (raw file):

  // @throws std::exception if the mesh has extremely sharp features. Read
  // more explanation in MaybeCreate().
  explicit FeatureNormalSet(const TriangleSurfaceMesh<double>& mesh_M);

nit: I think we should consider killing the public constructor in place of two factory methods:

MaybeCreate() (as you have below).
CreateOrThrow() which would be a thin wrapper on MabyeCreate() that throws the message.

Alternatively, do we actually need the public constructor? I did a quick pass through, and it doesn't look like it. (A unit test might have to be massaged bit, but that seem to be any kind of obstacle.)

geometry/proximity/calc_signed_distance_to_surface_mesh.cc line 95 at r5 (raw file):

FeatureNormalSet::FeatureNormalSet(const TriangleSurfaceMesh<double>& mesh_M) {
  std::variant<FeatureNormalSet, std::string> v = MaybeCreate(mesh_M);
  DRAKE_THROW_UNLESS(std::holds_alternative<FeatureNormalSet>(v));

nit: This is not a good way to throw. This implicitly throws away the message, turning the string message into a boolean.

geometry/proximity/calc_signed_distance_to_surface_mesh.cc line 96 at r5 (raw file):

  std::variant<FeatureNormalSet, std::string> v = MaybeCreate(mesh_M);
  DRAKE_THROW_UNLESS(std::holds_alternative<FeatureNormalSet>(v));
  FeatureNormalSet f = std::get<FeatureNormalSet>(v);

nit: You should capture a reference to the variant and then move the normals.

geometry/proximity/distance_to_point_callback.h line 83 at r5 (raw file):

  /* Data for calculating signed distances of every mesh geometry.  */
  const std::unordered_map<GeometryId, VolumeMeshBoundary>& mesh_data;

nit: This is an incredibly generic name. Let's name it something that corresponds with the semantics of the data. mesh_boundaries_?

geometry/proximity/distance_to_point_callback.cc line 379 at r5 (raw file):

                             std::get<std::string>(mesh_G.feature_normal()));
  }
  const auto d = CalcSignedDistanceToSurfaceMesh(

nit: shouldn't be auto.

Code quote:

auto

geometry/proximity/distance_to_point_callback.cc line 383 at r5 (raw file):

      std::get<FeatureNormalSet>(mesh_G.feature_normal()));
  const double distance = d.signed_distance;
  const Vector3<T> p_GN_G = d.nearest_point;

nit: You're copying each of the vector3s twice. Probably better to dump these values straight into the constructor.

geometry/proximity/volume_mesh_boundary.h line 15 at r5 (raw file):

namespace internal {

class VolumeMeshBoundary {

nit: VolumeMeshBoundary can be a misleading name. We have a type called VolumeMesh but this can be instantiated from a TriangleSurfaceMesh. So, the relationship between VolumeMeshBoundary and VolumeMesh implied by the names isn't true.

It seems to me that the name isn't supposed to relate two types, but it's supposed to hint at a mesh that, in some sense, encloses a volume. Is that right?

If so, while it's a laudable goal, the type names we have in Drake means that this name invites ambiguity and possibly confusion. We should come up with a different name.

We can be more specific if we think it's domain of utility is limited to the current one: MeshDistanceBoundary. Only if we think this boundary would pick up meaning that isn't distance related should we stay away from the word "distance" in its name.

geometry/proximity/volume_mesh_boundary.h line 20 at r5 (raw file):

  // This version does not take ownership of the volume mesh. It creates and
  // stored associated data without the volume mesh itself.

nit: typo

Suggestion:

stores

geometry/proximity/volume_mesh_boundary.h line 34 at r5 (raw file):

  }
  const std::variant<FeatureNormalSet, std::string>& feature_normal() const {
    return feature_normal_M_;

BTW We might consider changing the semantics here. What if this returned a const FeatureNormalSet& or threw. We'd still get the behavior that if no one ever accessed the feature normals, we wouldn't throw. But it also means that call sites that need the feature normals don't have to worry about the semantics of the variant and handle the exception throwing itself.

geometry/proximity/volume_mesh_boundary.h line 45 at r5 (raw file):

  Bvh<Obb, TriangleSurfaceMesh<double>> tri_bvh_M_;

  // Provides angle weighted normals at vertices and edges of the

nit: This documentation repeats the documentation of the normal set and ignores the variant nature of the member.

  // Contains either the normals for the mesh's edges and vertices or an error
  // message explaining why no such normals could be computed.

geometry/proximity/test/distance_to_point_callback_test.cc line 507 at r5 (raw file):

// Simple smoke test for signed distance to Mesh or Convex. Separately we test
// the actual calculation in calc_signed_distance_to_surface_mesh_test.
// The objective of this test is to verify that there is a code path to the

nit: The stated purpose of the test does not align with the implementation. If the goal is simply "verify that there is a code path to the right function", these two calls are redundant. One call would be enough to establish that the function is getting dispatched.

(Although, as I look at the test in this file, there's a degree of confusion taking place here. Lots of tests are labeled "simple smoke test" but aren't. And the role of these per-shape tests vs the more general TestScalarShapeSupport()-invoking tests.)

But, at least for this PR, the DistanceToPoint functor has very limited logic when interacting with VolumeMeshBoundary. Here's what dut does and what should actually be tested:

If the VolumeMeshBoundary doesn't have feature normals, it should throw with whatever message is there.
It calls CalcSignedDistanceToSurfaceMesh().
It packages the results correctly (copies most values, but re-expresses gradient).

geometry/proximity/test/distance_to_point_callback_test.cc line 509 at r5 (raw file):

// The objective of this test is to verify that there is a code path to the
// right function.
GTEST_TEST(DistanceToPoint, Mesh) {

BTW For alphabetical reasons, this would be better between the HalfSpace and Sphere tests.

geometry/proximity/test/distance_to_point_callback_test.cc line 523 at r5 (raw file):

  // Test an exception with a zero-volume mesh with knife edges.
  {
    // All vertices of the tetrahedron is on the X-Y plane.

nit typo

Suggestion:

are

geometry/proximity/test/distance_to_point_callback_test.cc line 684 at r5 (raw file):

                       &X_WGs,       &mesh_data, &distances};

  // The Drake-supported geometries (minus Mesh which isn't supported by

nit: This comment is no longer valid.

geometry/proximity/test/distance_to_point_callback_test.cc line 765 at r5 (raw file):

}

// Test coverage of point_distance::Callback() for meshes (Mesh and Convex).

I suspect we're over-testing the Callback vis a vis Mesh/Convex. point_dstance::Calback only as one responsibility:

If the fcl representation is fcl::GEOM_CONVEX, check to see if we have sdf data. If so, dispatch it to the DistanceToPoint functor, otherwise make it a no-op and move on.

Testing anything else is essentially redundantly testing another (hopefully already tested) entity.

geometry/proximity/test/distance_to_point_callback_test.cc line 797 at r5 (raw file):

  mesh_encoding.write_to(&mesh_collision_object);

  auto unsupported_mesh_fcl_geometry = make_shared<fcl::Convexd>(

nit: You don't have to declare a brand new geometry to test "unsupported' geometry. You just pass an empty map for the so-called "mesh_data".

geometry/proximity/test/distance_to_point_callback_test.cc line 815 at r5 (raw file):

  // changes to support fcl::GEOM_CONE, we will change to another unsupported
  // shape.
  auto unsupported_shape = make_shared<fcl::Coned>(0.1, 0.5);  // radius, lz

This is an example of overtesting. Ostensibly, this test is about Mesh-specific things. Putting in a geometry with an unsupported fcl representation means that we don't go near the mesh-specific code.

geometry/proximity/test/distance_to_point_callback_test.cc line 889 at r5 (raw file):

  {
    CallbackData<double> callback_data{
        &query_point, kThresholdZero, p_WQ, &X_WGs, &mesh_data, &distances};

nit; None of the code supporting Mesh/Convex introduces any special behavior regarding the threshold, so this is also redundant.

geometry/test/proximity_engine_temporary_test.cc line 0 at r5 (raw file):
Blocking comment based on the understanding that this file will go away.

Is this code simply a playground, or will you be moving this test code into proximity_engine_test.cc and, therefore, this code should be properly reviewed?

tutorials/hydroelastic_contact_nonconvex_mesh.ipynb line 0 at r5 (raw file):
Blocking comment to account for the implied suggestion that this file will disappear from the final PR.

DamrongGuoy

Reviewable status: 24 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity_engine.cc line 556 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Actually, we even need it for ComputeSignedDistanceToPoint queries as all non-convex meshes are represented by their convex hulls in the BVH. Perhaps the following instead:
// We currently represent Mesh shapes with their convex hulls in fcl.

Done. Thanks. I appreciate the proposed document.

geometry/proximity_engine.cc line 1052 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Let's make this more precise:
    // Meshes are unsupported if we cannot compute a VolumeMeshBoundary.
    // point_distance::Callback() skips every Mesh that doesn't have an entry
    // in mesh_sdf_data_.
This makes it clear why file types are unsupported and suggests that this is the logic that defines whether a Mesh is supported or not and that point_distance::Callback simply keys on the signal defined here.

Done. Thank you. I appreciate the suggested document.

geometry/proximity_engine.cc line 1154 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'd leave off all of the elaboration. It's documented elsewhere. And just gives us one more site to keep in sync.

Done. Thanks. I left off all of the elaborations and added "(Mesh and Convex)", so we know the word "meshes" refer to both Mesh and Convex.

geometry/query_object.h line 747 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Done below.

Done with the new proposed document.

geometry/query_object.h line 749 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Nit: this footnote isn't needed at all. Convex is already defined as a convex hull (as specified by a mesh). And a reported value here already implies normal support. No further statement isn't required.

Done. I removed the footnote for Convex. Thanks.

geometry/query_object.h line 751 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: No need to specify "triangle meshes" in OBJ.

Done. I used your suggested doc below, which is greatly appreciated.

geometry/query_object.h line 753 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I see the language here borrows from the previous version. In its old version, it may have been slightly redundant. But, at the very least, it was trying to make something clear about the relationship between a tetrahedral mesh and the signed distance. With broader support of more mesh types, it's less targeted and feels far more redundant. I'd suggest simply:
   - ᶜ Only supports OBJ and tetrahedral VTK meshes. Unsupported meshes are
       simply ignored; no results are reported for that geometry. For OBJ meshes
       the surface mesh must satisfy specific requirements (see below). Unlike
       the other Shapes, witness points and gradients can be discontinuous on a
       mesh's exterior if it is non-convex.

Done. Thank you. I appreciate the suggested document. It tells me precisely what to do.

geometry/query_object.h line 759 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This query suffers from the same issues that computation of a mesh's spatial inertia does. It has some requirements on the mesh that aren't validated but necessary for the final result to be valid. I'd propose an extra phrase:
   @pre The %Mesh of a triangular surface mesh must be a closed manifold
   without duplicate vertices or self-intersection, and every triangle's face
   winding gives an outward-pointing face normal. Drake does not currently
   validate the input mesh with respect to these properties. Instead, it does a
   good-faith computation assuming the properties, possibly returning incorrect
   results. Non-compliant meshes will ...
The same applies to tet meshes (see my note below). It probably would be better saying that this query has certain requirements for the meshes, indicate what the requirements are, and then state one time about it being untested and the possible ramifications of the requirements not being met.

Done. Thanks. I added the proposed extra phrase.

geometry/shape_specification.h line 470 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Don't say "triangle .obj". We automatically triangulate it when we parse .obj file. Although, "tetrahedral .vtk" is, of course, quite appropriate.

Done. Thanks. I removed the word "triangle."

geometry/proximity/distance_to_point_callback.cc line 379 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: shouldn't be auto.

Done. Thanks. I changed auto to SignedDistanceToSurfaceMesh.

geometry/proximity/distance_to_point_callback.cc line 383 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You're copying each of the vector3s twice. Probably better to dump these values straight into the constructor.

Done. I hope this change satisfies what you meant by "dump these values straight into the constructor". Please let me know, otherwise.

geometry/proximity/volume_mesh_boundary.h line 20 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: typo

Done. Thanks.

geometry/proximity/volume_mesh_boundary.h line 45 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This documentation repeats the documentation of the normal set and ignores the variant nature of the member.
  // Contains either the normals for the mesh's edges and vertices or an error
  // message explaining why no such normals could be computed.

Done. Thanks. I used the proposed document together with the mention of frame M.

DamrongGuoy

Reviewable status: 21 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity/calc_signed_distance_to_surface_mesh.h line 53 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I think we should consider killing the public constructor in place of two factory methods:

MaybeCreate() (as you have below).

CreateOrThrow() which would be a thin wrapper on MabyeCreate() that throws the message.

Alternatively, do we actually need the public constructor? I did a quick pass through, and it doesn't look like it. (A unit test might have to be massaged bit, but that seem to be any kind of obstacle.)

Done. Per f2f discussion, we keep only MaybeCreate() without the public constructor.

geometry/proximity/calc_signed_distance_to_surface_mesh.cc line 95 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This is not a good way to throw. This implicitly throws away the message, turning the string message into a boolean.

Done. I removed the public constructor as suggested in the other comment.

I agree it's not a good way to throw. If we keep this constructor, I should throw the error message somewhat like this:

if (holds_alternative<string>(v))
  throw std::runtime_error("FeatureNormalSet: " + std::get<string>(v));

geometry/proximity/calc_signed_distance_to_surface_mesh.cc line 96 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You should capture a reference to the variant and then move the normals.

Done. I removed the public constructor.

I agree it should have been move-assignment instead of copy-assignment.

DamrongGuoy

Reviewable status: 19 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity/volume_mesh_boundary.h line 15 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: VolumeMeshBoundary can be a misleading name. We have a type called VolumeMesh but this can be instantiated from a TriangleSurfaceMesh. So, the relationship between VolumeMeshBoundary and VolumeMesh implied by the names isn't true.

It seems to me that the name isn't supposed to relate two types, but it's supposed to hint at a mesh that, in some sense, encloses a volume. Is that right?

If so, while it's a laudable goal, the type names we have in Drake means that this name invites ambiguity and possibly confusion. We should come up with a different name.

We can be more specific if we think it's domain of utility is limited to the current one: MeshDistanceBoundary. Only if we think this boundary would pick up meaning that isn't distance related should we stay away from the word "distance" in its name.

Done. Thanks. Renamed VolumeMeshBoundary to MeshDistanceBoundary.

geometry/proximity/volume_mesh_boundary.h line 34 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW We might consider changing the semantics here. What if this returned a const FeatureNormalSet& or threw. We'd still get the behavior that if no one ever accessed the feature normals, we wouldn't throw. But it also means that call sites that need the feature normals don't have to worry about the semantics of the variant and handle the exception throwing itself.

Thank you. I'd like to throw at the higher level, please. The caller (DistanceToPoint()) can give more info when it throws.

SeanCurtis-TRI

BTW I've been only keeping a distant eye on this. Please let me know when you're ready for the next full review pass.

Reviewable status: 19 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

DamrongGuoy

Thanks for asking. I'll be working on #22137 for a few days and return to clean up this PR and start the full review.

Reviewable status: 19 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

jwnimmer-tri · 2024-11-07T19:13:37Z

@DamrongGuoy this PR is your highest and only priority. Do not work on anything else until this merges.

DamrongGuoy

Ok. I'll pause #22137 and work on this PR first.

Reviewable status: 19 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

DamrongGuoy

Reviewable status: 12 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity_engine.cc line 396 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I'm not clear what that would entail. Surely the mesh_sdf_data_ doesn't depend on any ProximityProperties.

Done. I removed the TODO. Thank you for confirming that mesh_sdf_data_ doesn't depend on ProximityProperties. Previously I wasn't sure.

geometry/query_object.h line 735 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Where does this number come from? It should be supported in the characterization test. And yet, distance_to_point_characterize_test.cc still reports Convex as being ignored.

This also has made the MeshIsConvex() test (in charcterization_utilities.h invalid. And it probably was always incorrect because it doesn't allow for a Mesh having multiple representations based on the query. It was a laudable goal, but, nevertheless it proves to be more wrong than right.

The number was my guess. I'll have to work on distance_to_point_characterize_test to get the right number. Set to working.

geometry/proximity/BUILD.bazel line 274 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW It's not clear to me why the calc_signed_distance_to_surface_mesh build target got rolled into here. Can you explain why? (This is a request for a change to the PR, just an explanation in this discussion of what I'm missing.)

Thanks for asking. There was a circular dependency that I'm not ready to fix in this PR.

CalcSignedDistanceToSurfaceMesh needs the box distance in distance_to_point_callback (distance between the query point and a bounding box of a BVH's node).
distance_to_point_callback calls CalcSignedDistanceToSurfaceMesh.

geometry/proximity/distance_to_point_callback.h line 83 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This is an incredibly generic name. Let's name it something that corresponds with the semantics of the data. mesh_boundaries_?

Done. Thanks. I like the new name.

geometry/proximity/test/distance_to_point_callback_test.cc line 507 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: The stated purpose of the test does not align with the implementation. If the goal is simply "verify that there is a code path to the right function", these two calls are redundant. One call would be enough to establish that the function is getting dispatched.

(Although, as I look at the test in this file, there's a degree of confusion taking place here. Lots of tests are labeled "simple smoke test" but aren't. And the role of these per-shape tests vs the more general TestScalarShapeSupport()-invoking tests.)

But, at least for this PR, the DistanceToPoint functor has very limited logic when interacting with VolumeMeshBoundary. Here's what dut does and what should actually be tested:

If the VolumeMeshBoundary doesn't have feature normals, it should throw with whatever message is there.

It calls CalcSignedDistanceToSurfaceMesh().

It packages the results correctly (copies most values, but re-expresses gradient).

Done. I redo the test. Thank you for listing the three properties that we want to test.

geometry/proximity/test/distance_to_point_callback_test.cc line 509 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW For alphabetical reasons, this would be better between the HalfSpace and Sphere tests.

Done. Thanks. I moved it to the appropriate location.

geometry/proximity/test/distance_to_point_callback_test.cc line 523 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit typo

Done. Thanks.

geometry/proximity/test/distance_to_point_callback_test.cc line 684 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This comment is no longer valid.

Done. Removed. Thanks. I also moved the fcl::Convexd case to the correct alphabetic order.

DamrongGuoy

Reviewable status: 10 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity/test/distance_to_point_callback_test.cc line 765 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I suspect we're over-testing the Callback vis a vis Mesh/Convex. point_dstance::Calback only as one responsibility:

If the fcl representation is fcl::GEOM_CONVEX, check to see if we have sdf data. If so, dispatch it to the DistanceToPoint functor, otherwise make it a no-op and move on.

Testing anything else is essentially redundantly testing another (hopefully already tested) entity.

Done. Thank you for the description of the responsibility of point_distance::Callback, which I used as a guideline to simplify the test.

geometry/proximity/test/distance_to_point_callback_test.cc line 797 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You don't have to declare a brand new geometry to test "unsupported' geometry. You just pass an empty map for the so-called "mesh_data".

Done. Thank you. That simplified the test a lot.

geometry/proximity/test/distance_to_point_callback_test.cc line 815 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This is an example of overtesting. Ostensibly, this test is about Mesh-specific things. Putting in a geometry with an unsupported fcl representation means that we don't go near the mesh-specific code.

Done. Removed it. Thanks.

geometry/proximity/test/distance_to_point_callback_test.cc line 889 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit; None of the code supporting Mesh/Convex introduces any special behavior regarding the threshold, so this is also redundant.

Done. Removed. Thanks.

DamrongGuoy

-(status: do not merge) -(status: do not review) I also removed the draft status.

+@SeanCurtis-TRI for feature review, please.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Something just occurred to me. Is there a reason that CalcSignedDistnaceToSurfaceMesh is double-valued and not T-valued? I.e., in principle, it could take a Vector3<T> p_MQ. We should make it clear why we've excluded T or put in a TODO for what it would take to have it.

I do double first to deliver the code early. I don't know AutoDiff enough to be sure. I wonder whether the iterative search in CalcSignedDistanceToSurfaceMesh can play well with AutoDiff in terms of speed and quality.

DamrongGuoy

Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/query_object.h line 735 at r5 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

The number was my guess. I'll have to work on distance_to_point_characterize_test to get the right number. Set to working.

Done. They are 4e-15 instead of 1e-13. Thank you for helping me modify the test.

geometry/query_object.h line 771 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

While assigning the name 'simplicial complex' to an acceptable mesh is nice, I don't know how much value it adds. You provide the label and then define it. Reference to terms like that are best if the term replaces the definition. A label and definition only makes sense if you plan on using the label moving forward. That doesn't seem to be the case here.

Instead, we should unify our discussion of volume mesh with surface mesh. We have requirements on the meshes for this algorithm to produce meaningful results. So, let's enumerate the requirements for each kind of mesh and then discuss the possible issues that might/will arise if they aren't met.

Done. Per the f2f discussion, changed to the new writing. Thank you for helping me draft the new writing.

DamrongGuoy

Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

geometry/test/proximity_engine_temporary_test.cc line at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Blocking comment based on the understanding that this file will go away.

Is this code simply a playground, or will you be moving this test code into proximity_engine_test.cc and, therefore, this code should be properly reviewed?

Done. I removed it. There is no need to review. Thanks.

tutorials/hydroelastic_contact_nonconvex_mesh.ipynb line at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Blocking comment to account for the implied suggestion that this file will disappear from the final PR.

Done. I removed it. Thanks.

DamrongGuoy

CC: @joemasterjohn @rpoyner-tri in case you want to see how it works.

Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

DamrongGuoy

Reviewable status: 8 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/query_object.h line 735 at r5 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Done. They are 4e-15 instead of 1e-13. Thank you for helping me modify the test.

And 5e-15 on mac-arm-sonoma-clang-bazel-experimental-release.

DamrongGuoy added status: do not merge release notes: feature This pull request contains a new feature labels May 22, 2024

jwnimmer-tri added the status: do not review label May 23, 2024

jwnimmer-tri self-assigned this May 23, 2024

jwnimmer-tri reviewed May 28, 2024

View reviewed changes

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from c33fff4 to 03421f9 Compare June 5, 2024 18:24

DamrongGuoy commented Jun 5, 2024

View reviewed changes

jwnimmer-tri reviewed Jun 5, 2024

View reviewed changes

SeanCurtis-TRI reviewed Jun 5, 2024

View reviewed changes

jwnimmer-tri removed their assignment Jun 7, 2024

DamrongGuoy mentioned this pull request Jun 13, 2024

[geometry] Calculate signed distance from point to closed meshes #21566

Merged

DamrongGuoy changed the title ~~WIP [geometry] ComputeSignedDistanceToPoint for compliant-hydroelastic tetrahedral meshes~~ WIP [geometry] ComputeSignedDistanceToPoint for tetrahedral meshes Jun 13, 2024

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from 03421f9 to f4a9bf4 Compare July 25, 2024 04:28

DamrongGuoy changed the title ~~WIP [geometry] ComputeSignedDistanceToPoint for tetrahedral meshes~~ WIP [geometry] ComputeSignedDistanceToPoint for meshes Jul 25, 2024

DamrongGuoy commented Jul 25, 2024

View reviewed changes

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from f4a9bf4 to 54fb3a6 Compare July 25, 2024 04:50

DamrongGuoy commented Jul 25, 2024

View reviewed changes

calderpg-tri reviewed Jul 25, 2024

View reviewed changes

DamrongGuoy mentioned this pull request Aug 29, 2024

Convex (and Mesh) support for QueryObject::ComputeSignedDistanceToPoint #21323

Open

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from 54fb3a6 to 73b657d Compare October 22, 2024 22:57

DamrongGuoy commented Oct 24, 2024

View reviewed changes

SeanCurtis-TRI reviewed Oct 24, 2024

View reviewed changes

SeanCurtis-TRI reviewed Oct 28, 2024

View reviewed changes

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from c408566 to eb15210 Compare October 28, 2024 23:36

DamrongGuoy commented Oct 28, 2024

View reviewed changes

DamrongGuoy commented Oct 30, 2024

View reviewed changes

DamrongGuoy commented Oct 31, 2024

View reviewed changes

SeanCurtis-TRI reviewed Nov 6, 2024

View reviewed changes

DamrongGuoy commented Nov 7, 2024

View reviewed changes

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from 105d118 to 1ab912d Compare November 8, 2024 04:17

DamrongGuoy commented Nov 8, 2024

View reviewed changes

DamrongGuoy changed the title ~~WIP [geometry] ComputeSignedDistanceToPoint for meshes~~ [geometry] ComputeSignedDistanceToPoint for meshes Nov 8, 2024

DamrongGuoy marked this pull request as ready for review November 8, 2024 04:32

DamrongGuoy removed status: do not merge status: do not review labels Nov 8, 2024

DamrongGuoy assigned SeanCurtis-TRI Nov 8, 2024

DamrongGuoy commented Nov 8, 2024

View reviewed changes

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from 1ab912d to 46ee0ca Compare November 13, 2024 00:05

DamrongGuoy commented Nov 13, 2024

View reviewed changes

[geometry] ComputeSignedDistanceToPoint for meshes

b2de8d0

DamrongGuoy force-pushed the doc_mesh_signed_distance_to_point branch from 46ee0ca to b2de8d0 Compare November 13, 2024 00:15

DamrongGuoy commented Nov 13, 2024

View reviewed changes

5e-15 for mac-arm-sonoma-clang-bazel-experimental-release

6e748b6

DamrongGuoy commented Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[geometry] ComputeSignedDistanceToPoint for meshes #21471

[geometry] ComputeSignedDistanceToPoint for meshes #21471

DamrongGuoy commented May 22, 2024 •

edited

Loading

jwnimmer-tri commented May 23, 2024

jwnimmer-tri left a comment

DamrongGuoy left a comment

jwnimmer-tri left a comment

SeanCurtis-TRI left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

calderpg-tri left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

SeanCurtis-TRI left a comment

DamrongGuoy left a comment

jwnimmer-tri commented Nov 7, 2024

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

[geometry] ComputeSignedDistanceToPoint for meshes #21471

Are you sure you want to change the base?

[geometry] ComputeSignedDistanceToPoint for meshes #21471

Conversation

DamrongGuoy commented May 22, 2024 • edited Loading

jwnimmer-tri commented May 23, 2024

jwnimmer-tri left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

calderpg-tri left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 7, 2024

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy commented May 22, 2024 •

edited

Loading