search

Smithsonian / dpo-voyager

DPO Voyager - 3D Explorer and Tool Suite
Apache License 2.0
168 stars 30 forks source link

Cancellable models load #326

Open sdumetz opened 1 week ago

sdumetz commented 1 week ago

Currently if a model's derivative is no longer needed while still loading, the request still runs through.

This is a known limitation in three's FileLoader (see https://github.com/mrdoob/three.js/issues/20705) that will probably not get fixed anytime soon because it is easy to fix in user-space (3D-tiles, among others, does so) and a general-purpose one-size-fits-all solution is not trivial.

Fortunately in our case, it is relatively easy to implement.

this should have practically no effect unless you are switching quality levels in the Derivatives task faster than they could load but it is a hard requirement for https://github.com/Smithsonian/dpo-voyager/pull/324 or any similar feature.

Also, shouldn't break or slow down anything.

gjcope commented 4 days ago

I'm getting a couple of TS errors on build that seem to be missing type definitions. We must have some difference in build environment.

ERROR in /app/source/client/io/ModelReader.ts ./io/ModelReader.ts 129:48-54 [tsl] ERROR in /app/source/client/io/ModelReader.ts(129,49) TS2339: Property 'reason' does not exist on type 'AbortSignal'.

ERROR in /app/source/client/models/Derivative.ts ./models/Derivative.ts 82:36-63 [tsl] ERROR in /app/source/client/models/Derivative.ts(82,37) TS2554: Expected 0 arguments, but got 1.

gjcope commented 4 days ago

Different Node version maybe?

sdumetz commented 4 days ago

Ah, you must be using node 14? Just checked, it's been introduced in v16.

As it's not something that's immediately needed, should I wait for the project to move to v16 or try to make it work on v14 if that's not happening anytime soon?

Asking because latest versions of some tools don't support v14 anymore so this might block future updates.

gjcope commented 4 days ago

I'm using the Docker file in the repo which is currently on v16.20.2

gjcope commented 4 days ago

The types in package.json are still on v14. I've updated to v16 types locally and it's all good. When this gets merged it sounds like we will also need to up the minimum Node version to 16.14

gjcope commented 4 days ago

This is working really nicely, thanks again for the PR. We've needed this for awhile but I was leaving it to Three.js...

Similar to #325 the only issue I'm seeing is problems with the interaction prompt, probably also due to the async load here.

gjcope commented 4 days ago

This is working really nicely, thanks again for the PR. We've needed this for awhile but I was leaving it to Three.js...

Similar to #325 the only issue I'm seeing is problems with the interaction prompt, probably also due to the async load here.

I think the main culprit here is that we are trying to only show the interaction prompt once the initial model load finishes. We were previously using the 'busy' state of CVAssetManager as a proxy for this, which worked because of the synchronous loading. Now the loading comes in spurts and "finishes" multiple times during one load.

sdumetz commented 1 day ago

Busy might still be a good thing to track (though I'd like to surface this as an event or something to improve the experience when using voyager-explorer's public API).

It's just the loadingManager that is no longer hooked-up properly no? Maybe just https://github.com/Smithsonian/dpo-voyager/commit/d28f7b5af9688bbd9c8aaefaa49113574e10f520 would do the trick?

I tested on a few scenes locally and it seemed to work.

gjcope commented 1 day ago

It's just the loadingManager that is no longer hooked-up properly no? Maybe just d28f7b5 would do the trick?

Was this meant for #325? This PR looks to already be handling the loadingManager more or less the same way.

sdumetz commented 1 day ago

It can be used as-is on master and would work on #325 . Some changes might be needed to apply over this PR.

It all depends in which order you wish to apply the commits. I can merge the three changes together, make it work and submit a signel PR for the whole package if you think it's easier to review?

gjcope commented 1 day ago

Yeah it's not clear to me how it would change execution in this PR, so if you can submit something that would be helpful. Doesn't need to be all as one, but that's fine if it's easier.