Is the community up to add a new function to InputManager class?
InputManager doesn’t not check to see if the class is listening already before it adds it to the list
public void addRawInputListener(RawInputListener listener) {
rawListeners.add(listener);
}
This can cause multiple listeners of the same class (if error in code happens). This will cause it to remain in memory because if you call remove it will only remove one, and leave the second. So a reference to that class stays around.
public void removeRawInputListener(RawInputListener listener) {
rawListeners.remove(listener);
}
I’m thinking to add a function that code can call to see if itself is already listening so it can skip calling it.
Or maybe check if it is already there and not add it again?
Either way works?
I like doing it with the check, because I turn it on/off throughout the code life time.
* Call this function to determine if listener is already listening
* to raw inputs.
*
* @param listenerCheck listener to check to see if already listening
* to raw inputs.
* @return boolean to indicate listener is already listening
*/
public boolean isRawInputListening(RawInputListener listenerCheck) {
RawInputListener[] array = rawListeners.getArray();
for (RawInputListener listener : array) {
if (listener == listenerCheck)
return true;
}
return false;
}
Would it also work to just use the .contains() method, since rawListeners is a SafeArrayList and has a .contains() method?
public void addRawInputListener(RawInputListener listener) {
if(!rawListeners.contains(listener)){
rawListeners.add(listener);
}
}
That’s the format I follow for the add/register method of all the listeners I’ve written for my project to make sure the same one is never mistakenly double registered.
Yes, there are many solutions, but InputManager doesn’t prevent double listening and then staying around with a reference on the 2+ listener is this happens.
I’m just thinking that is this really the responsibility of InputManager? To fix bad code made by users. Is this typical for the listener pattern to also validate things like this?
As a general note, if one is using AppStates. Init & Cleanup works pretty well.
In an application that always removes the listeners that it adds… it’s possible that two of the same listener gets added on purpose because one is about to be removed.
There should be no forced uniqueness… and a check to see “is my code bad?” seems weird, too.
If you can find cases in regular Java where this pattern has been done, I could be swayed.
The proper name for isRawInputListening would be isRawInputListenerAlreadyAdded… which is why it feels weird.
I too believe that the method for registering a listener should not perform a strict check on duplicates. IMO if the presence of duplicates does not cause malfunctions within the functions executed by the InputManager, such a check should be performed by the programmer. This way everyone can freely decide what action their application should take:
Not add the duplicate.
Print a log message.
Raise an exception.
However, the InputManager class does not provide methods for knowing which/how many listeners are registered.
Doing a spot check most of the classes in which a listener can be registered do not provide methods to know how many/which listeners have been registered before.
To solve this problem, new methods could be added to the InputManager class:
Option 1:
public boolean hasRawInputListener(RawInputListener listener) {
for (RawInputListener rawListener : rawListeners.getArray()) {
if (rawListener == listener) {
return true;
}
}
return false;
}
public boolean hasJoystickConnectionListener(JoystickConnectionListener listener) {
for (JoystickConnectionListener joystickListener : joystickConnectionListeners.getArray()) {
if (joystickListener == listener) {
return true;
}
}
return false;
}
Option 2:
public JoystickConnectionListener getJoystickConnectionListener(int index) {
return joystickConnectionListeners.get(index);
}
public int getNumJoystickConnectionListeners() {
return joystickConnectionListeners.size();
}
public RawInputListener getRawInputListener(int index) {
return rawListeners.get(index);
}
public int getNumRawInputListeners() {
return rawListeners.size();
}
or Option3 = Option 1 + Option 2
Only in the old AnimControl class (now deprecated) for some reason a different pattern was adopted.
But this approach may be too aggressive for the InputManager class. Better as it is now without duplicate checks.
Here is an example:
/**
* Adds a new listener to receive animation related events.
*
* @param listener The listener to add.
*/
public void addListener(AnimEventListener listener) {
if (listeners.contains(listener)) {
throw new IllegalArgumentException("The given listener is already "
+ "registered at this AnimControl");
}
listeners.add(listener);
}
/**
* Removes the given listener from listening to events.
*
* @param listener the listener to remove
* @see AnimControl#addListener(com.jme3.animation.AnimEventListener)
*/
public void removeListener(AnimEventListener listener) {
if (!listeners.remove(listener)) {
throw new IllegalArgumentException("The given listener is not "
+ "registered at this AnimControl");
}
}
Yes, I was just thinking of just adding a check to see if the class passed in is already listening. This way everyone can do their special needs and not place limitations on what JME can do.
I like option 1.
Let me know what everyone thinks and I can submit a change for this.
I run into this with ToneGodGui having multiple screens.
Does it not have a way to detect uninitialization in this case?
I’m kind of against adding a method whose sole purpose is to help people work around their poor listener discipline. What will you do in all of the other cases where you need to remember to remove your added listeners?
Look across the whole of the JDK which has literally dozens and dozens of examples of adding/removing listeners and somehow it has managed to avoid having to add “did I already add this listener” methods everywhere.
I know it’s a small thing but it’s letting people continue to have broken code rather than forcing them to fix their code.
I personally like to leave extra checks like this in as guard rails for if/when my game is moddable, since I expect many modders to not be as skilled coders and they are ultimately the customers that I should tailor to and do my best minimize the frustration of. So I always write code to minimize the severity of doing something wrong and add lots of guard rails to my code (as long as isn’t invasive). But it’s also understandable if checks like these are best left out of the engine.
Yes that is the safe way to do it… but I’m not talking about just InputManager specifically, I’m just referring to any/all places in my code where I have lists that shouldn’t have duplicate entries.
This is most useful when I have a list of things that I call an update() method on (to avoid using controls) since I don’t want to ever let a modder do something like accidentally register the same mob twice by mistake, since that would make the NPC get updated twice per frame and move 2x as fast. Not a big deal and easy mistake to avoid, but also best to just add that check so an amateur modder has one less issue to run into.
I’m not saying to expose the core engine to modders, and I said its understandable that the engine isn’t written to accommodate bad code from modders. I’m just giving insight into my reasoning for adding .contains checks in similar places outside the core engine.
We are veering way off topic but it’s an interesting one…
It’s even better if you can avoid exposing them to managing raw lists like that in the first place. It doesn’t always work but sometimes thinking about the abstraction in a different way can make their lives easier. Switch raw lists for registries, stuff like that.
I understand the thinking behind everyone’s thought. I was not asking to limit but add a check to see if something is listening.
The other option, is about changing tonegodgui. Its management is. Control. Not an. AppState.
It would be much better if the screen was an AppState. It is not, it consumes events, even if not enabled.
Thought about fixing this, but it feels like tonegodgui screen should be an appstate instead an Ccontrol. This makes more sense in the flow of JME3.
But that would break backs wards compatibility, to me based on JME coding style, it feels like AppState would be it. I didn’t suggest that, as it would break everything.
Also, ToneGodGui doesn’t detach itself from the rawlistener. It stays attached. even when the Screen Control is removed. I would at least add the following function to get around this.
/**
* Call this function to setup the RawInputListener for this
* class. By default the listener is already setup.
* This is used if for some reason you need to stop listening
* to input but later on start listening again.
*
*/
public void attachInputListener()
{
app.getInputManager().addRawInputListener(this);
}
/**
* Call this function to tell class to stop listening to Raw
* Inputs. If you need to suspend the screen functions and
* everything under that screen. You can use this to make it
* stop reacting. This way you can detach the screen for a
* period of time and re-attach it and have it start listening
* to raw inputs again.
*/
public void detachInputListener()
{
app.getInputManager().removeRawInputListener(this);
}
This way the programmer can call these to turn on/off and I wish I could say when it is detached, but a control doesn’t get notified when being detached.
This is why it feels like it should be a AppState instead of control.
Maybe I’m wrong, but maybe you are not using the UI objects in the correct way in your application and are trying to modify other libraries instead of reviewing your code. I can’t say for sure since you haven’t shown us your Screen management code. I took a look at the code in the tonegodui library. When you create a new Screen, it automatically registers itself as a RawInputListener.
That makes sense to me, otherwise you would have to do it manually for each UI component. All UI components extend the Control class (see controls package), this makes me think that this is a clear design choice by the developer. You might have quite a bit of trouble turning everything into AppState without risking breaking something.
Perhaps you can simplify your life and try customizing your code this way:
“Programming is like philosophy, everyone has their own.” This is just my point of view, not the absolute truth. You are free to do as you wish.
Edit:
I have used the tonegodui library a little bit in the past, from the examples I always created only one Screen. Remember that you can always add/remove/show/hide the individual element (Inventory, Health, etc…) on the Screen as needed.
If Screen sets up a bunch of stuff when you create/add it then there should be a way for it to clean up that stuff… else it has really dirty lifecycle management.
Controls have no real lifecycle in JME. You can detect when they are added to a spatial and you can detect when they are removed from a Spatial but that’s it. There is no way to tell if the spatial you are added to is even viewed anymore.
…that’s why it can be a poor choice for adding things to global state.
Yes. I agree. Screen never removes itself from the listener. Which I guess the theory was it was a LIFE-TIME class for the entire application and was only INTENDED to have 1 SCREEN and each AppState or Node needs to getting the control from guiNode and reference it and keeps removing either “Windows” or individual objects every time a screen is shown/disabled.
There is no cleanup process for Screen, like you said it is hard to know the end of life for a control, easy for appstates.
