[Fixed] Unregistering of RenderGameOverlayEvent does not work

[Chimaine]

Hello all.

 

I already asked this on the irc channel, but we couldn't find a solution.

 

I am using the new RenderGameOverlayEvent to render behind or hide the GuiIngame hotbar.

When my gui is created, I'm registering it for the event at the forge event bus.

When my gui closes, I'm unregistering it from the event bus, but the event handler still gets called.

 

I could use some kind of visibility check, and while this solves the problem at hand, it creates some kind of memory leak, as the reference to the instance still gets held by the event bus.

 

Heavily cut of pastebin: http://pastebin.com/v3wwGqcu

 

Btw, I'm loving the new events! Last week I thought "damn, if only i could render behind the hotbar!" and today I saw that commit.

 

While I was at it, I also found a spelling error in the ListenerList class, line 100, unregiterAll -> unregisterAll

[Chimaine]

Hello all.

 

I already asked this on the irc channel, but we couldn't find a solution.

 

I am using the new RenderGameOverlayEvent to render behind or hide the GuiIngame hotbar.

When my gui is created, I'm registering it for the event at the forge event bus.

When my gui closes, I'm unregistering it from the event bus, but the event handler still gets called.

 

I could use some kind of visibility check, and while this solves the problem at hand, it creates some kind of memory leak, as the reference to the instance still gets held by the event bus.

 

Heavily cut of pastebin: http://pastebin.com/v3wwGqcu

 

Btw, I'm loving the new events! Last week I thought "damn, if only i could render behind the hotbar!" and today I saw that commit.

 

While I was at it, I also found a spelling error in the ListenerList class, line 100, unregiterAll -> unregisterAll

[RANKSHANK]

Use an instance, For example I use

public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

[RANKSHANK]

Use an instance, For example I use

public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

[SanAndreaP]

  On 4/22/2013 at 9:01 PM, RANKSHANK said:

Use an instance, For example I use

/**@author RANKSHANK*/
@SideOnly(Side.Client)
public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

 

@SideOnly(Side.Client)

Why is this there? You can use this class for a server as well, since it doesn't have any client-related stuff there!

[SanAndreaP]

  On 4/22/2013 at 9:01 PM, RANKSHANK said:

Use an instance, For example I use

/**@author RANKSHANK*/
@SideOnly(Side.Client)
public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

 

@SideOnly(Side.Client)

Why is this there? You can use this class for a server as well, since it doesn't have any client-related stuff there!

[RANKSHANK]

just a snippet from my project, probably should have left that out too I'll fix it

EDIT: fixed, thanks for that

[RANKSHANK]

just a snippet from my project, probably should have left that out too I'll fix it

EDIT: fixed, thanks for that

[Chimaine]

  On 4/22/2013 at 9:01 PM, RANKSHANK said:

Use an instance, For example I use

public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

Tried that. Did not work.

And to be honest I dont know why this should work, since you pass the same references to register and unregister as in my example.

 

Could you post a working example?

[Chimaine]

  On 4/22/2013 at 9:01 PM, RANKSHANK said:

Use an instance, For example I use

public final class BuildController{

private BuildController(){instance = this;}

private static BuildController instance;

public static void bind(){
	if(instance == null)
		MinecraftForge.EVENT_BUS.register(instance = new BuildController());
}

public static void unbind(){
	if(instance != null){
		MinecraftForge.EVENT_BUS.unregister(instance);
		instance = null;
	}
}
}

and it cleanly registered and unregistered the listeners in the class-

Tried that. Did not work.

And to be honest I dont know why this should work, since you pass the same references to register and unregister as in my example.

 

Could you post a working example?

[Chimaine]

No ideas?

 

Unregistering other events then RenderGameOverlayEvent work just fine, I'm using it at several other places (my own registries, like in the example provided and with this).

[Chimaine]

No ideas?

 

Unregistering other events then RenderGameOverlayEvent work just fine, I'm using it at several other places (my own registries, like in the example provided and with this).

[LexManos]

The overlay events work exactly the same as any other event. As lon

g as you unregister the same instance that you registered itll work.

[Chimaine]

  On 4/25/2013 at 7:35 PM, LexManos said:

The overlay events work exactly the same as any other event. As lon

g as you unregister the same instance that you registered itll work.

Are you absolutely sure?

I even tested if the references I pass to register and unregister are the same.

I tried several places and constructs to where and how I register and unregister...

 

I'm an experienced Java developer, but I can't get this to work. I feel really stupid right now...

[LexManos]

Yes im sure, im the one who wrote both these new gui events and the event system. Feel free to trace the code yourself.

[Chimaine]

I finally found it!

 

  On 4/26/2013 at 9:04 AM, LexManos said:

Yes im sure, im the one who wrote both these new gui events and the event system. Feel free to trace the code yourself.

I know that you wrote it And I'm gratefull for it. I was just wondering if you had it tested properly.

 

I found out that unregistering in general does not work. I did some tracing like you suggested and found that in ListenerList, line 208, the method

public void unregister(IEventListener listener)
{
    for(ArrayList<IEventListener> list : priorities)
    {
        list.remove(listener);
    }
}

is missing a

rebuild = true

so the event listeners are removed, but the cache is never rebuild!

 

It should be

public void unregister(IEventListener listener)
{
    for(ArrayList<IEventListener> list : priorities)
    {
        if (list.remove(listener))
        {
        	rebuild = true;
        }
    }
}

 

I would make a pull request on GitHub, but I dont have an account, sorry

 

//edit: Nevermind, I created a pull request: https://github.com/MinecraftForge/MinecraftForge/pull/543