Skip to content

Conversation

gabrielcramer
Copy link

@gabrielcramer gabrielcramer commented Aug 30, 2016

I needed this functionality and I think others will find it useful too. The possibility to abort can also be implemented just for a request ( The active request of a Loader). To accomplish this I was thinking about keeping a reference to the current XMLHttpRequest.

Here is an example with React:

class Example extends React.Component {

    constructor(props) {

        super(props);
        this.loadingManager = new THREE.LoadingManager();
        var texloader = new THREE.TextureLoader(this.loadingManager);
        // load textures

    }
    componentWillUnmount(){

        this.loadingManager.abortAllActiveRequests();

    }
}

@gabrielcramer
Copy link
Author

There was also an inconsistency inside ImageLoader. It didn't pass its loading manager ( this.manager) to the XHRLoader created inside the load method. So the ImageLoader was calling the itemStart and itemEnd methods of its loading manager and the XHRLoader was calling the itemStart and itemEnd methods of the DefaultLoadingManager for the same call of the load method of the ImageLoader.

@gabrielcramer gabrielcramer force-pushed the dev branch 2 times, most recently from 9baf3a6 to 4676939 Compare September 7, 2016 10:34
@gabrielcramer gabrielcramer changed the title Add possibility to abort all active requests managed by a LoadingManager. Allow user to abort all active requests managed by a LoadingManager. Sep 7, 2016
@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

There was also an inconsistency inside ImageLoader. It didn't pass its loading manager ( this.manager) to the XHRLoader created inside the load method. So the ImageLoader was calling the itemStart and itemEnd methods of its loading manager and the XHRLoader was calling the itemStart and itemEnd methods of the DefaultLoadingManager for the same call of the load method of the ImageLoader.

I suspect this change has side effects. It tends to be better to keep PR simple and on topic.

@gabrielcramer
Copy link
Author

I don't see what side effects it can have. The other loaders that use XHRLoader pass the loading manager. They let the XHRLoader call itemStart, itemEnd and itemError methods. Now if the load method of an ImageLoader is called these methods are called twice.

This was on topic. It's not possible to elegantly abort all active requests with two LoadingManagers ( DefaultLoadingManager always used by XHRLoader when created by ImageLoader and the one passed to the ImageLoader).

I think the PR is simple, just a few lines of code were modified.

@makc
Copy link
Contributor

makc commented Sep 28, 2016

abortAllActiveRequests

within this particular manager, right? not all requests in application?

@gabrielcramer
Copy link
Author

Yes, a loading manager can abort its active requests. Usually you don't use many loading managers in one application, but I'm open to suggestions. What do you think about the proposed solution to abort a single request (the current one of a loader)?

@makc
Copy link
Contributor

makc commented Sep 28, 2016

Usually you don't use many loading managers in one application,

it's the opposite for me, every time when user wants to load another scene, I make new manager to have common onLoad callback for all the stuff. but, if this happens before previous scene was loaded, I had to resort to hacks in order to abort it.

What do you think about the proposed solution

I did not check the code, sorry.

@gabrielcramer
Copy link
Author

gabrielcramer commented Sep 28, 2016

I don't understand why you don't use the same loading manager. When the user wants to load another scene you just abort all the active requests and the onLoad callbacks won't be called. It seems like a clean solution to me.

I did not check the code, sorry.

I didn't write any code that enables the user to abort only a request. I just proposed to have a reference inside every loader to the current request, but I'm not sure if it is the best way to do it. Another way could be to return the request.

@makc
Copy link
Contributor

makc commented Sep 28, 2016

to return the request

which was exactly what I did in #8091 and, because I went that easier way, I could not abort from the manager.

@mrdoob
Copy link
Owner

mrdoob commented Nov 5, 2016

I think I would implement this in a different way. LoadindManager could have a items = [] array that gets populated when calling itemStart and depopulated when calling itemEnds. We would then just need abortAll() which would go through the current list and about everything.

@gabrielcramer
Copy link
Author

gabrielcramer commented Nov 8, 2016

Ok, sounds good, but it should be something like scope.manager.itemStart( url, request); with request as an optional parameter. ( when we have the url in Cache we don't pass a request to itemStart )
It would be nice to do this only in FileLoader, to pass the manager from ImageLoader to FileLoader and don't use it inside ImageLoader. Now a loading manager is used twice. ( a default loading manager is used inside FileLoader).

@gabrielcramer gabrielcramer reopened this Nov 9, 2016
@gabrielcramer
Copy link
Author

I created a new commit with your instructions, but I think the name of the method abortAll is not suggestive enough.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 24, 2019

Um, I'm not sure LoadingManager.abortAll() is a good idea. To implement this method, it's necessary that LoadingManager has access to all active XMLHttpRequest request objects. However, ImageLoader does not load its images via FileLoader. It uses plain HTML image elements. These requests are currently not respected by this PR and thus can't be aborted (I think you can abort the loading of images by setting the src attribute to ""). But even if the requests of ImageLoader are respected in some way, ImageBitmapLoader needs a different processing since it uses fetch. I have the feeling all this makes an implementation quite complex.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2019

Closing for now. This feature needs to be implemented differently if it is actually desired.

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