-
Notifications
You must be signed in to change notification settings - Fork 464
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
I3dm updates #100
I3dm updates #100
Conversation
@lasalvavida did you draw all the figures? If not, what is their terms of use? |
**Figure 1**: Instanced 3D Model layout (dashes indicate optional fields). | ||
|
||
![](figures/layout.png) | ||
![header layout](figures/header-layout.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Cesium implementation span
is called scale
. What is the standard terminology for quantization? Let's use it in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment with origin
and translation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to whatever you think is best as far as naming goes.
span
is a little different from scale
in quantization. In quantization since we store everything in a decode matrix scale
is stored as span
divided by 2^16-1
. However, here we can choose not do that and potentially improve precision which is what I am doing currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it sounds like scale
is the right term here. Is origin
the standard quantization term used in the literature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the literature, origin
should be offset
and span
should be scale
. I'll make those changes.
Add a Resources section at the bottom that links to the papers for quantization and oct-endcoding. Also link to the Cesium helper functions implementation. |
Just like the Cesium implementation, this needs:
|
|
||
### X, Y, and Z for Translation | ||
|
||
`x`, `y`, and `z` are stored as `uint16` positions in the quantized instance region defined by the `origin` and `span` fields in the header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition here for "quantized instance region" should come earlier in the spec, and how it does/doesn't relate to the bounding volume should be explicit to avoid confusion; readers may think they are the same at first glance.
Also, "quantized volume" is a better name since instance is implicit in this context and the reader won't need to wonder if there is a difference between volume and region; it is the prefix, "bounding" or "quantized", that defines that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and quantized volume is a better name.
Good start here, @lasalvavida! |
Also:
|
Also, change the "Spec Status" for i3dm to "Solid base, only minor changes expected" See https://github.com/AnalyticalGraphicsInc/3d-tiles#spec-status |
Also explain how the transform (inverse transpose) affects normals. |
Wouldn't that be better suited for the tileset transform specification? Also this is updated and ready for another look. |
It should be mentioned here too; otherwise, someone writing a new tile loaded based on this spec may forget it. It can be short, just something like:
Later, that will link to the section in the spec that described how to compute the matrices using the tile and its ancestors. |
Is this ready? |
Yup. Everything should be addressed now. |
|
||
### Examples | ||
|
||
In these examples, the semantic values are shown as JSON arrays. This is done to make the examples more human readable, and is still a valid feature table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment from before still holds. Please revise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new approach with the code example is good (actually really good for those doing writers in Node.js). Update this paragraph since it is out-of-date.
The
Then under the table, include a link to the example section so readers could jump to common layouts. |
We need to define what happens if |
My issue with this is that something being required but with an optional alternative, ( I think applying a blanket "Sometimes, if" to both cases is confusing. If anything I think maybe:
|
Alright, updated, let me know what you think. |
This is pretty good, not sure that it is perfect, but let's go with it and see if we get any feedback. |
Just that one small update and this is ready to merge! |
Updated |
Nice! |
@pjcozzi these are the i3dm spec changes. Can you take a look over this and tell me what you think?