Skip to content
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

Pnts update #116

Merged
merged 15 commits into from
Aug 22, 2016
Merged

Pnts update #116

merged 15 commits into from
Aug 22, 2016

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Aug 18, 2016

Will follow this PR with another that includes per-point batchID and batch table.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Initial work for #22.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Same comment about the Required column that I had for i3dm.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Same comment about strong binary data for the feature table that I had for i3dm.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Same comment about transforms, #98, as I had for i3dm.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Made a few small tweaks. Check my commits.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

@lasalvavida do you want to give this a quick read?


### Examples

Note: these examples use JSON arrays for illustration purposes but for best performance per-point properties like `POSITIONS` and `RGB` should be stored in the `featureTableBinary`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. We need to be careful with this because, at a glance, people would criticize this as a naive approach with just JSON. Try separating each example below into JSON and a "binary" section.

Copy link
Contributor

@lasalvavida lasalvavida Aug 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we like it, maybe try what I put for i3dm. It makes this clarification and shows a json with the byteOffset attribute. I think it is a little verbose to do a separate one for every example.

edit: We don't like it, make one for each example.

@pjcozzi pjcozzi mentioned this pull request Aug 19, 2016
@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

Is this ready?

@lasalvavida
Copy link
Contributor

lasalvavida commented Aug 19, 2016

The same no, unless clause from the i3dm for QUANTIZED_VOLUME_OFFSET and QUANTIZED_VOLUME_SCALE should be applied here.

@lilleyse
Copy link
Contributor Author

Is this ready?

I'm waiting for #100 to be done. Then it should be ready soon after.

@lilleyse
Copy link
Contributor Author

Updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 20, 2016

For the Quantized Positions section, can you include the figures (and any additional wording if any) from the i3dm spec?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 20, 2016

Can we add an example with oct-encoded normals?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 20, 2016

Made some tweaks in 27bad0a.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 20, 2016

This is very close, thanks!

@@ -169,7 +169,7 @@ var featureTableBinary = new Buffer(new Float32Array([
#### Quantized Positions and Oct-Encoded Normals

In this example, the 4 instances will be placed with an orientation `up` of `[0.0, 1.0, 0.0]` and `right` of `[1.0, 0.0, 0.0]` in oct-encoded format
and they will be placed on the corners of a quantized volume that spans from `-250.0` to `250.0` units in the `x` and `z` directions.
and they will be placed on the corners of a quantized volume that spans from `-500.0` to `0.0` units in the `x` and `z` directions.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if these numbers were supposed to also take the offset into account, so I adjusted them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lasalvavida can you confirm this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this change is not correct. The offset puts the origin at (-250, 0, -250), then the scale makes the opposite corner -250 + 500 = 250 : (250, 0, 250).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah alright, I mistook the offset as the center. I'll need to update CesiumGS/cesium#4183 as well.

@lilleyse
Copy link
Contributor Author

The folder name is still called Points. Should we change this? It may cause some broken links.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 22, 2016

The folder name is still called Points. Should we change this? It may cause some broken links.

I noticed that too. Please rename and ensure that all links inside this repo are updated. I don't think there are many external deep links.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 22, 2016

Looks good, please merge after renaming.

@@ -187,21 +187,21 @@ var featureTableJSON = {
}
};

var positionQuantizedBinary = new Buffer(new UInt16Array([
var positionQuantizedBinary = new Buffer(new Uint16Array([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this!

@lilleyse
Copy link
Contributor Author

Updated.

@lasalvavida lasalvavida mentioned this pull request Aug 22, 2016
1 task
@pjcozzi pjcozzi merged commit d26851f into master Aug 22, 2016
@pjcozzi pjcozzi deleted the pnts-updates branch August 22, 2016 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants