search

realitycollective / com.realitycollective.service-framework

The Service Framework package components for the Reality Collective. This package an extensible service framework to build highly performant components for your Unity projects.
https://service-framework.realitycollective.net
MIT License
9 stars 5 forks source link

Remove Enable(), Disable() and IsEnabled from services #101

Closed FejZa closed 5 months ago

FejZa commented 5 months ago

Reality Collective - Service Framework Feature Request

At this time the IService interface defines a IsEnabled property and Enable() as well as Disable() APIs. However, those have never been implemented properly and are confusing as hell.

Example 1

When a service is initialized by the manager, its IsEnabled is not set to true. So essentially although all services registered with the manager are running, they are not enabled by definition.

Example 2

A disabled service will still receive all Updates every frame.

Example 3

A IsEnabled status does not make sense for services anyways. A service is a piece of business logic that usually provides frame independent APIs to manipulate the application. There is some cases, where a service may be "enabled" or not. But that is up to the application to implement properly as it is strongly driven by the appliation requirements.

Example 4

Calling Enable() / Disable() on a service will NOT change its IsEnabled property!

Example 5

The current implementation is supposed to mimic a MonoBehaviours enabled state, it fails miserably at this though. And again, a service is NOT a MonoBehaviour and as such it's fine for it to not have those APIs.

I could keep going. Generally what I am saying is, the current implementation is broken and very confusing and IMHO the most straightforward and best solution here is to simply remove it.

FejZa commented 5 months ago

@SimonDarksideJ thoughts?

SimonDarksideJ commented 5 months ago

As the implementation currently depends on the use of the base implementation of an IService, this is "implemented", but in review is not documented clearly.

Additionally, in the years since its implementation, I too have not had call to "disable" a running service as it is far simpler to just unregister the service and guarantee its state (relying on the Unity Disabled/Enabled events is not practical)

So in review (and discussion internally) the best option is to simply remove this capability in favour of more used routes.