normal vectors are not unit normals

[metsean]

When using 'update normals' option on save, normal vectors are calculated as cross product between two edges but then need to be normalised by their length (so that norm =1). Suggest using numpy.linalg.norm for this.

[wolph]

It's fairly easy to normalize them but I'm not the biggest fan of changing existing behaviour.

Is there any real benefit to doing this? As opposed to leave this to the user. I'm aware that the normals are unit vectors in most cases but it's not a requirement, right?

[metsean]

Unit normal is specified for the stl file format: https://www.fabbers.com/tech/STL_Format

I had output file errors which I thought were due to this but turn out to be unrelated. So sorry about this flag - I have no evidence that non-normalised unit vectors are an issue. Thanks for the quick reply and the excellent package.

[wolph]

Hmm, fair enough :)

I've added a bit of code that should do the trick. Can you test if the new version on develop (or master) works for you? I think I've implemented it correctly but linear algebra has never been my strong suit ;)

[metsean]

Not quite working as is not calculating normal for each facet. Try the below.

def update_normals(self): '''Update the normals for all points''' normals = numpy.cross(self.v1 - self.v0, self.v2 - self.v0) normal = numpy.linalg.norm(normals,axis=1) normals /= normal[:,None] self.normals[:] = normals

[wolph]

I've updated the code, thank you :)

Can you try again?

[metsean]

It's failing on your if check as normals is a an array of size Nx1. Suggestion:

def update_normals(self): '''Update the normals for all points''' normals = numpy.cross(self.v1 - self.v0, self.v2 - self.v0) normal = numpy.linalg.norm(normals, axis=1) if normal.any(): normals /= normal[:, None] self.normals[:] = normals

[wolph]

I've made another change to hopefully handle the zero values correctly.

The normal.any() doesn't actually solve it because it could still contain 0 values. A normal.all() would work, but I'm not sure how that would impact the calculations so I've opted for only performing calculations on non-zero values. I hope that's correct.

[metsean]

Tested DEV and all OK. Thanks for sorting this out!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.