Cancellable models load #326
Conversation
fix a race condition on derivative load callback. Explain early return cases in comments
|
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 ERROR in /app/source/client/models/Derivative.ts |
|
Different Node version maybe? |
|
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. |
|
I'm using the Docker file in the repo which is currently on v16.20.2 |
|
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 |
|
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. |
| @@ -306,7 +312,7 @@ export default class CVModel2 extends CObject3D | |||
| } | |||
| else if (ins.quality.changed) { | |||
| const derivative = this.derivatives.select(EDerivativeUsage.Web3D, ins.quality.value); | |||
| if (derivative && derivative !== this.activeDerivative) { | |||
| if (derivative) { | |||
There was a problem hiding this comment.
What's the rationale for removing the activeDerivative check?
There was a problem hiding this comment.
It's checked inside this.loadDerivative. Otherwise if you had Highfully loaded and switched from to Low then to High again before Low was loaded, you'd have derivative === this.activeDerivative and Lowwouldn't get cancelled.
Here loadDerivative get called, this._loadingDerivative (Low) gets cancelled and loadDerivative returns early here.
Could have been the other way around and added and interrupt here but I'd rather have loadDerivative as robust as possible.
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. |
|
It's just the I tested on a few scenes locally and it seemed to work. |
|
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? |
|
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. |
|
Closed by #332 |
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 mrdoob/three.js#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
Derivativestask faster than they could load but it is a hard requirement for #324 or any similar feature.Also, shouldn't break or slow down anything.