EventHandler::OnRegister no longer called upon loading save

[Player701]

Starting from some commit after 3.7.2 - probably this one but it's uncertain because some of the commits, including that one, crash when starting a map with the test WAD loaded - EventHandler::OnRegister is no longer called upon loading a save file after the corresponding EventHandler has been deserialized. This is a breaking change: my mod relies on OnRegister being called again to verify that the save file that has just been loaded was made with the same version of the mod that is being run now, and if it's not true, then a warning message is printed. This is achieved by storing the version string in the event handler itself, and comparing it with the version string retrieved from a static class included in the mod. Obviously, this feature no longer functions after the change.

Test WAD attached. In GZDoom 3.7.2, a message is printed to the console both when a level starts and when a save made on that level is loaded. In the latest git, no message is printed upon loading a save.

UPD: I have reimplemented the version check as a separate EventHandler method invoked from another (static) event handler in its WorldLoaded event. However, it would still be better if the original behavior could be restored.

[Graf Zahl]

Crap. Trust the modders to use things in a way that ensures that code needs to remain broken. OnRegister gets called before things are fully set up, it's only meant to set some internal properties for registering.

The problem here was that the event manager did not properly serialize its data so it ended up rebuilding the entire handler chain on reloading - and as a side effect of this, things got called that shouldn't have gotten called, among them OnRegister. The current implementation just serializes the list links and implicitly rebuilds it, thus not needing OnRegister anymore.

This is going to be one lousy hack to be added back...
I have to wonder how many mods depend on it.

[Player701]

Like I said, I needed it to verify that the version of the mod is still the same after loading a save file. Since static event handlers are not serialized in save files, I needed a non-static handler to store the version string and compare it upon loading. However, I have since reimplemented this routine with another method which does not rely on OnRegister being called again.

I have no idea if there are other mods that rely on the old behavior, but modders will have to be aware of these changes if it is decided that they will be kept in the next release.

[Graf Zahl]

That's the curse of ZScript. At some point it will be inevitable to really clean house and break stuff - if these unfixable bugs keep piling up the engine is going to break down eventually.
The event handler system was a particularly big problem. When this was submitted I had no time to do a proper review and trusted that it's all good. Sadly it had some fundamental design problems I overlooked which prompted a recent rewrite of its internal organization.
The biggest problem was that both static and non-static handlers were in the same list which rendered the serializer code quite broken, needing horrible workarounds. Splitting up this list and fixing the broken serializer was one of the things I did and this is the result.

I think what should be done here is deprecating OnRegister, restore the call after loading the savegame and provide a new method, called OnCreate, to do it properly. The thing is, modders who do not know about this old quirk of OnRegister are in for a rough ride if they use that function for internal setup and then fall victim to a savegame reload killing all their values.

[Rachael]

I still think that a code rewriter to fix broken code is a viable option. That way, if people pull such shenanigans then it can either rewrite it to do it the right way or disable the code in question (if non-vital) or in worst case scenario, I_FatalError() out.

[Graf Zahl]

If it was just harmless checks like this one, sure. But as things stand I'd basically have to redo large parts of the UI code (status bar excluded, fortunately, because when I did that I already learned how not to do it in order to avoid bad use.) All of the menu, the level summary screen and other exposed parts of the UI suffer from relying on the generic Screen.DrawText function which makes it extremely difficult to sanitize the 2D drawing without causing a buttload of problems for their custom content.

I think we're going to see this with some mods and the current menu rework. None of the ones I checked created their own option menu item drawers but rest assured that some actually did, and now won't look right anymore.
This is also why I am a bit concerned about fixing the global scaling variables to be floats. This can cause some serious problems with value types in function calls. This needs to be fixed because with the current font the system is no longer working as intended. It had been designed for that tiny SmallFont that comes from the games.

[Rachael]

After reading that I had an idea (just now, for some reason not before when you first brought this up) to rename the global scaling values something else entirely (i.e. FScaling) and have a function that when it's changed, it updates its corresponding integer value for compatibility purposes (but would be locked to an integer and would always be 1 or greater).

That gives you something to work with while transitioning to the new system internally, and it gives a compatibility method for mods that still use it.

[Graf Zahl]

That would not work. If you have a scaling factor of 0.5 you can't get anything proper with both 0 and 1 as the nearest integers. One is twice the scale the other is a single spot on the screen.
These values only do what they do because they are unifiorm for an entire screen (or at least for a relevant subsection of it.)

I think the only way to do it would be to give the UI a fake screen size and have those constants always be 1, but that might cause problems with status bars that mix both dedicated and generic drawing methods, but I guess at that point one has to say that a line has to be drawn somewhere. But such an approach would move the control for scaling back into the native code removing a wide open field for design errors. This would mean if you had a 1920x1080 screen and a UI scale of 2, the UI would think it's operate on a 960x540 screen, but in order to pull that off it'd also be necessary to force a uniform UI scale instead of the separate sliders and render all texts with a font run through a 2x upscaler with texture filtering so that it also works on scales that are not full integers.

[Rachael]

That actually seems like a better way, to be honest.

[Graf Zahl]

Yeah, I think I'll do that before the next release, even if it delays it by two weeks. It'd be worth it and also remove a lot of issues with 320x200 (at the cost of blurry fonts, of course.) One good thing is that if we decide to uncouple the HUD scale from the 3D view it'd be fully transparent then - the mod will never be able to retrieve that value anymore at all.

[Nash]

Graf, that all sounds good. Looking forward to it. :)

[Blue Shadow]

From here:

It should be said in no uncertain terms that OnRegister operates on an uninitialized level so it should only be used for setting up the registering process of the event handler itself and nothing else - not even the event handler's data!!!

To leave no room for confusion, does that include initializing the handler's own member variables? If so, where could that be done, then?

[Graf Zahl]

Since OnRegister is also being called after loading a savegame, it'd overwrite all its variables after reloading. Guess why I consider this problematic. Too bad that people depend on this misbehavior...

Currently there is no way to reliably set up an event handler only once. You'd have to use a flag variable as guard.