Hotfix/binary normals #164

jimfoltz · 2016-12-22T01:27:32Z

No description provided.

thomthom

I don't think this is a correct fix - we might produce incorrect normals. See inline comment for details.

thomthom · 2016-12-22T09:43:06Z

src/sketchup-stl/exporter.rb

      def self.write_face(file, face, scale, tform)
        normal = face.normal
        normal.transform!(tform)
        normal.normalize!
-        mesh = face.mesh(0)
+        mesh = face.mesh(0 | 4)


I'm curious though, write_face_binary takes the vertex normal from the mesh. Even though the method is given a normal. Also, the normal we pass on has been processed to match the nested transformation currently in play.

See write_face
https://github.com/SketchUp/sketchup-stl/blob/master/src/sketchup-stl/exporter.rb#L97

We compute a global normal and pass that to write_face_binary
https://github.com/SketchUp/sketchup-stl/blob/master/src/sketchup-stl/exporter.rb#L133

But instead of using it we used the an arbitrary vertex normal. One potential issue here is that the normal we use isn't transformed to the global space. But also that we use a vertex-normal - assuming it's the same direction as the face. If an edge is soft/smooth then the vertex normals connected to that face will not be the same direction as the face.

I think the correct fix here is to use the normal we extract from face.normal - similar to what we do in write_face_ascii.

jimfoltz · 2016-12-22T09:53:24Z

I will need to look at this later, but the docs for normal_at do not say which normal is returned - the polygon normal or the vertex normal? You're saying it is a vertex normal?

jimfoltz · 2016-12-22T11:00:54Z

So we get a mesh from a face. The face may be divided into x polygons but all the polygons would have the same normal as the face. I agree the face normal could/should be used instead. I'll need to revisit it after work.

thomthom · 2016-12-22T11:14:45Z

Yes, the docs are lacking there. I'll see if I can improve that.

But yes, it's vertex normals. You can also see the exporter code get the index for a vertex in the polygon: norm = mesh.normal_at(polygon[0].abs) It just picks the index of the first vertex in the triangle and use that as the facet normal.

thomthom · 2016-12-22T11:16:09Z

So we get a mesh from a face. The face may be divided into x polygons but all the polygons would have the same normal as the face.

Yes, since SketchUp doesn't have any support for custom face normals all the triangles would always be perpendicular to the face's plane.

thomthom

I would like to reverse the changes and comment in self.write_face and the change to the attribute byte count before merging this.

thomthom · 2016-12-23T10:49:43Z

src/sketchup-stl/exporter.rb

+      # GitHub Issue #163: Binary preview is black in Mac OS.
+      # Caused because normals were not being requested in the Face#mesh call,
+      # and so not being exported.
+      # face.mesh was being called using only 0. We need to request normals


This comment isn't needed any more.

thomthom · 2016-12-23T10:49:59Z

src/sketchup-stl/exporter.rb

      def self.write_face(file, face, scale, tform)
        normal = face.normal
        normal.transform!(tform)
        normal.normalize!
-        mesh = face.mesh(0)
+        mesh = face.mesh(0 | 4)


We don't need to get the normals from the mesh any more.

thomthom · 2016-12-23T10:55:14Z

src/sketchup-stl/exporter.rb

+            # 2-byte "Attribute byte count" spacer. Nonstandard use by some stl software
+            # to store color data. Was never widely supported. Should be 0. 
+            # C - Integer: 8-bit unsigned
+            file.write([0, 0].pack("CC"))


The STL specs says that this is an unsigned 16bit int. It's not two separate bytes.

http://www.fabbers.com/tech/STL_Format#Sct_binary
https://en.wikipedia.org/wiki/STL_(file_format)#Binary_STL

The way I read the specs is that this uint16_t is the size of an optional data section that can follow the facet data. Though some seem to have mis-used it as two "free" bytes.

For this reason I think this should be reversed back to [0].pack('v') as it correctly reflect the file specs.

I guess it depends on which spec we go by. Wikipedias says it's a single 16-bit unsigned int while fabbers says it's two 8-byte unsigned ints. In practice there is no difference. It was the "VAX" that threw me. I think "S<" is what we want, although the table for Array.pack isn't completely clear. I'll try to find out.

http://ruby-doc.org/core-2.0.0/Array.html#method-i-pack

Edit - I'm sorry confusing size and type. The 3rd column of the Array.pack table isn't line-for-line - it is read as one cell of comments.

Concusion: "S<" is the more correct, clear option.

A unsigned int that is 2 bytes in size is a uint16 - their description isn't accurate to the data struct in their illustration.

Hm... so S< is little endian? It doesn't end up as native endian?

Hm... so S< is little endian? It doesn't end up as native endian?

It's hard to tell when we fill them with zeroes anyway. But I can test with some other value to be certain.

Yes, "S<" right.

# 16-bit unsigned integer, VAX (little-endian) byte order [1].pack("v") => "\x01\x00" # 16-bit unsigned int, little endian [1].pack("S<") => "\x01\x00" # 16-bit insigned int, native-endian [1].pack("S") => "\x01\x00" # 16-bit unsigned int, big-endian [1].pack("S>") => "\x00\x01"

No longer need vertex normals from mesh. Write "attribute byte count" spacer as 16-bit unsigned int, little-endian

thomthom

Looks good. I checked out the branch - made an export and checked it with 010 Viewer. Facet normals looked right. I also tried it on my mac and the preview displayed as expected.

I'll see if I can get around to upload a new build to EW next week.

jimfoltz added 2 commits December 21, 2016 20:11

request normals for face.mesh. FIxes #163

1977ae3

corrected issue number

7079faf

thomthom self-requested a review December 22, 2016 09:42

thomthom requested changes Dec 22, 2016

View reviewed changes

jimfoltz added 2 commits December 22, 2016 17:08

use face normal rather than vertex normal.

6638c9f

comments

3de7483

thomthom requested changes Dec 23, 2016

View reviewed changes

Removed comments.

05057c3

No longer need vertex normals from mesh. Write "attribute byte count" spacer as 16-bit unsigned int, little-endian

thomthom approved these changes Dec 23, 2016

View reviewed changes

thomthom merged commit fefa909 into SketchUp:master Dec 23, 2016

jimfoltz deleted the hotfix/binary_normals branch December 23, 2016 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hotfix/binary normals #164

Hotfix/binary normals #164

jimfoltz commented Dec 22, 2016

thomthom left a comment

thomthom Dec 22, 2016

jimfoltz commented Dec 22, 2016

jimfoltz commented Dec 22, 2016

thomthom commented Dec 22, 2016

thomthom commented Dec 22, 2016

thomthom left a comment

thomthom Dec 23, 2016

thomthom Dec 23, 2016

thomthom Dec 23, 2016

jimfoltz Dec 23, 2016 •

edited

Loading

thomthom Dec 23, 2016

jimfoltz Dec 23, 2016

jimfoltz Dec 23, 2016

thomthom left a comment

Hotfix/binary normals #164

Hotfix/binary normals #164

Conversation

jimfoltz commented Dec 22, 2016

thomthom left a comment

Choose a reason for hiding this comment

thomthom Dec 22, 2016

Choose a reason for hiding this comment

jimfoltz commented Dec 22, 2016

jimfoltz commented Dec 22, 2016

thomthom commented Dec 22, 2016

thomthom commented Dec 22, 2016

thomthom left a comment

Choose a reason for hiding this comment

thomthom Dec 23, 2016

Choose a reason for hiding this comment

thomthom Dec 23, 2016

Choose a reason for hiding this comment

thomthom Dec 23, 2016

Choose a reason for hiding this comment

jimfoltz Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

thomthom Dec 23, 2016

Choose a reason for hiding this comment

jimfoltz Dec 23, 2016

Choose a reason for hiding this comment

jimfoltz Dec 23, 2016

Choose a reason for hiding this comment

thomthom left a comment

Choose a reason for hiding this comment

jimfoltz Dec 23, 2016 •

edited

Loading