Skip to content

Conversation

@J0EK3R
Copy link

@J0EK3R J0EK3R commented Sep 28, 2025

After many hours of thinking, refactoring, moving and renaming files - here is my suggestion for a generic handling of inputdevices (like gamecontrollers, MCP server,...).

Now, there is one instance of a InputDeviceManagerService handling all InputDeviceServices (all kinds of inputdevice services like GameControllerService, Gyro, whatever) and all InputDevices (gamecontrollers, gyro and so on).

The central InputDeviceManagerService still raises inputdevice events on button press, axis movement, channel changes and connection/disconnection inputdevices.

An InputDeviceService handles one special kind of inputdevices like gamecontrollers.
(In this branch there are only InputDeviceServices for gamecontrollers).

Now there is common base class InputDevice that replaces the former "Controller" - there was and still is a lot of renaming to do...
All inputdevices are derived from this baseclass.

One interesting part of code is the initialisation of all registered InputDeviceServices in InputDeviceManagementModule - perhaps there is a better way...

builder.RegisterType<InputDeviceManagerService>().As<IInputDeviceManagerService>().As<IInputDeviceEventServiceInternal>().As<IInputDeviceEventService>().SingleInstance()
                .OnActivated(e =>
                {
                    // resolve all registered input device services to trigger their registration
                    e.Context.Resolve<IEnumerable<IInputDeviceService>>();
                });

Please take some time and comment.

@vicocz vicocz requested a review from Copilot September 30, 2025 18:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This is a comprehensive refactoring to introduce a generic input device management system to replace the existing game controller-specific implementation. The changes introduce an InputDeviceManagerService to handle all input device types, with InputDeviceService implementations for specific device types like game controllers.

Key changes:

  • Replaced game controller-specific classes with generic input device interfaces and base classes
  • Introduced a centralized InputDeviceManagerService to manage all input device types
  • Renamed events, types, and properties throughout the codebase to use "InputDevice" terminology
  • Updated platform-specific implementations (iOS, Android, WinUI) to use the new architecture

Reviewed Changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
Various ViewModels and Services Updated to use new IInputDeviceEventService interface instead of IGameControllerService
PlatformServices/InputDevice/ New generic input device interfaces and base classes
PlatformServices/InputDeviceService/ New service layer for managing input device types
InputDeviceManagement/ New centralized management service and dependency injection setup
Platform-specific GameController implementations Refactored to inherit from generic input device base classes
UI and Business Logic Updated to use new event types and property names

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Owner

@vicocz vicocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still out of time, therefore left few comments only.

But in general I like Input Device naming. So I guess this will be nice feature to complete, even if it's kind of refactoring only.

.OnActivated(e =>
{
// resolve all registered input device services to trigger their registration
e.Context.Resolve<IEnumerable<IInputDeviceService>>();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? Why do we need RegisterInputDeviceService method? What's wrong with resolving IEnumerable<IInputDeviceService> as parameter of InputDeviceManagerService constructor?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still I need to check this more deeply...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IEnumerable as parameter of InputDeviceManagerService constructor?

That was my first attempt and it crashed - it's the chicken-or-egg problem ;)

The InputDeviceService derivations constructor need the InputDeviceManagerService instance as parameter.
So you can't resolve IEnumerable<IInputDeviceService> as parameter of InputDeviceManagerService constructor.
So we first need to create the InputDeviceManagerService instance and then all the registered InputDeviceService types.

@J0EK3R J0EK3R force-pushed the merge/vicocz/InputDeviceManagementService branch from 51cad4c to de81671 Compare October 3, 2025 10:34
@J0EK3R
Copy link
Author

J0EK3R commented Oct 3, 2025

I've startet renaming all the Controller-stuff (types, methods, etc.) to match the more common "InputDevice"-naming...
Does it make sense - yes, I think so.
But maybe we loose the ability to load or shere old stuff (Database?, json serialization...?)

What do you think?

@vicocz
Copy link
Owner

vicocz commented Oct 3, 2025

I've startet renaming all the Controller-stuff (types, methods, etc.) to match the more common "InputDevice"-naming... Does it make sense - yes, I thing so. But maybe we loose the ability to load or shere old stuff (Database?, json serialization...?)

What do you think?

No breaking changes from persistence point of view. So JSON related names should be the same.
Honestly, I will not approve such changes.
The goal is not to make the app nicest but keep it maintained and adding reasonable features.

@J0EK3R
Copy link
Author

J0EK3R commented Oct 4, 2025

One interesting part of code is the initialisation of all registered InputDeviceServices in InputDeviceManagementModule - perhaps there is a better way...

builder.RegisterType().As().As().As().SingleInstance()
.OnActivated(e =>
{
// resolve all registered input device services to trigger their registration
e.Context.Resolve<IEnumerable>();
});

It seems, there is a much better solution to instantiate objects at startup: IStartable

https://autofac.org/apidoc/html/5F3C297F.htm

@J0EK3R
Copy link
Author

J0EK3R commented Oct 4, 2025

One interesting part of code is the initialisation of all registered InputDeviceServices in InputDeviceManagementModule - perhaps there is a better way...
builder.RegisterType().As().As().As().SingleInstance()
.OnActivated(e =>
{
// resolve all registered input device services to trigger their registration
e.Context.Resolve();
});

It seems, there is a much better solution to instantiate objects at startup: IStartable

https://autofac.org/apidoc/html/5F3C297F.htm

Oh, yezzz - much better:

builder.RegisterType<GameControllerService>().As<IInputDeviceService>().As<IStartable>().SingleInstance(); // ensure it's started as soon as the container is built in Autofac

J0EK3R@0639939

@J0EK3R
Copy link
Author

J0EK3R commented Oct 4, 2025

I've startet renaming all the Controller-stuff (types, methods, etc.) to match the more common "InputDevice"-naming... Does it make sense - yes, I thing so. But maybe we loose the ability to load or shere old stuff (Database?, json serialization...?)
What do you think?

No breaking changes from persistence point of view. So JSON related names should be the same. Honestly, I will not approve such changes. The goal is not to make the app nicest but keep it maintained and adding reasonable features.

I have identified these types that must retain their name and the names of their properties - they are used in database and sharing context:

  • ControllerAction
  • ControllerEvent
  • ControllerProfile

I've renamed files and types ControllerTesterPage and ControllerTester and keep my fingers off the rest... ;)

@J0EK3R

This comment was marked as resolved.

@J0EK3R J0EK3R changed the title [WIP] InputDeviceManagementService InputDeviceManagementService Oct 8, 2025
@J0EK3R J0EK3R marked this pull request as ready for review October 8, 2025 04:14
@J0EK3R J0EK3R force-pushed the merge/vicocz/InputDeviceManagementService branch from 60e3025 to 53bf0e6 Compare October 8, 2025 12:31
@vicocz vicocz added the enhancement New feature or request label Oct 8, 2025
@vicocz vicocz added this to the 2025.6 milestone Oct 8, 2025
J0EK3R added 17 commits October 11, 2025 14:39
NotifyInputDevicessChangedAction  -> NotifyInputDevicesChangedAction
TODOs in
* ControllerProfilePageViewModel
* ControllerTesterPageViewModel
…lways is true as all inputdevice services support ControllerId now
to ensure InputDeviceService is instantiated and started as soon as the container is built in Autofac
@J0EK3R J0EK3R force-pushed the merge/vicocz/InputDeviceManagementService branch from 53bf0e6 to 8b8c9e1 Compare October 11, 2025 12:48
@vicocz
Copy link
Owner

vicocz commented Nov 4, 2025

Just FYI I plan to start looking this week.

@vicocz vicocz merged commit 108f953 into vicocz:default Nov 24, 2025
1 of 3 checks passed
@vicocz
Copy link
Owner

vicocz commented Nov 24, 2025

Thank you for your contribution :)

@J0EK3R
Copy link
Author

J0EK3R commented Nov 24, 2025

You're welcome. Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants