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

glTF 2.0 in samples generator #139

Merged
merged 24 commits into from
Aug 31, 2018
Merged

glTF 2.0 in samples generator #139

merged 24 commits into from
Aug 31, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Mar 21, 2018

Updated the samples generator project to use glTF 2.0.

The tilesets all load in Cesium and the embedded glTFs pass glTF Validator.

Not quite ready for merging yet because

  • Some samples are commented out. This includes anything that used features from gltf-pipeline#1.0 including texture compression, quantization, or oct encoding.
  • package.json uses gltf-pipeline 1.0.5 but requires gltf-pipeline 2.0.0 which isn't released yet.

After merge:

@lilleyse lilleyse force-pushed the 2.0-samples-generator branch 2 times, most recently from f4541b6 to 86d650f Compare July 29, 2018 02:06
@lilleyse
Copy link
Contributor Author

This is ready now.

Review the Cesium PR first - CesiumGS/cesium#6857. The review on this one isn't as urgent.

@ggetz
Copy link
Contributor

ggetz commented Jul 30, 2018

PointCloudDraco and PointCloudDracoBatched are still using url instead of uri

@lilleyse
Copy link
Contributor Author

PointCloudDraco and PointCloudDracoBatched are still using url instead of uri

I'll make sure to fix that in #150

@ggetz would you mind doing a quick review?

@ggetz
Copy link
Contributor

ggetz commented Aug 30, 2018

Thanks @lilleyse! A few comments.

Running the script gives me an error in Node 6.

Unhandled rejection TypeError: "list" argument must be an Array of Buffers
    at Function.Buffer.concat (buffer.js:314:13)
    at mergeBuffers (C:\workspace\3d-tiles-tools\samples-generator\node_modules\gltf-pip
eline\lib\mergeBuffers.js:46:31)
    at C:\workspace\3d-tiles-tools\samples-generator\node_modules\gltf-pipeline\lib\writ
eResources.js:65:13
    at tryCatcher (C:\workspace\3d-tiles-tools\samples-generator\node_modules\bluebird\j
s\release\util.js:16:23)
    at Promise._settlePromiseFromHandler (C:\workspace\3d-tiles-tools\samples-generator\
node_modules\bluebird\js\release\promise.js:512:31)
    at Promise._settlePromise (C:\workspace\3d-tiles-tools\samples-generator\node_module
s\bluebird\js\release\promise.js:569:18)
    at Promise._settlePromise0 (C:\workspace\3d-tiles-tools\samples-generator\node_modul
es\bluebird\js\release\promise.js:614:10)
    at Promise._settlePromises (C:\workspace\3d-tiles-tools\samples-generator\node_modul
es\bluebird\js\release\promise.js:693:18)
    at Promise._fulfill (C:\workspace\3d-tiles-tools\samples-generator\node_modules\blue
bird\js\release\promise.js:638:18)
    at PromiseArray._resolve (C:\workspace\3d-tiles-tools\samples-generator\node_modules
\bluebird\js\release\promise_array.js:126:19)
    at PromiseArray._promiseFulfilled (C:\workspace\3d-tiles-tools\samples-generator\nod
e_modules\bluebird\js\release\promise_array.js:144:14)
    at Promise._settlePromise (C:\workspace\3d-tiles-tools\samples-generator\node_module
s\bluebird\js\release\promise.js:574:26)
    at Promise._settlePromise0 (C:\workspace\3d-tiles-tools\samples-generator\node_modul
es\bluebird\js\release\promise.js:614:10)
    at Promise._settlePromises (C:\workspace\3d-tiles-tools\samples-generator\node_modul
es\bluebird\js\release\promise.js:693:18)
    at Async._drainQueue (C:\workspace\3d-tiles-tools\samples-generator\node_modules\blu
ebird\js\release\async.js:133:16)
    at Async._drainQueues (C:\workspace\3d-tiles-tools\samples-generator\node_modules\bl
uebird\js\release\async.js:143:10)

This no longer outputs Point Cloud Draco tiles at the same path, so related tests in Cesium are failing.

Also tilesets of tilesets test is failing:

× preserves query string with external tileset JSON file
        Expected 'http://localhost:9876/Data/Cesium3DTiles/Tilesets/TilesetOfTilesets/tileset2.json?a=1&b=boy' to equal 'http://localhost:9876/Data/Cesium3DTiles/Tilesets/TilesetOfTilesets/tileset2.json?v=1.2.3&a=1&b=boy'.
            at <Jasmine>
            at Specs/Scene/Cesium3DTilesetSpec.js:1306:79
            at Promise.then (Source/ThirdParty/when.js:196:34)
            at Source/ThirdParty/when.js:297:13
            at processQueue (Source/ThirdParty/when.js:647:4)

@lilleyse
Copy link
Contributor Author

Running the script gives me an error in Node 6.

I tried 6.14.4 and didn't get that error. What version are you using?

This no longer outputs Point Cloud Draco tiles at the same path, so related tests in Cesium are failing.

Those are part of #150

Also tilesets of tilesets test is failing:

That is fixed in #151

@ggetz
Copy link
Contributor

ggetz commented Aug 31, 2018

I tried 6.14.4 and didn't get that error. What version are you using?

Using 6.14.3

Those are part of #150
That is fixed in #151

What's the plan there, did we want to get those merge in before?

@lilleyse
Copy link
Contributor Author

#151 can be merged before, and then I can merge master into here.
#150 I still need to finish up but doesn't need to hold up this PR.

I switched to 6.14.3, cleared away node_modules, and didn't get any errors.

Add extras dictionary to tileset
@ggetz
Copy link
Contributor

ggetz commented Aug 31, 2018

Ok sounds good. Reviewed and merged #151.

And I also blew away node_modules, this fixed the error. Thanks!

@lilleyse
Copy link
Contributor Author

Merged in master - should be ready to go now.

@ggetz
Copy link
Contributor

ggetz commented Aug 31, 2018

👍

@ggetz ggetz merged commit 5c28737 into master Aug 31, 2018
@ggetz ggetz deleted the 2.0-samples-generator branch August 31, 2018 14:55
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