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

Model Loading Performance Improvement #4628

Closed
sebjf opened this issue Dec 1, 2023 · 6 comments · Fixed by #4629
Closed

Model Loading Performance Improvement #4628

sebjf opened this issue Dec 1, 2023 · 6 comments · Fixed by #4629

Comments

@sebjf
Copy link
Contributor

sebjf commented Dec 1, 2023

Description

This issue is for the .io changes to improve loading speed.

Product Team Issue: https://github.com/3drepo/3D-Repo-Product-Team/issues/346

Tasks and Goals to follow as investigation proceeds.

Goals

Tasks

Related Issues

Bouncer: 3drepo/3drepobouncer#654
Unity: https://github.com/3drepo/3drepounity/issues/493

@sebjf sebjf added the feature label Dec 1, 2023
@sebjf sebjf self-assigned this Dec 1, 2023
sebjf added a commit that referenced this issue Dec 1, 2023
sebjf added a commit that referenced this issue Dec 1, 2023
…-gzipped. This is for testing and needs to be updated to depend on db.
Charence added a commit that referenced this issue Dec 2, 2023
@sebjf
Copy link
Contributor Author

sebjf commented Dec 6, 2023

Hi @carmenfan,

Regarding getting the encoding in the db to responseCodes.js, do you want me to have a go at it or do you want to do it?

I was thinking we'd add a member to the ref node to store the encoding, and return the Buffer inside another object which contained it, so responseCodes could set the right header.
It seems that _fetchFile was originally able to do return a metadata object anyway, its just not used anymore.

Also, it looks like the Unity Editor doesn't accept gzip encodings so we could add a small snippet that pipes gzipped responses through a decompressor if the request doesn't support such encodings, which accounts for the unlikely edge case we discussed at the start anyway.

@carmenfan
Copy link
Member

carmenfan commented Dec 6, 2023

@sebjf

Regarding getting the encoding in the db to responseCodes.js, do you want me to have a go at it or do you want to do it?

I would prefer if you take a stab at it first as I'm a bit preoccupied with some other work 😆

I was thinking we'd add a member to the ref node to store the encoding, and return the Buffer inside another object which contained it, so responseCodes could set the right header.

for the purpose of the asset bundles, you just need to set the header to the right mime type right (if it's already compressed) - that should bypass the compression right? but yes adding a metadata on the ref bson is the right approach i think.

It seems that _fetchFile was originally able to do return a metadata object anyway, its just not used anymore.

We've moved all file servicing to v5 iirc. And it would be preferred if we make changes to use/edit that as it has good test coverage thus may more reliable

Also, it looks like the Unity Editor doesn't accept gzip encodings so we could add a small snippet that pipes gzipped responses through a decompressor if the request doesn't support such encodings, which accounts for the unlikely edge case we discussed at the start anyway.

So this is for situations where you want to load the model in unity editor?

@sebjf
Copy link
Contributor Author

sebjf commented Dec 6, 2023

or the purpose of the asset bundles, you just need to set the header to the right mime type right (if it's already compressed)

That's right, really the ref node member is needed only so we don't have to rebuild all the Unity bundles!

So this is for situations where you want to load the model in unity editor?

Yes - which I am aware is basically just me during development now! 😆 If it were hard I would come up with another solution but I am expecting it be trivial in Node, and supports other hypothetical non-gzip supporting users of the endpoint too as a side-effect.

@carmenfan
Copy link
Member

or the purpose of the asset bundles, you just need to set the header to the right mime type right (if it's already compressed)

That's right, really the ref node member is needed only so we don't have to rebuild all the Unity bundles!

So this is for situations where you want to load the model in unity editor?

Yes - which I am aware is basically just me during development now! 😆 If it were hard I would come up with another solution but I am expecting it be trivial in Node, and supports other hypothetical non-gzip supporting users of the endpoint too as a side-effect.

@sebjf fair enough - if it's easy we may as well!

sebjf added a commit that referenced this issue Dec 6, 2023
…provided by the ref node in the database. Updated unity asset bundles route to use the new FilesManager to return the bundle.
@sebjf
Copy link
Contributor Author

sebjf commented Dec 6, 2023

Hi @carmenfan,

Here is how I have implemented it,

https://github.com/3drepo/3drepo.io/pull/4629/files
https://github.com/3drepo/3drepobouncer/pull/656/files

On the bouncer side it uses the metadata node parameter, which is now stored in the web buffers type all the way from where the buffer is created. The members of this node are appended to the ref node, so in mongo it looks like...

image

On .io the method to return the ref node parameters like size and type picks this up as well, and the handler for the asset bundle route forwards it to the response method, where it is used to set the headers or decode the stream, depending on its existence, state and whether the client agent supports it.

I have used the V5 versions of everything, but have left getUnityBundle member in the unityAssets.js of v4.

@carmenfan
Copy link
Member

Hi @carmenfan,

Here is how I have implemented it,

https://github.com/3drepo/3drepo.io/pull/4629/files https://github.com/3drepo/3drepobouncer/pull/656/files

On the bouncer side it uses the metadata node parameter, which is now stored in the web buffers type all the way from where the buffer is created. The members of this node are appended to the ref node, so in mongo it looks like...

image

On .io the method to return the ref node parameters like size and type picks this up as well, and the handler for the asset bundle route forwards it to the response method, where it is used to set the headers or decode the stream, depending on its existence, state and whether the client agent supports it.

I have used the V5 versions of everything, but have left getUnityBundle member in the unityAssets.js of v4.

@sebjf Thanks! had a quick look, the approach looks sane to me. 2 quick comments:

For this one - if you get the version from fs/promises (we already include unlink from that up top) you won't have to manually wrap it around with promises

For this one - if I'm reading it correctly you're now using the v5 responder thus this is N/A?

sebjf added a commit that referenced this issue Dec 7, 2023
sebjf added a commit that referenced this issue Dec 7, 2023
sebjf added a commit that referenced this issue Dec 8, 2023
…instead of enumerating Scene nodes in assetsMeta to improve performance on large models
carmenfan added a commit that referenced this issue Dec 13, 2023
carmenfan added a commit that referenced this issue Dec 15, 2023
carmenfan added a commit that referenced this issue Dec 15, 2023
@carmenfan carmenfan added this to the 5.7.0 milestone Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants