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

Move LHCb to use Draco and LHCb.glb #530

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Move LHCb to use Draco and LHCb.glb #530

merged 4 commits into from
Jan 11, 2023

Conversation

EdwardMoyse
Copy link
Collaborator

This migrates LHCb to compressed glb (generated with: gltf-pipeline -i LHCb.gltf -o LHCb.glb -d)

This reduces the size of the geometry from 19Mb to 7.2Mb

To uncompress the geometry, we need to add the DRACOLoader (see the discussion in #529)

In my local tests this doesn't really improve anything. However I would expect it might running remotely... however I need to test this (hopefully I can use the deployment here to do so).

cc @sponce

@EdwardMoyse
Copy link
Collaborator Author

EdwardMoyse commented Dec 20, 2022

Okay, comparing surge ( http://phoenix-pr-530.surge.sh/#/lhcb) vs normal (http://hepsoftwarefoundation.org/phoenix/#/lhcb) I see:

Surge:  4.5s, 4.3s, 3.8s
Normal: 3.6s, 4.6s, 4.3s.

So, I don't think it makes it any faster (at least on my very quick connection), but this means the overhead of decompressing isn't too bad (at least on my M1 Pro mac)... if my connection was slower, I can imagine it would help. So I'd be inclined to accept it.

image

@9inpachi
Copy link
Collaborator

9inpachi commented Dec 23, 2022

You can check how to throttle network using Chrome DevTools and see how long it takes for the geometry to load on a slow network. I tried on the surge URL for PR but the application is throwing errors on slow network. The deployed version works fine.

@sponce
Copy link
Collaborator

sponce commented Jan 5, 2023

I confirm I see surge slightly slower on the CERN fast connection. No big news here and I agree we could merge. One thing though, don't you want to drop the old file ? Otherwise the gltf and the glb will quickly become out of sync. And the other missing bit is maybe the recipe for creating the glb form the gltf, for future updates

@bencouturier
Copy link

I agree it's best to keep only one and that we should document the conversion! Looks very useful for slow connections, many thanks.

@EdwardMoyse
Copy link
Collaborator Author

Hi @sponce and @bencouturier - I removed the gltffile now (I didn't want to do it without your permission) and added some new geometry documentation (feel free to add to it!)

@github-actions github-actions bot temporarily deployed to pull-request January 9, 2023 10:03 Inactive
@github-actions github-actions bot temporarily deployed to pull-request January 9, 2023 10:05 Inactive
@github-actions github-actions bot temporarily deployed to pull-request January 10, 2023 09:50 Inactive
@sponce sponce merged commit d42f25e into main Jan 11, 2023

const dracoLoader = new DRACOLoader();
dracoLoader.setDecoderPath(
`https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/js/libs/draco/`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get now a failure in the LHCb deployment that this path gives a 404. In our case REVISION happens to be 148.0 and indeed the path is wrong for that version. "js/libs" should be replaced with "jsm".
@EdwardMoyse Do you understand why and why it does not fail for you ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just checked and your path is correct for 146... It seems things have changed on the threejs side !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right - updating threejs version is always a bit of work. Since phoenix is on 0.147.1 I would definitely not recommend moving your local deployment to something ahead of that. If you need it for some reason, we can prioritise updating phoenix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My issue is that we did not intend to move our local deployment. I have to check why we do not have the default 147. @bencouturier any idea ?

@9inpachi 9inpachi deleted the master--draco branch May 10, 2023 20:53
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