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

WebGLRenderer: Release cache at the end of .render(). #14946

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

takahirox
Copy link
Collaborator

I think it's better to release _currentCamera at the end of WebGLRenderer's .render(). Unless doing this, disposed camera resource won't be released until next .render() call because renderer keeps a reference to camera.

@takahirox takahirox changed the title Release _currentCamera at the end of .render() Release cache at the end of .render() Sep 21, 2018
@mrdoob mrdoob added this to the r101 milestone Jan 7, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 7, 2019

Say that we're rendering a scene with just one object and one material...
Wouldn't be doing unnecessary binding calls when rendering again?

@takahirox
Copy link
Collaborator Author

What does "binding calls" mean by in this context?

@takahirox
Copy link
Collaborator Author

takahirox commented Jan 8, 2019

Ah, binding buffer, isn't it?

Even in the current impl it binds every frame for a scene with one object and one material because it releases the cache at the beginning of .render(). So this PR change nothing about binding calls.

@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob mrdoob modified the milestones: r104, r105 Apr 24, 2019
@mrdoob mrdoob modified the milestones: r105, r106 May 30, 2019
@mrdoob mrdoob modified the milestones: r106, r107 Jun 26, 2019
@mrdoob mrdoob modified the milestones: r107, r108 Jul 30, 2019
@mrdoob mrdoob modified the milestones: r108, r109 Aug 27, 2019
@mrdoob mrdoob modified the milestones: r109, r110 Sep 25, 2019
@mrdoob mrdoob modified the milestones: r110, r111 Oct 30, 2019
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob modified the milestones: r120, r121 Aug 25, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2021

@mrdoob This PR is about when a cache or cache-like variables should be resetted.

Similar like the reset of render states and lists, the mentioned code block should be moved at the end of render(). In this way, the cache variables do no hold references to objects after the method has been quitted. This change does not increase the number of binding calls.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2021

@Mugen87 Is it safe to merge?

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2021

I guess there is only one way to know... 🤓

@mrdoob mrdoob merged commit 5f8ef89 into mrdoob:dev Mar 17, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2021

Thanks!

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.

3 participants