smart-fun / smartGL

SmartGL is a Graphic Engine for creating Android Games and Apps. It is based on OpenGL and handles 2D Sprites and 3D Textured Objects.
Apache License 2.0
109 stars 24 forks source link

Memory leak in RenderPassSprite.java #1

Closed luna-park closed 7 years ago

luna-park commented 7 years ago

Hi, Arnaud! Your library is cool, but i found memory leak in RenderPassSprite.java in the sortObjects method:

@Override
    void sortObjects() {
        Vector<RenderObject> objects = getRenderObjects();
        Collections.sort(objects, new Comparator<RenderObject>() {
            @Override
...

I fix it like this (maybe not best solution):

private Comparator<RenderObject> comparator;

 public RenderPassSprite() {
        super(false, false);
        ShaderTexture shader = new ShaderTexture();
        setShader(shader);
        comparator = new Comparator<RenderObject>() {
            @Override
            public int compare(RenderObject leftO, RenderObject rightO) {
....
    }

@Override
    void sortObjects() {
        Vector<RenderObject> objects = getRenderObjects();
        Collections.sort(objects,  comparator);

... move comparator creation in constructor.

smart-fun commented 7 years ago

Hello luna-park and thanks for your feedback :)

I can't see any memory leak in this code, the Comparator is created when the sortObjects() method is called, and is released when the sortObjects() method is finished. The Comparator will be garbage collected later.

Could you please explain why do you think there is a memory leak?

For performance reasons it would be probably better to create a static method instead of creating an instance at every frame. I'll note this as a possible improvement.

thanks again,

Arnaud.

luna-park commented 7 years ago

Thanks for reply :)

In my application too many sprites and (if I run it on my old "shitphone") phone memory is quickly filled with instances of objects. bullrampdump

Happen very often calls the garbage collector.

smart-fun commented 7 years ago

Hello,

I've made an optimization (not released yet) so that the Comparator method is created only once instead of once per frame, but there is still a problem with the java Collections.sort: it consumes memory :/

see stackoverflow memory-consumption-of-java-collection-sort

For the moment I have no alternatives for sorting the array without allocating memory, but I'll search. If you have any idea please tell.

How many sprites do you display? Do you use the setDisplayPriority on the sprites? If yes, what values are you using? Do you change them very often or just at start ?

I'm thinking of disabling sorting if not necessary, or handle the sprites differently so that they are sorted when changing their priorities, not at every frame.

luna-park commented 7 years ago

Hello, Arnaud.

I think you should leave the sorting method as it is. It is enough that once comparator is created, not in the cycle.

The application displays a minimum of 5 sprites, 2 of which is a dynamic gamepad. I have not used setDisplayPriority for sprites, since I did not know about this feature. Thanks for the tip, I'll try to experiment with it :)

In my code, there is not a good fragment:

    @Override
    public void onPrepareView(SmartGLView smartGLView) {
        SmartGLRenderer renderer = smartGLView.getSmartGLRenderer();

        RenderPassSprite bgSprite = new RenderPassSprite();
        renderPassObject3D = new RenderPassObject3D(RenderPassObject3D.ShaderType.SHADER_TEXTURE, true, true);
        renderPassSprite = new RenderPassSprite();

        renderer.addRenderPass(bgSprite); // Static background sprite
        renderer.addRenderPass(renderPassObject3D);
        renderer.addRenderPass(renderPassSprite);  // HUD

What do you think about it?

smart-fun commented 7 years ago

Regarding the sort method, I have pre-released a version 1.1.0, if you want to test it.

Regarding your "not a good fragment" question, I do not understand what the problem is. Could you explain more?

Just for your information, you can do a Hud using Android views: if you have a RelativeLayout with a SmartGLView, you can add other standard views on top of it. You don't have to use a RenderPass if you are more comfortable with TextView, ImageView or Button ;)

luna-park commented 7 years ago

Do not worry about "not a good fragment", bro. Probably I need not worry.

In earlier versions of the application, I used the standard views for HUD. But on some devices appeared strange bug - instead of the characters appear black rectangles. I fixed it by turning off hardware acceleration in these views, which resulted in reduced performance. Bad practice.

smart-fun commented 7 years ago

Try to optimize your textures, like not using 1024x1024 pixels textures if on screen the faces are small. Don't forget to release the textures when you are done with them. This way the system has better chances to get space in the gpu.

If we are done with the "memory leak", we could close this Issue. Ok?

luna-park commented 7 years ago

Yes, close it. Thank you very much :)