Support zipped versions of gltf and glb files

[sponce]

After I've discovered how compression was giving a factor 8-10 on glb files, I've implemented direct usage of zip files in phoenix.
And applied it to LHCb description, which went from 7MB to 0.3MB ! I admit I've also cleaned up the geometry, but this was only a factor 2.

For info, I've put some documentation concerning optimization of gltf/glb files in the readme of the root_cern-To_gltf-Exporter : https://github.com/HSF/root_cern-To_gltf-Exporter, at the bottom. May be of interest

[EdwardMoyse]

I'm kind of amazed that zipping glb leads to savings! Very interesting.

[sponce]

Yes, I was surprised too. But actually the 'b' is only for meshes. Otherwise, you get the plain text of the gltf. And even the meshes are not compressed, just not stored as base64.

Otherwise do you get why the checks ar efailing :

STDERR node-pre-gyp info it worked if it ends with ok

This is a bit cryptic to me

[EdwardMoyse]

Compiling locally I see:

- Generating browser application bundles (phase: setup)...
src/event-display.ts(336,5): error TS2588: Cannot assign to 'name' because it is a constant.
src/managers/three-manager/import-manager.ts(238,33): error TS2314: Generic type 'Promise<T>' requires 1 type argument(s).
src/managers/three-manager/import-manager.ts(259,20): error TS2339: Property 'eventDisplay' does not exist on type 'ImportManager'.
src/managers/three-manager/import-manager.ts(414,46): error TS2663: Cannot find name 'parseGLTFGeometryFromArrayBuffer'. Did you mean the instance member 'this.parseGLTFGeometryFromArrayBuffer'?
src/managers/three-manager/index.ts(839,32): error TS2341: Property 'parseGLTFGeometry' is private and only accessible within class 'ImportManager'.
src/managers/three-manager/index.ts(839,50): error TS2345: Argument of type 'File' is not assignable to parameter of type 'string'.
9:11:00 AM - Found 6 errors. Watching for file changes.
✔ Browser application bundle generation complete.

[sponce]

Sounds like my lack of knowledge in javascript/typescript. Do we need a "var" keyword in front of that line ? I would guess so.
I've now done it and pushed. Still works for me and the commit hooks did convert my "var" into a "const"... weird for fixing a "Cannot assign to 'name' because it is a constant" I must say. But as I mentioned, I'm not at all expert in this

[EdwardMoyse]

Hang on - I think I have fixes for this, and the other issues (though tests are still failing)

[EdwardMoyse]

Sounds like my lack of knowledge in javascript/typescript. Do we need a "var" keyword in front of that line ? I would guess so. I've now done it and pushed. Still works for me and the commit hooks did convert my "var" into a "const"... weird for fixing a "Cannot assign to 'name' because it is a constant" I must say. But as I mentioned, I'm not at all expert in this

Honestly, I'm not sure where the const name came from, so I made a new name. As you can see from my commits, there were several other issues though, some of which I cannot understand why you wouldn't see, since many are just pure errors. For example this:

src/managers/three-manager/import-manager.ts(259,20): error TS2339: Property 'eventDisplay' does not exist on type 'ImportManager'.

cannot be something depending on the local setup (and I could only comment out that line to get around it). Are you sure you don't see this in your log somewhere?

BTW please look at my fixes - I'm really not sure if they are correct, just that they got rid of the compilation failures.

[EdwardMoyse]

Ah pants - I must have messed up the conflict resolution. :-(

[EdwardMoyse]

@sponce I think I will need to rebase to fix this. Is that okay? You will need to force pull afterwards if so.

[sponce]

No problem. Go on.
Note, I can see the errors now, when doing a yarn start, but they are hidden from the view by the rest of the log, and still things are working. I'm more and more puzzled.
Anyway, feel free to modify !

[EdwardMoyse]

Okay, compilation problems should be "fixed" (as I said, I'm not sure I handled these correctly, but hopefully my changes at least help).

Now I'm trying to understand:

      Summary of all failing tests
       FAIL  projects/phoenix-ui-components/lib/components/ui-menu/io-options/io-options-dialog/io-options-dialog.component.test.ts (40.375 s)
        ● IoOptionsDialogComponent › handleFileInput › handleFileInput sync › should handle glTF file input

[EdwardMoyse]

Oh man - I'm so confused by this error!

I can see that handleGLTFInput(files: FileList) { gets called, but it seems like parseGLTFGeometry is not? I don't understand why, and I find debugging this jest stuff so problematic! But I'll keep trying.

[EdwardMoyse]

Okay, I think I understood it (finally!) At least I understand jest a little better too...

BTW @sponce, you might want to check that it still works correctly after this change? I think it should, but just in case...

And I looked at the documentation you added - nice! We have a similar (but less detailed) version in Phoenix: https://github.com/HSF/phoenix/blob/main/guides/developers/geometry-tips.md#converting-gltf-to-glb.

[EdwardMoyse]

Okay, I think I understood it (finally!) At least I understand jest a little better too...

BTW @sponce, you might want to check that it still works correctly after this change? I think it should, but just in case...

And I looked at the documentation you added - nice! We have a similar (but less detailed) version in Phoenix: https://github.com/HSF/phoenix/blob/main/guides/developers/geometry-tips.md#converting-gltf-to-glb.

@sponce if you confirm that the functionality still works, I will merge this.

Edit: just to say, I looked at the LHCb demo of course and it all looks fine.

I tried with the ATLAS barrel and this also worked, and went from 12Mb to 1.3Mb (!!!)

[sponce]

@EdwardMoyse thanks a lot for the debugging ! I've looked at the diffs and they look fine. Except I do not really understand why they are needed (especially last one). Anyway, if it works I'm happy. But... locally it fails with :

 TypeError: file.name is undefined
    parseGLTFGeometry http://localhost:4200/main.js:7795

So I suspect it one more iteration of not having the same version of underlying libraries or something of the like. I've tried to update using yarn install but it fails with "Package pixman-1 was not found in the pkg-config search path", which confuses me completely, as yarn start is precisely supposed to install whatever is needed.

Bottom line : just merge, I'll recreate an environment from scratch as I already needed to do very regularly for phoenix. Somehow yarn is not very good at keeping things consistent in the long term.

[EdwardMoyse]

@EdwardMoyse thanks a lot for the debugging ! I've looked at the diffs and they look fine. Except I do not really understand why they are needed (especially last one).

The problem was this (in o-options-dialog.component.test.ts:

      afterEach(() => {
        expect(component.handleFileInput).toHaveBeenCalled();
      });

If you don't call handleFileInput at some point, it fails.

Anyway, if it works I'm happy. But... locally it fails with :

 TypeError: file.name is undefined
    parseGLTFGeometry http://localhost:4200/main.js:7795

So I suspect it one more iteration of not having the same version of underlying libraries or something of the like. I've tried to update using yarn install but it fails with "Package pixman-1 was not found in the pkg-config search path", which confuses me completely, as yarn start is precisely supposed to install whatever is needed.

Bottom line : just merge, I'll recreate an environment from scratch as I already needed to do very regularly for phoenix. Somehow yarn is not very good at keeping things consistent in the long term.

I regularly have to do rm -rf node_modules and yarn install.

[sponce]

I've now managed to restart my local server. It took quite some iterations on installing various dev libraries I obviously did not need so far (starting with libpixman-dev), then I dropped node_modules and rebuilt everything. However, I still have the same error file.name is undefined when trying to load a gltf or glb file from the import/export icon.
Did you try it ? Take care that it takes a slightly different path than the auto loading, so I fear you last change did break that

[EdwardMoyse]

Yikes. Okay, I see this now. Will revert in a new MR.

[sponce]

The problem is that in case we load via the icon, we do not have a File object. So we pass the content of the file as a string. And this is handled downstream. I will have to leave this for now, but I can try to figure it out later today. Maybe we do not have to revert then, we only fix

[EdwardMoyse]

The problem is that in case we load via the icon, we do not have a File object. So we pass the content of the file as a string. And this is handled downstream. I will have to leave this for now, but I can try to figure it out later today. Maybe we do not have to revert then, we only fix

Okay, well for the moment I will revert (as I don't want Phoenix broken l in the meantime).