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

I3dm updates #4101

Merged
merged 5 commits into from
Aug 22, 2016
Merged

I3dm updates #4101

merged 5 commits into from
Aug 22, 2016

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jul 8, 2016

This is the first set of spec migrations from #33.

The i3dm header now has 6 double-precision fields for translating and scaling quantized coordinates.

Each instance is now composed of x, y, z 16-bit quantized coordinates along with 2 oct-encoded vectors that form an orthonormal basis for rotation.

Sample models updated to demonstrate all of these features.

@pjcozzi, @lilleyse, can you take a look at this when you get a chance?

Tasklist:

  • Integrate any changes from the finalized #100
  • Regenerate composite tiles (I want to make sure that nothing else in #100 changes before I do this.)

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 8, 2016

Sounds awesome, thanks @lasalvavida!

@lilleyse can you please do the first review?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 8, 2016

For CesiumGS/3d-tiles#33

@@ -192,15 +198,41 @@ define([
var batchTableByteLength = view.getUint32(byteOffset, true);
byteOffset += sizeOfUint32;

//>>includeStart('debug', pragmas.debug);
var gltfByteLength = view.getUint32(byteOffset, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be outside the pragma.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 8, 2016

You'll need to update the composite tilesets as well.

@lilleyse
Copy link
Contributor

lilleyse commented Jul 8, 2016

That's all from me, I like the new instanced tileset!

@lilleyse
Copy link
Contributor

lilleyse commented Jul 8, 2016

Just a note here... it also looks like another 3d tiles test is failing:
replacement refinement - selects root when sse is not met and subtree is not refinable (3)

But it's unrelated to this PR.

@lasalvavida
Copy link
Contributor Author

Updated @lilleyse, I'm still working on the composite tilesets

AttributeCompression.octDecode(normalTwoX, normalTwoY, normalRight);

// Compute third normal
Cartesian3.cross(normalRight, normalUp, normalOut);
Copy link
Contributor

Choose a reason for hiding this comment

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

"Out" here is confusing since, if I am following correctly, we would usually call it "Up" here, e.g., east/north/up; however, "Up" is already used, and perhaps rightfully so.

I don't have a better idea now; will let you know if I come up with something.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 10, 2016

@lasalvavida very nice start here. This still needs:

  • Using TILES3D_SCALE and TILES3D_NON_UNIFORM_SCALE semantics in the batch table when available. Instanced 3D Model updates 3d-tiles#33 (comment)
  • Updating these semantics at runtime (e.g., with Cesium3DTileFeature) - or explicitly not allowing it. They do not need to be stored in the batch table textures since they are handled by the instancing code. @lilleyse can advise. Let me know if you need my input.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 10, 2016

@lasalvavida perhaps add a tasklist to the top of this PR with #4101 (comment), composite, and anything else that still needs to be done before merging.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jul 18, 2016

@pjcozzi, @lilleyse Can you take a look at the implementation changes from CesiumGS/3d-tiles#100?

Particularly the Cesium3DTileFeatureTableResources. The way I have it set up is that values can be provided in the JSON, or be referenced into the binary if byteOffset is present. Let me know what you think, and if we like that approach I'll roll it into the batchTableResources as well.

Cartesian3.fromRadians(longitude, latitude, height, ellipsoid, position);
var modelMatrix = Transforms.eastNorthUpToFixedFrame(position);
var featureTableResources = new Cesium3DTileFeatureTableResources(featureTableJSON, featureTableBinary);
this.featureTableResources = featureTableResources;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this.featureTableResources is needed? Can't the feature table just be created at load time for parsing into the proper data structures, e.g., creating Cesium primitives, and then discarded? This is a very heavy object and the memory consumption is not palatable to keep it for the lifetime of a tile.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 18, 2016

We might want to unit test Cesium3DTileFeatureTableResources separately. I'll let you know after the next round of edits.

/**
* @private
*/
function Cesium3DTileFeatureTableResources(featureTableJSON, featureTableBinary) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the approach of generating a JS array that holds onto the property's binary data just seems too expensive. I think it would be easier if this class stored a TypedArray view for each property, and then when getting an instance's property it just indexes into the typed array. To avoid allocations, the getter can take an optional results array parameter.

This is also make it easier to work with Points3DTileContent which wants TypedArrays for the color and position properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see this comment: #4101 (comment) kind of alludes to that too.

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 we write unit tests that test this file in isolation without an entire tile?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jul 19, 2016

I went ahead and pushed some of these changes. Feel free to comment. I'm finishing up rewriting my tile generation code for the new spec, so we should have tests again shortly.

return arrayBuffer;
};

Cesium3DTileFeatureTableResources.prototype.getGlobalProperty = function(semantic, componentType, count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a global property ever need to be stored in binary? If not, this function could just return the JSON.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not; I think that's probably better for making the implementation more streamlined

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should assume it will always be JSON. A global property could be a big array, even if it isn't per-feature data, so we shouldn't make the spec this limited unless there is a significant implementation reason, which I do not see here since we already need to handle binary for the other properties.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

@lilleyse let me know when this is ready for me to review.

@lasalvavida
Copy link
Contributor Author

Updated with some test models for semantics. Once I have #4121, I'll add a test model for oct-encoding, and a test model with all semantics and update the tests.

var feature = featureTable.getProperty('TEST', 1, ComponentDatatype.UNSIGNED_BYTE, 2);
expect(feature).toEqual([2, 3]);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

For this file also test that the DeveloperErrors are hit.

@lasalvavida
Copy link
Contributor Author

Updated. The composite tile samples have been updated as well. Once there's a verdict on the FeatureTable array issue, this should be ready to go.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

@lasalvavida please merge in 3d-tiles.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

Does the Sandcastle example cover most i3dm use cases? If not, we can add another one or two small tilesets like we did in #4183? We don't need exhaustive testing in the Sandcastle example, but it is good to cover command use cases for manual testing.

@lasalvavida
Copy link
Contributor Author

Does the Sandcastle example cover most i3dm use cases?

The Sandcastle example only does basic instancing, but I do have test models in specs for all semantics, so it would be trivial to add one or two of those.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

I have the following test failures:

Scene/Composite3DTileContent rejects readyPromise on error
DeveloperError: glTF byte length is zero, i3dm must have a glTF to instance.

Scene/Composite3DTileContent renders composite
Expected undefined to be defined.

Scene/Composite3DTileContent renders composite of composite
Expected undefined to be defined.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

Does the Sandcastle example cover most i3dm use cases?
The Sandcastle example only does basic instancing, but I do have test models in specs for all semantics, so it would be trivial to add one or two of those.

Yes, please.

Think representative, not exhaustive.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

There are a lot of .i3dm files in this PR that are 100-400 KB. In the future, please please please focus on creating very small test files so we do not bloat the repo and we keep the tests really fast. For example, if a dataset for a test has an embedded texture, resize it to be small or if a tile has 16K instances change it to be 32 instances.

Short-term, is it possible to change the git history in this branch and use smaller i3dm files?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 15, 2016

Just those comments. Overall nice work here.

@lasalvavida
Copy link
Contributor Author

Short-term, is it possible to change the git history in this branch and use smaller i3dm files?

Yup, no problem.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

What's left before this can be merged?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Aug 19, 2016

I have all of my sample tilesets regenerated except for gltfExternal. I will have this updated shortly.
*edited

@lasalvavida
Copy link
Contributor Author

Okay, tests should be passing now; and this should be ready for final review. As far as not bloating the git history, I think if you git merge --squash when you merge this, it will flatten out the history.

@mramato
Copy link
Contributor

mramato commented Aug 19, 2016

@lasalvavida you can squash yourself very easily. (assuming origin is the name of the official cesium remote)

  1. Merge in origin/3d-tiles into i3dm-updates
  2. Run git reset origin/3d-tiles (This will make your branch identical to 3d-tiles but with all of your changes edited locally.)
  3. Commit everything providing a good overall description.
  4. run git push --force to override the remote branch with your new squashed branch.

Let me know if you want me to stop by and walk you through it.

@lasalvavida
Copy link
Contributor Author

Thanks @mramato. This is all squashed and ready for review.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 19, 2016

@lasalvavida @lilleyse is this ready to merge?

@lilleyse
Copy link
Contributor

@lasalvavida I think you forgot to add Cesium3DTileFeatureTableResources back after squashing.

@lasalvavida
Copy link
Contributor Author

Missing files re-added this should be ready for a look for real this time.

@lilleyse
Copy link
Contributor

One minor suggestion - can you place the instanced models fully above the ground? The shadows look wrong at the moment.

Other that that, everything looks good.

@lasalvavida
Copy link
Contributor Author

Updated, sample tiles are now on the ground.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 22, 2016

@lilleyse please merge when ready.

@lasalvavida nice work here!

@lilleyse lilleyse merged commit 33b05c2 into CesiumGS:3d-tiles Aug 22, 2016
@lilleyse lilleyse deleted the i3dm-updates branch August 22, 2016 17:37
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.

4 participants