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

Instanced 3D Model spec #34

Merged
merged 14 commits into from
Nov 8, 2015
Merged

Instanced 3D Model spec #34

merged 14 commits into from
Nov 8, 2015

Conversation

pjcozzi
Copy link
Contributor

@pjcozzi pjcozzi commented Nov 6, 2015

Fixes #23

@slchow please copyedit. The new writing is in TileFormats/Instanced3DModel/README.md. Note that most of this PR is the same changes as #31.

@lilleyse please do a technical review. Does the Cesium client handle the corner cases for when, for example, instancesLength or batchTableLength is zero?

@lilleyse merge #31 first.

Before we declare 1.0, there's a few things to address in #33

@lilleyse
Copy link
Contributor

lilleyse commented Nov 6, 2015

  • Right now I'm using uint16 for batchId, but we could go with uint32 as well.
  • Should gltfLength and batchTableLength be rename with byteLength?
  • I tested the corner cases, and they work correctly.


The `instances` field immediately follows the `glTF` field (which may be omitted when `header.glTFLength` is `0`).

The `instances` field contains `header.instancesLength` tightly packed instances. Each instance has three fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a word missing here? I couldn't make sense of "contains header.instancesLength tightly packed instances"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A word is not missing. That should read as "the instances field contains x tightly packed instances" where the value of "x" is defined by header.instancesLength.

If you think we could make it more clear, please have at it.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 7, 2015

Right now I'm using uint16 for batchId, but we could go with uint32 as well.

uint16 is OK. That allows 64K per tile. That ought to be enough for anybody. :)

If experience proves otherwise, we can make this unit16 or uint32 depending on the value of header.batchTableLength or just uint32 if compression make the size difference not matter.

Should gltfLength and batchTableLength be rename with byteLength?

Yes, I will make the rename for b3dm and i3dm in this branch.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 7, 2015

If experience proves otherwise, we can make this unit16 or uint32 depending on the value of header.batchTableLength or just uint32 if compression make the size difference not matter.

Sorry, I mean the length of the arrays in the batch table, not the size of the JSON string in bytes.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 7, 2015

Should gltfLength and batchTableLength be rename with byteLength?

Also, please make these same renames throughout the Cesium implementation.

glTFLength -> gltfByteLength

glTFFormat -> gltfFormat

batchTableLength -> batchTableByteLength

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 7, 2015

@lilleyse this is updated. I'll do the b3dm batchTableByteLength rename in the composite branch, which has the other b3dm changes, now.

@slchow are you still copyediting?

@slchow
Copy link
Contributor

slchow commented Nov 7, 2015

@pjcozzi I'm done.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 8, 2015

Thanks @slchow!

@lilleyse merge when ready.

@lilleyse
Copy link
Contributor

lilleyse commented Nov 8, 2015

The layout image still has batchId as uint32, Other than that, it looks good to me.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Nov 8, 2015

Good catch, @lilleyse. Updated.

lilleyse added a commit that referenced this pull request Nov 8, 2015
@lilleyse lilleyse merged commit ed16038 into master Nov 8, 2015
@lilleyse
Copy link
Contributor

lilleyse commented Nov 8, 2015

Merged.

@pjcozzi pjcozzi deleted the i3dm branch December 5, 2015 21:45
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