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

Revisit Draco3d downloads from gstatic #9787

Closed
ebogo1 opened this issue Sep 2, 2021 · 2 comments
Closed

Revisit Draco3d downloads from gstatic #9787

ebogo1 opened this issue Sep 2, 2021 · 2 comments

Comments

@ebogo1
Copy link
Contributor

ebogo1 commented Sep 2, 2021

This is a follow-up to #9778, which just got merged in time for the 1.85 release. Instead of manually downloading .wasm and wrapper files from the Draco repo, CesiumJS now fetches them from gstatic during install time with a prepare script in package.json. There are several loose ends from these changes:

  • Using version 1.3.5 instead of the latest (1.4.1 at the time of writing): As of 1.3.6, it seems the Draco decoder does not work with our decodeDraco worker. I suspect this is due to our usage of SkipAttributeTransform:
    // Skip all parameter types except generic
    var attributesToSkip = ["POSITION", "NORMAL", "COLOR", "TEX_COORD"];
    if (parameters.dequantizeInShader) {
    for (var i = 0; i < attributesToSkip.length; ++i) {
    dracoDecoder.SkipAttributeTransform(draco[attributesToSkip[i]]);
    }
    }
    When these lines are commented out, newer Draco versions work fine but the attributes are returned as Float32 arrays instead of Uint16 arrays. The action item here is to do some digging into the SkipAttributeTransform source code and try to make an example demonstrating a bug if there is one (and to open an issue in the Draco3d repo).
  • Using the gstatic files instead of the npm package: I tried two things with the npm module:
    • Point the Draco TaskProcessor directly to the .wasm and wrapper files in the npm package dist: This didn't work due to a runtime error, possibly related to the first bullet point but I'm not sure.
    • Compile the .wasm module from the npm package using the decoder API: Similarly to the other libraries we pull from npm, I imported the Decoder from the Draco module. Then, following the example in the Draco repo, modified the decodeDraco(event) function in our Draco worker to load the module using the createDecoderModule API. I think the changes in the worker were correct, but I was unable to test because I ran into build errors due to a dependency on fs and path inside the Decoder from npm when building the files in Source/ThirdParty/.
  • Updating the gulp "prepare" task: With the inclusion of a "prepare" script in package.json, we needed to remove the script from the package.json that gets bundled into our zip file. Since we rushed to get the Draco download changes into 1.85, we might've missed a cleaner way to do this; ideally, we wouldn't need to write out a temporary package.noprepare.json file during the "makeZipFile" task and delete it immediately after.
@ebogo1 ebogo1 added the cleanup label Sep 2, 2021
@lilleyse
Copy link
Contributor

lilleyse commented Sep 2, 2021

The action item here is to do some digging into the SkipAttributeTransform source code and try to make an example demonstrating a bug if there is one (and to open an issue in the Draco3d repo).

👍 the sooner the better, while it's on our minds

Point the Draco TaskProcessor directly to the .wasm and wrapper files in the npm package dist

It would be good to get to the bottom of this as well. I was under the impression that draco_decoder_nodejs.js in the npm package and draco_wasm_wrapper.js on gstatic were different, but they're actually identical. It seems like a pure npm solution is maybe possible here.

@ebogo1
Copy link
Contributor Author

ebogo1 commented Sep 23, 2021

The action item here is to do some digging into the SkipAttributeTransform source code and try to make an example demonstrating a bug if there is one (and to open an issue in the Draco3d repo).

Opened google/draco#742.

@ebogo1 ebogo1 closed this as completed Oct 15, 2021
@ebogo1 ebogo1 moved this from Next priority to Issue/PR closed in CesiumJS Issue/PR backlog Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

No branches or pull requests

2 participants