Skip to content

Conversation

@psomers3
Copy link
Contributor

@psomers3 psomers3 commented Mar 4, 2021

Guess I was too quick to say I can't add more bindings to this...
I've just added some getters that are specific to BaseMeshTopology. Mostly so that it is possible to access tetrahedron mesh data when making a custom forcefield.

@jnbrunet
Copy link
Contributor

jnbrunet commented Mar 8, 2021

Hey @psomers3 ,

Thanks for these additional bindings.

Why the static vectors in convertEdge, convertTetra and edgesInTetrahedron ?

@psomers3
Copy link
Contributor Author

psomers3 commented Mar 8, 2021

I went off what was done with the helper functions in BaseCamera. It should help to be faster since these functions are likely to end up in a loop over a mesh, correct? So it doesn't reallocate memory every call. Although I'm not sure how the resize to the same size is handled, but I had the problem that when I initialized the vectors to a size it didn't make them the size I initialized them to...

@jnbrunet
Copy link
Contributor

jnbrunet commented Mar 8, 2021

I went off what was done with the helper functions in BaseCamera. It should help to be faster since these functions are likely to end up in a loop over a mesh, correct? So it doesn't reallocate memory every call. Although I'm not sure how the resize to the same size is handled, but I had the problem that when I initialized the vectors to a size it didn't make them the size I initialized them to...

I see. I wasn't aware of this in BaseCamera. I would be very surprised if this has any performance benefits since a new vector will be created anyway by the returned value of the method. You also prevent the compiler to copy elide the vector. In addition, looping on the elements of the mesh using this method is probably a bad idea anyway, one should instead get hold of the reference to the "vector of elements" directly and bind it to an python (or numpy) array, hence avoiding any copies (see BaseMeshTopology::getTetras() and friends).

Anyway, I think I would remove these static variables since they complexity the code. std::array should also be used for statically known dimension, something like:

c.def("getEdge", 
      [] (const BaseMeshTopology & self, const sofa::Index & index) -> std::array<sofa::Index, 2> {
          const auto & e = self.getEdge(index);
          return {{e[0], e[1]}};
      },
      py::arg("index")
);

c.def("getLocalEdgesInTetrahedron", 
      [] (const BaseMeshTopology & self, const sofa::Index & index) -> std::array<sofa::Index, 2> {
          const auto & e = self.getLocalEdgesInTetrahedron(index);
          return {{e[0], e[1]}};
      },
      py::arg("index"),
      "Returns for each index (between 0 and 5) the two vertex indices that are adjacent to that edge."
);

c.def("getEdgesInTetrahedron", 
      [] (const BaseMeshTopology & self, const sofa::Index & index) -> std::array<std::array<sofa::Index, 2>, 6> {
          const auto & e = self.getEdgesInTetrahedron(index);
          return {{
              {e[0][0], e[0][1]}, // Edge 0
              {e[1][0], e[1][1]}, // Edge 1
              {e[2][0], e[2][1]}, // Edge 2
              {e[3][0], e[3][1]}, // Edge 3
              {e[4][0], e[4][1]}, // Edge 4
              {e[5][0], e[5][1]}  // Edge 5
          }};
      },
      py::arg("index"),
      "Returns the set of edges adjacent to a given tetrahedron."
);

c.def("getTetrahedron", 
      [] (const BaseMeshTopology & self, const sofa::Index & index) -> std::array<sofa::Index, 4> {
          const auto & n = self.getTetrahedron(index);
          return {{n[0], n[1], n[2], n[3]}};
      },
      py::arg("index"),
      "Returns the vertices of Tetrahedron at index."
);

Let me know what you think.

@psomers3
Copy link
Contributor Author

psomers3 commented Mar 8, 2021

Thanks a ton for the info! You've provided a much cleaner way to do these wrappings. I think you're right with the maintaining a reference to the tetrahedrons directly (which already works since "tetras" is a Data object. Super convenient how you guys did that btw).

My only issue getting your code to compile (well, I did get it to compile) is that the const BaseMeshTopology & self cannot be const for the functions:

  • getEdge
  • getEdgesInTetrahedron
  • getTetrahedron

So I guess somewhere in the call hierarchy something is not const? I can't find it though... I can take a deeper look later, but at that level it seems to be a Sofa thing and not a SofaPython3. Would you be fine leaving the const away as needed?

Again, thanks for the programming tips! It's really encouraging to want to contribute to the community since you guys are so helpful.

@jnbrunet
Copy link
Contributor

jnbrunet commented Mar 9, 2021

Yeah I just checked, and BaseMeshTopology has these signatures:

const Edge getEdge(EdgeID i) override;
const EdgesInTetrahedron& getEdgesInTetrahedron(TetraID i) override;
const Tetra getTetrahedron(TetraID i) override;

which aren't const.

So yeah, just leave out the constiness here ;)

@psomers3
Copy link
Contributor Author

psomers3 commented Mar 9, 2021

changes added. I did change the getEdgesInTetrahedron from what you posted because the actual function doesn't return the edges as a list of vertices.

Copy link
Contributor

@jnbrunet jnbrunet left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot!

@jnbrunet jnbrunet merged commit c2198b8 into sofa-framework:master Mar 9, 2021
@guparan guparan added this to the v21.06 milestone Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants