Update Model3DTileContent specs to have parity with 3D Tiles 1.0 formats #10633
Conversation
|
Thanks for the pull request @ptrgags!
Reviewers, don't forget to make sure that:
|
| @@ -13,6 +14,7 @@ import { | |||
| VertexAttributeSemantic, | |||
| } from "../../../Source/Cesium.js"; | |||
| import createScene from "../../createScene.js"; | |||
| import pollToPromise from "../../pollToPromise.js"; | |||
There was a problem hiding this comment.
Missing the "renders point cloud with tile transform" test from PointCloud3DTileContentSpec
There was a problem hiding this comment.
Is there a reason why this comment is in PntsLoaderSpec?
for the tile transform tests, we only wanted to have one set of them since it works the same for each content type. For this one I kept the b3dm ones since I was having difficulty getting the camera setup right for the glTF ones.
There was a problem hiding this comment.
Is there a reason why this comment is in
PntsLoaderSpec?
Because most of the tilesets tested in Point3DTileContentSpec were copied over to PntsLoaderSpec, except for the one with the tile transform. So if we don't duplicate it, the tileset associated with it will never get tested
There was a problem hiding this comment.
Ah I understand now. I added a test in PntsLoaderSpec, though it seems of limited use, since the transform is applied in the tileset.json, not the .pnts file itself.
There was a problem hiding this comment.
@ptrgags ah I see... let's just leave it there for now so we don't hold up the PR, we can move things around later.
| // only have one destroy test (this one) | ||
| it("destroys glTF content", function () { |
There was a problem hiding this comment.
FYI there's a "destroy pnts content" test, so either this comment or that test should be removed to be consistent
There was a problem hiding this comment.
And if we're keeping this comment, maybe reword this more formally...
| // only have one destroy test (this one) | |
| it("destroys glTF content", function () { | |
| // Since all content types are loaded as a Model3DTileContent, only one destroy test is necessary | |
| it("destroys", function () { |
... and move the test outside the glTF suite
There was a problem hiding this comment.
ah I forgot to put TODO in the comment so I missed a spot.
This comment was saying keep the glTF destroy test, but remove the pnts/b3dm/i3dm equivalents. I updated these.
|
@j9liu updated everything except for a couple questions I left in the comment threads |
|
@j9liu thanks for answering my questions, I updated the PR! |
|
@ptrgags looks good, merging now! |
This PR adds specs from
Batched3DModel3DTileContent,Instanced3DModel3DTileContentandPointCloud3DTileContenttoModel3DTileContent(and sometimes other places like the loaders)Some notes:
Model3DTileContent.getFeature()wasn't throwing exceptions like the interface expects, so I updated the code therexit()-ed the test and opened Support point cloud back face culling inModelExperimental#10632 so we don't have to hold up the staging branch.@j9liu could you review?