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

Proper way to release context of a renderer immediately? #27100

Closed
hybridherbst opened this issue Nov 1, 2023 · 5 comments · Fixed by #27136
Closed

Proper way to release context of a renderer immediately? #27100

hybridherbst opened this issue Nov 1, 2023 · 5 comments · Fixed by #27136

Comments

@hybridherbst
Copy link
Contributor

hybridherbst commented Nov 1, 2023

Description

I'm running into a case where (currently) multiple renderers are used for GPU texture readback.
(see https://github.com/mrdoob/three.js/blob/834d77de4055ee81afa78ad8141447460501daf5/examples/jsm/utils/TextureUtils.js#L61C11-L61C11).

I understand that the better way would be to pass a renderer into the method; however, I'd like to learn more about improving the current default behaviour.

While debugging some cases where I'm getting context losses:
image
I noticed that calling renderer.forceContextLoss prevents there being too many contexts. Even with hundreds of renderers being created quickly after each other, there's no context loss, which is a much better behaviour.

My understanding was (and the docs say so) that forceContextLoss basically simulates losing the context – and shouldn't be used to clean up resources. The practical effect does, in fact, seem to be fully cleaning up the renderer and avoiding side effects.

So the question is:

  • should renderer.forceContextLoss() be called in the renderer's dispose method to avoid the problem of context losses due to too many contexts?
  • is there a proper way of actually releasing the context?

Reproduction steps

Repeatedly call TextureUtils.decompress, e.g. 20 times in a loop.

Code

	if ( _renderer ) {

                // this actually prevents context losses on many renderers
                _renderer.forceContextLoss();
                // this is not enough to prevent context losses
		_renderer.dispose();
		_renderer = null;

	}

Live example

Screenshots

No response

Version

latest

Device

No response

Browser

No response

OS

No response

@gkjohnson
Copy link
Collaborator

gkjohnson commented Nov 2, 2023

There's no way to explicitly dispose of a WebGL context. The forceContextLoss function exists purely to simulate a context loss to test app behavior in that scenario and relies on the WEBGL_lose_context extension. See the MDN page.

@hybridherbst
Copy link
Contributor Author

I understand the theory. In practice though, forceContextLoss does prevent the "too many active WebGL contexts" event from happening, thus my question :)

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2023

#26666 would actually add forceContextLoss() to TextureUtils.decompress(). The PR hasn't been merged yet since there are some open issues in other contexts though.

In general, it was never clear to me how loseContext() should be used since the spec is somewhat ambiguous. It says:

Implementations should destroy the underlying graphics context and all graphics resources when this method is called. This is the recommended mechanism for applications to programmatically halt their use of the WebGL API.

When I read this, it seems okay to use the method for "cleanup" operations and not just for simulating a context loss.

I'm okay with adding forceContextLoss() to TextureUtils.decompress() but not to WebGLRenderer.dispose() for now. This is something I would only do if there is a recommendation from one of the WebGL implementers.

@gkjohnson
Copy link
Collaborator

This is the recommended mechanism for applications to programmatically halt their use of the WebGL API.

Oh interesting - I hadn't seen this. The use of the word "simulating" in the spec is confusing, then, and at odds with this statement.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 3, 2023

@hybridherbst Would you be willing to file a PR that adds the forceContextLoss() call to TextureUtils.decompress()? I think we can extract this change from #26666 and apply it now.

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.

3 participants