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

Memory leaked with Skybox and IndirectLight on Android #6412

Open
August1996 opened this issue Dec 28, 2022 · 7 comments · May be fixed by #8470
Open

Memory leaked with Skybox and IndirectLight on Android #6412

August1996 opened this issue Dec 28, 2022 · 7 comments · May be fixed by #8470
Assignees
Labels
android Issue/feature request for Android only

Comments

@August1996
Copy link

image

As the picture showing, Creating Skybox or IndirectLight by KTXLoader.createXXX will create cubemap Texture.
Normally, we may use Engine.destroy to release the Skybox or IndirectLight, but the cubemap Texture will not be destroyed unless the engine is destroyed or destroying the cubemap Texture explicitly(it is very oddly because nobody can realize it).

@romainguy romainguy added bug Something isn't working android Issue/feature request for Android only labels Jan 9, 2023
@romainguy
Copy link
Collaborator

Note: this only happens with the utility functions.

@pixelflinger
Copy link
Collaborator

Thanks for the report. This is a bad one, basically the API is broken.

@romainguy
Copy link
Collaborator

We can fix it easily in the Java case. Instead of a single long we should return two (the two pointers) and have the Java layer perform the additional destroy as needed.

@pixelflinger
Copy link
Collaborator

pixelflinger commented Jan 10, 2023

@romainguy I don't think it will work, because the java (kotlin actually) layer doesn't store the long, it just returns it to the caller:

    fun createIndirectLight(engine: Engine, buffer: Buffer, options: Options = Options()): IndirectLight {
        val nativeEngine = engine.nativeObject
        val nativeIndirectLight = nCreateIndirectLight(nativeEngine, buffer, buffer.remaining(), options.srgb)
        return IndirectLight(nativeIndirectLight)
    }

This is completely stateless.

@romainguy
Copy link
Collaborator

We can store that in the Java IndirectLight object. It's very doable.

@pixelflinger
Copy link
Collaborator

pixelflinger commented Jan 10, 2023

Okay but we now made all the Java objects bigger just to support that broken API -- And we're breaking the model of java objects being "dumb", now these have logic.

@pixelflinger pixelflinger removed the bug Something isn't working label Jan 16, 2025
@poweifeng
Copy link
Contributor

circling back after two years. I think we can just make createIndirectLight return a pair of IndirectLight and Texture. Callers will have to own the Texture as well.

poweifeng added a commit that referenced this issue Feb 24, 2025
We break existing  API by returning both the IndirectLight and the
texture. Same thing with Skybox.

Fix one bug where the float array in getSphericalHarmonics wasn't
getting written out.

Fixes #6412
poweifeng added a commit that referenced this issue Feb 24, 2025
We break existing  API by returning both the IndirectLight and the
texture. Same thing with Skybox.

Fix one bug where the float array in getSphericalHarmonics wasn't
getting written out.

Fixes #6412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Issue/feature request for Android only
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants