Reduce coupling between Input System and CameraTargetSystem

[emanuele3d]

As per conversation in PR https://github.com/MovingBlocks/Terasology/pull/1094:

The Input System has a dependency on the CameraTargetSystem (CTS), which crops up in places like StateMainMenu.init(GameEngine), where the CTS is initialized and registered for the Input System to work correctly. While Immortius has features in mind requiring camera(s) in the Main Menu (i.e. to deal with 3d content - customizable avatar perhaps?), the Main Menu currently has no need for this. In any case the dependency should be reduced, to avoid having to deal with the CTS when there are no cameras involved.

[zeokav]

@emanuele3d I went through this and it looks like the CTS is used to figure out what the mouse's position was when a button was clicked. Basically the code that updates bind state uses the targetSystem-

bind.updateBindState(
    action.getInput(),
    action.getState().isDown(),
    delta,
    getInputEntities(),
    targetSystem.getTarget(),
    targetSystem.getTargetBlockPosition(),
    targetSystem.getHitPosition(),
    targetSystem.getHitNormal(),
    consumed
);

I logged all the targetSystem values and turns out that no matter where I click or what I do on the main screen, the values are the same (Everything zero). What I'm thinking for a workaround is a separate class to get Input from the menus (Might be a lot of changes though), or I could throw in a new updateMenuBindState function that just sets the values of the target system args as nulls or zeroes when on a menu (Hoping that there are no NPEs later in other menus and stuff).

[emanuele3d]

@zeokav: stand-by for more feedback tonight. Right now I have to prep lunch for my family.

Tagging @rzats as he's one of our UI wizards and perhaps he can provide some comments on the matter.

[emanuele3d]

I haven't found the time to look at the code today and I'm not quite sure what you are saying here.

1. are you saying that the target system is used to keep track of the mouse on the screen for UI purposes?
2. but then you are saying that it actually doesn't do anything?

Somehow I feel the target system make sense in a 3D context. In a 2D UI it feels a bit overkill conceptually. Or perhaps there are good reasons.

rzats has received the notification about this issue. He hopes to contribute to this discussion later today. Let's see what he says.

[zeokav]

@emanuele3d I'm not saying that it does not do anything. Actually, that's what I'm unsure of. The updateBindState method is called when an input action takes place. And you're right - in a 2D context, the values of the arguments targetSystem.getTarget(), targetSystem.getTargetBlockPosition(), targetSystem.getHitPosition(), targetSystem.getHitNormal() remain the same regardless of where you give the input.

Also, we can't just get rid of the CameraTargetSystem since the same InputSystem class is being used for both targeting while actually playing the game (which is where it's really required) and for navigating through menus. An alternative that we have is to have a separate Input System implementation for the menus. The current InputSystem is initialized only when the person enters the game world (which already happens in StateLoading). In this case the CTS would not be required in the main menu. Let me know how that sounds.

EDIT - Playing around with the code a bit more, CameraTargetSystem is actually being initialized thrice. Twice in the menu and once in the StateLoading procedure. Would like some general knowledge on this - does initializing and registering the same system multiple times drop the previous initialization?

[emanuele3d]

Personally my problem with the CTS used this way is that it implies a Camera, and while we could argue that OpenGL always see things through a Camera, even when dealing with 2d things, in the context of Menus it's a bit of a stretch and makes the code less clear.

In this context I'd like the idea of a separate InputSystem for menus that doesn't use the CTS, especially given that it seems the CTS is not in use while in Menus. There is however a cost/benefit concern: as it is it works. If it's a lot of work to disentangle the two we might want to hold on it as the current status quo, albeit not ideal, works. If it's not too much work then I'd recommend we do it. I guess the exception would be a complete refactoring in the context of a GSOC proposal, but you'd have to come up with solid ideas on why such refactoring would be useful.

Regarding the triple-initialization: I can see the initialization happening in RegisterInputSystem and StateMainMenu. Where's the third initialization?

That been said, it does seem unnecessary and possibly even wrong to initialize it in all those places. If something is stored in the CoreRegistry we should get it from there. By the sound of it we end up with three separate instances of the CTS used in different parts of the engine when in fact we should be using the same one, if anything because it saves memory.

[zeokav]

@emanuele3d The task didn't look like it was going to be so deeply rooted when I first saw it (Quite a few other places where it's being used). I'll try to implement a separate InputSystem for the menus by this weekend. If I can't come up with a viable solution, I'll move on the some other issues that deal with rendering. (It'd be nice if I could find something at an entry level difficulty :p) It's definitely not big enough to be a GSoC item - there's not much you can do with it really.

Two of the initializations are in StateMainMenu (which also calls RegisterInputSystem), and once before the game starts. Removing one of the two initializations of the CTS in StateMainMenu does not cause anything wrong to the game. I'll remove it in my next PR.

[emanuele3d]

Good luck. Meanwhile I'll see if I can line up some stars to get a reviewer that is familiar with that code.

[rzats]

Dropping in to say that yes - a separate, lightweight InputSystem is the best solution to this issue, as CameraTargetSystem (and also LocalPlayer) don't appear to be used for any menu-related purposes. Theoretically they might be used in a fancy "playable" menu like this, but that shouldn't really factor into this issue.

[emanuele3d]

@rzats: if one day we have a character selection/customization screen or window, will this change come to hunt us? I suspect not, but I thought I'd ask.

[rzats]

@emanuele3d: I don't think so. If we ever need to create a widget that can render a character model, the information about the model's state, the camera's position, angle etc. can (and should!) be stored within the widget itself.

[emanuele3d]

Understood. Thank you.

[zeokav]

@rzats Thanks for dropping in! I'm currently trying to use the original input system and extract things out of it to make a bare minimum class that can process inputs. If you are familiar with that part of the code, are controllers being used to navigate the menus? I don't have one so I cannot check - but it doesn't look like keyboard inputs are being used (if I modify it to get only mouse inputs, it's going to cut down a good 100 lines). I have figured out the sequence of actions that happen to process mouse clicks - fairly straightforward now that I have it written down. I have somewhat simulated it without using CameraTargetSystem using the InputSystem class. I need to translate that to another class and pray for no crashes. 😛 Also, you said that LocalPlayer doesn't seem to be doing anything - but all the mouse inputs are processed against the client entity, even on the menu. And also one more thing... even when I click once, two mouse events are sent (figured that out while logging to check something). I can't figure out why. While it doesn't really matter because the update rate is really high, I would still like to know why that happens.