BerkeleyAutomation / meshpy

A 3-D triangular mesh package for Python.
Apache License 2.0
42 stars 21 forks source link

A bug(?) on non-integer coordinate's sdf value computation #3

Closed lianghongzhuo closed 6 years ago

lianghongzhuo commented 6 years ago

I just found here. Is it correct to change the type of self.coords_buf? If so, after this line of code, every call of this method with non-integer coordinate will turn into integer. And we all know in Python, the default mechanism of this is floor.

jeffmahler commented 6 years ago

I don't believe this is a bug because the previous if statement ensures that the coordsbuf contains integers. It may be a redundant line of code, however.

Did you experience any specific issues as a result of this? If so perhaps we can investigate further.

I'd also like to note that we will be replacing meshpy with trimesh in the dex-net repo sometime this summer, so we don't have intentions of supporting this code long-term.

jeasinema commented 6 years ago

Thanks for your reply. In fact, if this line of code is executed, then the type of self.coords_buf_ will turn into the np.int from np.float64 forever, which means that, at next time when we call this method with non-integer coords, in Line295-297(here) , it will be unexpectedly floored into integer since now the type of self.coords_buf_ is np.int, and we will not get the accurate sdf value of this non-integer coords.

By the way, although np.int == int will be True, np.int32/int64 is not equal to int at all. Since at most of the time the default type of integer in numpy is np.int64, the if statement you mentioned will mostly not be executed. This can explain why this function mostly works well.

At last, thanks for your hard work on migrating dex-net to trimesh. I'm looking forward to the next official release!

jeffmahler commented 6 years ago

Ah I see. It seems that this could be fixed by copying the coordsbuf object and changing the if statement to use the correct numpy types. @jeasinema Would you be interested in doing a PR for this?

jeasinema commented 6 years ago

@jeffmahler Please see #4.

jeffmahler commented 6 years ago

Fixed per #4