VideoCommon/Statistics: Remove global system accessor from s_after_frame_event #12546
Conversation
…ame_event Instead, we make the event take a reference to the system and then pass it in when the event is triggered. This does introduce two other accessors, but these are much easier to refactor out over time, and without modification to the existing event interface.
Potentially avoids reallocations if the capture buffer of the callback is quite large.
|
Doing this for one event that touches one or two places that use system seems unnecessary unless we're trying to be forward thinking (most of the callbacks do not use system, maybe they will later?). And if we're being forward thinking, maybe it'd be worthwhile to add them to all events? |
|
Apologies, maybe you are trying to make smaller changes. I probably could contribute too.. Would you like me to open a PR for the other events? |
|
I'm not sure if this is the correct way to approach this. Consider what happens when two running Systems exist: Any event that is added during Initialize() or similar (rather than statically on program start) would get multiple copies of the event registered, and every one of them would then be called when any one System fires the event, no? I think what actually needs to happen here is that the HookableEvent itself needs to be de-globalized and then every System gets one instance of it per event type. Granted, even then it would probably be useful to pass the System so it doesn't need to be captured, similar to the MMIO handlers. So I suppose in that sense this makes sense. |
This is part of the gradual effort to remove the global use of the system instance and other related facilities. I don't think leaving it using a global is a good idea when there's a perfectly fine way of resolving that (passing it in). I don't really see what the issue is, considering the core timing callback events work the same way and several of them also don't use the system instance.
We realistically need to even get to that point first to adequately design around that case fully. And even then, pulling globals out of the events is better, because when designing around that, all you'd need to do is look at the Trigger() sites or event aliases to see what needs to be managed, rather than inspecting all the callbacks manually |
I thought the plan was there would still be one system but that it would be instantiated by the program instead of statically to better handle lifetime? I wasn't aware there was a plan to have multiple. If there are, then I agree we shouldn't pass it through the event. Though then would it make sense to store |
Fair point. Perhaps I'm getting a bit ahead of myself here. |
I just wanted to understand the approach here. I guess it makes sense to add system to callbacks when needed. The interface symmetry was already broken with the |
If the calling code can instantiate a System then what's stopping it from instantating two? |
Instead, we make the event take a reference to the system and then pass it in when the event is triggered.
This does introduce two other accessors, but these are much easier to refactor out over time, and without modification to the existing event interface.