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

Add LightTag and call Three.js light.dispose when removed #5915

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

takahirox
Copy link
Contributor

Add LightTag component that must be set for all lights. Call Three.js light.dispose() when removed in remove-object3D-system.

This new tag and flow provides a proper light resource disposing.

Currently light.dispose() is called right after DirectionalLight creation in inflator/directional-light if light.shadow.map is non-null but it seems a bug because light.shadow.map should be null when light is created. This commit also fixes it by removing the lines. Lights will be properly disposed when removed with the change.

Add LightTag component that must be set for all lights. Call
Three.js light.dispose() when removed in remove-object3D-system.

This new tag and flow provides a proper light resource disposing.

Currently light.dispose() is called right after DirectionalLight
creation in inflator/directional-light if light.shadow.map is
non-null but it seems a bug because light.shadow.map should be
null when light is created. This commit also fixes it by removing
the lines. Light will be properly disposed when removed with
the change.
Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

if (light.shadow.map) {
light.shadow.map.dispose();
(light.shadow.map as any) = null; // TODO: Correct the Typescript definition in three. This /is/ nullable.
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Thanks for removing this

@takahirox takahirox merged commit bcb4277 into master Jan 30, 2023
@takahirox takahirox deleted the LightTag branch January 30, 2023 18:37
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.

None yet

3 participants