search

3MAH / microgen

Microstructure generation
https://3mah.github.io
GNU General Public License v3.0
68 stars 8 forks source link

Fix infill and coordinates issue #65

Closed kmarchais closed 6 months ago

kmarchais commented 6 months ago

Fixes #64.

Since the introduction of the infill feature, the cylindrical and spherical tpms geometries were not correctly computed and were using wrong coordinates to compute the tpms field in the grid.

To generate a cylindrical grid for example, we must transform the cartesian coordinates x, y, z to cylindrical coordinates. When computing the TPMS field, we need to use the original x, y, z to have this rotation effect.

This animation may explain better than words. coords

After bringing the infill feature, since the grid where is computed the tpms field has not the same shape as x, y, z after clipping with the object infilled, one option was to get x, y, z by using the grid's points attribute.

But this is working only in a cartesian frame. In cylindrical or spherical coordinates the coordinates of the points are not the original x, y, z of the meshgrid.

To solve this problem, one solution is to store the original coordinates as a field in the grid to use them in the computation of the tpms field. Maybe there is a better solution.

ricardo0115 commented 6 months ago

To facilitate the PR review for reviewers can you describe and talk a little bit more about the bug that you are trying to fix and how you did it please ?

kmarchais commented 6 months ago

Thanks for the review, I added some explanation of the problem solved in the first message.

rdesparbes commented 6 months ago

The correction looks good to me! Is there a way to add a test to modify an existing one to avoid regressions for this correction?

kmarchais commented 6 months ago

Thanks for your comment, I just added a test that checks if the coords field is the same as the corresponding cartesian coordinates. Until now, I couldn't find how to test if we have the correct tpms field computed.

ricardo0115 commented 6 months ago

LGTM, is important to take into account that the test test_tpms_given_coord_system_tpms_coordinates_field_must_be_in_cartesian_frame is testing an implementation detail such as storing the grid coordinates in a dictionary somewhere in the tpms object. This test may broke easily later with minor modifications. We can keep it for the moment but we should think in a better way to test this in the future.