3DLIRIOUS / MeshLabXML

Create and run MeshLab XML scripts with Python
GNU Lesser General Public License v2.1
202 stars 43 forks source link

Added parsing of hausdorff distance #7

Closed DavidWatkins closed 6 years ago

DavidWatkins commented 6 years ago

There was no functionality to parse the computed hausdorff distance between two meshes. I have added a parse_hausdorff function inside of compute.py. This creates a dictionary of five values, the min_distance, max_distance, mean_distance, rms_distance, and number_points which are returned by the call. Can you pull this into the main branch?

3DLIRIOUS commented 6 years ago

Thanks a lot for adding this feature!

I did have a question regarding the addition of the default_geometry, default_topology and default_hausdorff_distance functions. It appears that these were added in order to initialize self.geometry, etc. when the Filterscript class is called, however I don't think that this is desired. The self.geometry, etc. attributes should not exist unless the measure_geometry, etc. functions are called and the attributes are populated with real data. Am I perhaps missing a consideration where initializing these for every script would be desirable?

DavidWatkins commented 6 years ago

I was trying to make sure the code passed my linter. Namely I got this error otherwise: "instance attribute is defined outside the init method". This is not something that is strictly necessary and definitely can be omitted for the sake of merging this into master.

3DLIRIOUS commented 6 years ago

I read up on that error, and I agree that it is good practice to initialize all variables in init, even if just for the sake of readability. However, I think that these values should be initialized as None instead of default values. I would prefer to keep the initialization of the defaults inside the parsing functions. Would you mind making this change before I commit?

DavidWatkins commented 6 years ago

Already done. Let me know if there is anything else you recommend otherwise I think it's good for merging.