X2CommunityCore / X2WOTCCommunityHighlander

https://steamcommunity.com/workshop/filedetails/?id=1134256495
MIT License
60 stars 68 forks source link

Investigate registering listeners on the shell menu #297

Open robojumper opened 6 years ago

robojumper commented 6 years ago

Currently, X2EventListeners are registered in tactical, and in strategy. The fact that they don't work on the shell screen was a drawback we accepted whenever we decided to use an event hook instead of a DLC hook.

TLE moves some UI to the shell that may be relevant for event hooks (dynamic class icons, for example). We should investigate whether registering on the shell is wanted.

GingerAvalanche commented 6 years ago

What are the downsides to using a new boolean RegisterInShell for the CHEventListenerTemplate (or even for X2EventListenerTemplate) and then registering event listeners with that boolean set to true, in OnInit of UIShell or UIFinalShell, and then removing them prior to unloading the shell level?

robojumper commented 6 years ago

Update: This seems somewhat difficult, as there are lots of places on the shell menu where the history is reset, clearing all registered listeners. Probably better to wait for a real use case or problem before attempting this...

Iridar commented 4 years ago

Well the Issue #783 would be a real use case, I even have the Event done and set up. I'm perfectly content with changing it to a DLC hook, though. I'm not sure if there's something that an Event would accomplish, but a DLC hook would not, in this specific case, since there's all event listeners would have to use ELD_Immediate to take advantage of it anyway.

Xymanek commented 4 years ago

Somewhat related: personally, I think we should register strategy listeners before calling CreateStrategyGameStart - the setup process may be invoking CHL extension events which are currently getting "lost"

Musashi1584 commented 4 years ago

I have a new "real use case" with events for the mcm actions in my mcmbuilder that should work in the shell too. @robojumper is this something you would consider to pick up again?

robojumper commented 4 years ago

Conceptually, the shell is much closer to strategy than tactical. Are there any drawbacks to just registering strategy listeners on the shell?

Xymanek commented 4 years ago

Strategy listeners can expect things like HQPRES to exist, which would no longer be the case

Musashi1584 commented 4 years ago

Just tested calling class'X2EventListenerTemplateManager'.static.RegisterStrategyListeners(); in a UISL on the shell enables strategy listeners to receive events.

Musashi1584 commented 4 years ago

Strategy listeners can expect things like HQPRES to exist, which would no longer be the case

wouldn't none checking that be in the error handling responsibility of the listener code?

Xymanek commented 4 years ago

Why would you check for existence of global singletons?

Musashi1584 commented 4 years ago

Why would you check for existence of global singletons?

Cause there are obviously situation where they are not there

Xymanek commented 4 years ago

Please state an example of HQPRES == none while in strategy

Musashi1584 commented 4 years ago

https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/blob/d80a587a67ecdc37707f26ad1b1095e8d60623d4/X2WOTCCommunityHighlander/Src/XComGame/Classes/XComHeadquartersCamera.uc#L74

Xymanek commented 4 years ago

That is an example of a check. It might've been wrong, for old code, called too early into strategy setup or many other reasons.

What I'm asking is an example scenario which would cause HQPRESS == none for a "generic"/"normal" bRegisterInStrategy listener

Musashi1584 commented 4 years ago

The point is you can also listen to events beside event templates which have no bIsStrategy so perse there is no guarantee when a listener is called. I see your point and I am not against a new bIsShell or whatever. Just saying making assumptions in code always having the possibility of error when code changes or the assumption is not true for an edge case.

pledbrook commented 3 years ago

So the two linked issues only need to have listeners registered in time for campaign initialisation, not necessarily as soon as the shell menu is displayed. I don't know if that would suit Musashi's use case, but it should certainly be pretty straightforward to implement.

I would also like to voice my support for having a new type of listener that isn't either strategy or tactical. I think there's a strong chance that registering existing strategy listeners before a campaign is initialised will break some mods. Unless we can guarantee that none the existing strategy events are fired at an earlier point than they currently are.