Open up the DebugOverlay to add/remove MetricsModes

[emanuele3d]

Introduction

I was recently experimenting with a new MetricsMode for the DebugOverlay. I was aiming for a generic solution where objects for the retrieval, filtering, sorting and formatting of the data could be provided to the mode upon construction or even at runtime. From an architecture/coding perspective this wasn't difficult.

In practice, the setup of such objects was becoming an issue. Right now modes are quickly instantiated with a short "new MyMode()" instruction within a list initialization in the DebugOverlay class. Conversely, setting up a more generic mode in the worst case scenario would require something like:

GenericMetricsMode myMode = new GenericMetricsMode();
myMode.setDataRetriever(new ExecutionTimeRunningMeansRetriever());
myMode.setDataFilter(new StartsWithFilter("MyString::"));
myMode.setDataSorter(new AscendingAlphabeticalOrder);
myMode.setDataFormatter(new CustomFormatter());

Data-processing objects could be reused by multiple modes, but would also require additional lines to instantiate them. Permutations in the configuration of modes would have to be dealt with by instantiating and adding new modes, i.e. the same code above with a DescendingAlphabeticalOrder instead. Eventually part of the DataOverlay class would be peppered with this stuff.

Incidentally, this also got me thinking that if a module wanted to add a custom mode at runtime, it can't.

Proposal

I would therefore suggest for the DebugOverlay class to provide methods to dynamically register and unregister objects implementing MetricsMode, so they can be added to the list of modes to be cycled over. This would allow modules to add their own modes. Furthermore, mode-configuration code would stay out of the DebugOverlay class and remain a responsibility of whatever code registers the a mode. Specifically I would add the following methods:

public Object register(MetricsMode mode);
public void unregister(Object receipt);

Notice the use of a simple receipt object, returned by the register() method, to unregister a mode. I'm not sure if this is the best way to go about it but the idea is that only the code that has registered a mode or code that has been given the receipt can unregister a mode. This is a pattern I've seen in the OSGI framework to register services, but perhaps there is a better way using the engine's security features?

Runtime Re-configuration?

A final issue that occurred to me in this context, is that it would be nice if some characteristics of a mode could be changed at runtime. I.e. the sorting order of a mode flipped from ascending to descending. Or a number of filtering criteria cycled over. From a specific mode's perspective this shouldn't be too difficult: dataSorters and dataFilters can be simply replaced or a list of them can be mantained and cycled over. The problem is, how can the user trigger the change? I.e. would it be possible for a mode to assign a "toggle sorting order" meaning to the CTRL-F4 key-combination?

[immortius]

1) This pattern

public Object register(MetricsMode mode);
public void unregister(Object receipt);

Is a bit silly to me, would just go with:

public void register(MetricsMode mode);
public void unregister(MetricsMode receipt);

Anything that could hold onto the "token" could just as easily hold on the the metrics mode itself. Not sure what the receipt object achieves over that (maybe avoid holding the object in memory, but a WeakReference would also achieve that).

2) Probably the only question is whether the externally registered modes would survive the debug overlay being closed/opened. Otherwise go for it. The debug overlay is what it is because that was the minimum required to achieve its goals - if you have the time to improve on it why not.

[emanuele3d]

Did you mean:

public void register(MetricsMode mode);
public void unregister(MetricsMode mode); 
// above you wrote "receipt" here - doesn't matter really I think I understood: you want to use the same object

But I agree with you that the use of a receipt is silly in this context. In the OSGI frameworks it makes sense because one can also browse through services: with your proposal I'd only need to obtain a service to be able to remove it. But in our case I didn't think (d'oh!) that the MetricsModes available to the DebugOverlay are stored in a private list and there is no way to access them.

Regarding point 2, I'm tempted to say this line creates an actual instance of DebugOverlay. I tried to quickly go through the NUI code to verify but it's all a bit too alien to me. So, as long the the DebugControlSystem is instantiated and initialized, the DebugOverlay instance lives too: pressing F4 until it disappears actually toggles a NullMetricsMode, but the overlay instance still lives. What I can't find is the DebugControlSystem's lifecycle! Where does it get instantiated?

[immortius]

DebugControlSystem is annotated with @RegisterSystem, so it is automatically created as part of a game load, and disposed when a game ends.

[emanuele3d]

Ah! Those pesky annotations. I'm not used to them and I miss the valuable information they contain.

So, within the context of a game the MetricsModes are always alive, but as soon as a game is unloaded they need to be disposed. I can see the BaseComponentSystem class has a shutdown() method. DebugOverlaySystem could override it and within it call a new DebugOverlay.dispose() method which in turns could call a new MetricsMode.dispose() method for all MetricsModes registered.

Sounds good?

[immortius]

Do they need to be disposed? I would have thought garbage collection would have sufficed for those.

[emanuele3d]

Do they need to be disposed? I would have thought garbage collection would have sufficed for those.

At this stage, for my own specific "renderland" needs, I'd cautiously say you are right. But here we are discussing about opening this functionality to modules adding their own potentially complex MetricsModes. In that context I thought a dispose() method might be beneficial.

Should we follow a strict "if it just might be used we do not bloat the code with it"? Or is there some other rule of thumb that defines some kind of threshold criteria beyond which it makes sense to have a code feature, even though there are no immediate plans to take advantage of it?

[immortius]

I guess the rule of thumb is whether there is a known or likely use case for it. There are very few things modules can interact with that need disposal, and most of those are disposed during environment switches anyway (e.g. assets). I guess there may be a need if metrics modes are added and disposed during a game run (doubtful).

Possibly it would make sense to make the debug screen a container for a bunch of metrics screens, so that what is being plugged in are UIWidgets - which already have methods for disposal.

[emanuele3d]

Possibly it would make sense to make the debug screen a container for a bunch of metrics screens, > so that what is being plugged in are UIWidgets - which already have methods for disposal.

I like this idea as it would potentially open the door to non-textual widgets. I already have enough of my plate however and I won't go further in this direction at this stage though. Also, given your feedback a disposal path is not yet warranted and I'll leave it to when the need arises.

I will however submit a PR to add the methods

public void register(MetricsMode mode);
public void unregister(MetricsMode mode); 

to the DebugOverlay and to make the DebugOverlay instance somehow retrievable at runtime. I imagine this should happen through the ingame GameState-specific context now.

[emanuele3d]

I'm thinking this issue should be closed as it has been addressed by PR #2316 (thank you @tdgunes!).

In favor? 1. Against? 0. Abstained? 0.

The Ayes have it, the Ayes have it.

Who said democracy is messy?