Implement AGSSpriteFont plugin for Clifftop Games

[criezy]

This pull request does mainly two things:

• Register the AGSSpriteFont plugin (since currently it is not used and stub functions are used instead).
• Implement the variant of the plugin used for Clifftop Games (Kathy Rain and Whispers of a Machine).

It also fixes some issues with the AGSSpriteFont plugin, such as memory leaks.

I am aware of PR #834, but I used a different approach in the implementation here: I added two new font renderer classes that derive from the unmodified ones of the original plugin and implement the modified functions only. The advantage of this approach is that we can either instantiate the base renderers or the modified ones depending on the game. However that is something I did not actually know how to do (see f89a2f5) so any guidance on that point would be welcome.

This is the same approach I used previously in ScummVM to implement the Clifftop Games variant of the plugin and most of the changes in this pull request is borrowed from my previous work. Sadly the approach we used in ScummVM to select the renderers is probably not applicable here since it relies on the game detection system specific to ScummVM.

Another impact of this design decision is that I had to make the IAGSFontRenderer destructor virtual so that the destruction of the renderers in the plugin works properly.

I updated the Visual Studio and Xcode files, but this was not tested and I don't know if this is correct.
The changes were only tested on macOS using cmake.

And finally here is a comparison with Kathy Rain:

Current AGS code:

With the changes from this pull request:

[ericoporto]

@criezy the build errors are the missing objects in this file: https://github.com/adventuregamestudio/ags/blob/master/Plugins/AGSSpriteFont/Makefile-objs

I think just adding SpriteFontRendererClifftopGames.o and VariableWidthSpriteFontClifftopGames.o should work.

[i30817]

Time to delete the hacked up plugin i had to make this 'work'.

BTW, there is another place than dialog font where this plugin effect is apparent (iirc, it's been some years). In the ending zone of the game, after the puzzle with the biker ghost-monster, there is a puzzle with refrigerator and magnets. It used letters, which were invisible (iirc) with the old plugin.

[criezy]

To make sur this is not missed: this pull request is currently missing one crucial peace of the puzzle to work with Kathy Rain and Whispers or a Machine: there are two variants of the font renderer in the plugin and the plugin needs to know which variant to instantiate. Currently it always instantiates the original version. For the screenshot I included I just changed the code to always instantiate the Clifftop Games variant of the renderers, but that is obviously not a good solution.

[criezy]

I have added one more commit to properly use the Clifftop Games variants of the renderers for Kathy Rain and Whispers of a Machine.

The renderer selection is based on the game name and uses the Game::get_Name plugin function. But because the game name is not yet set in the play GameState struct when AGS_EngineStartup is called for the plugins, I had to delay the renderers instantiation.

I don't find this solution very clean, but I could not think of a better one.

[ivan-mogilko]

The renderer selection is based on the game name and uses the Game::get_Name plugin function. But because the game name is not yet set in the play GameState struct when AGS_EngineStartup is called for the plugins

I could see if it's okay to move the "play" struct initialization earlier; but I wonder if it's a good idea to call script functions in AGS_EngineStartup, as the scripts nor game loop itself is run at the time. Also, Game.Name is a non-unique settable property and may be freely set or even changed in script at runtime (it's rather a human-readable title than an ID).

Since this is about porting/enhancing plugin anyway, an alternate solution could be to expand the plugin API, either pass some info on startup (game's GUID, etc), or add an API method to retrieve an extendable info struct from a game. For instance we added a new GetRenderStageDesc method recently for the purpose of better supporting 3D plugins. We might add one for this plugin, or any other that would need such information.

[criezy]

Since this is about porting/enhancing plugin anyway, an alternate solution could be to expand the plugin API, either pass some info on startup (game's GUID, etc), or add an API method to retrieve an extendable info struct from a game.

That sounds like a better solution. I will take a look at that in the next few days.

[ivan-mogilko]

Another impact of this design decision is that I had to make the IAGSFontRenderer destructor virtual so that the destruction of the renderers in the plugin works properly.

Regarding this... I might need to double check this, but if I recall right, we intentionally kept the interface classes of the plugin API without virtual destructors, as making them virtual would also change virtual tables of such classes, which in turn could break compatibility with older plugins. The decision was to not use pointers of the interface class's type when deleting an object. (Engine cannot do that anyway, as it does not know what kind of memory the pointer points to: an object on heap, an object on global stack in plugin, etc.)

Perhaps the alternate solution could be to change the class hierarchy within the plugin, and create its own parent class with a virtual destructor.

[criezy]

Another impact of this design decision is that I had to make the IAGSFontRenderer destructor virtual so that the destruction of the renderers in the plugin works properly.

Regarding this... I might need to double check this, but if I recall right, we intentionally kept the interface classes of the plugin API without virtual destructors, as making them virtual would also change virtual tables of such classes, which in turn could break compatibility with older plugins.

I reviewed that change, and I did not actually need to make the destructor virtual in IAGSFontRenderer. It was sufficient to make it virtual in the standard font renderers of the AGSSpriteFont plugin. The custom renderers for Kathy Rain and WOAM derive from those already. I have now forced-pushed a change to remove the changes to the IAGSFontRenderer class.

I have also pushed another two commits to add a function in the plugin interface to get game info, and to use that in the AGSSpriteFont plugin to determine which renderers to instantiate. If this looks like a good approach I can squash commits 885a3d7 (the old approach) and 6814b60 (the new approach).

[ivan-mogilko]

@criezy it looks fine, but could you also add guid and uniqueid fields to the struct (uniqueid is an older integer game id, used before guid was added)? In my opinion, it is safer to test the game by guid if it's available rather than the title.

Perhaps I will be able to give this new plugin a quick test in a few days.

[ivan-mogilko]

Hmm, ah, don't know why I did not think about this earlier... But maybe this plugin could expose a switch between renderers into the script. Because right now this switch will be hardcoded. So it's good for running with pre-existing games, but becomes useless for making new ones.

I guess this is something for the future TODO list though. Then again, maybe we'll incorporate this plugin's functionality into the engine at some point.

[criezy]

could you also add guid and uniqueid fields to the struct (uniqueid is an older integer game id, used before guid was added)?

Done

In my opinion, it is safer to test the game by guid if it's available rather than the title.

I also made that change.

[ivan-mogilko]

Alright, I've built tested this with "Kathy Rain" and "Woam" on Windows, building from MSVS solution.

"Kathy Rain" is working fine, from what I see, so this PR definitely works!

"WoaM" has some problem with the text height calculation, apparently. In the main menu the option texts are cut from below, the line spacing is too small, and the speech text is missing bottom parts of the outlines (I think?):

Do you also have same problem? what about scummvm?

I don't know if this is related, but with "WoaM" there's another problem where engine does not handle its screen size correctly. We had a ticket about it, but in the end decided to not fix this, as it's related to their custom engine: #868. So, in theory, this could be not the plugin's problem but the engine's compatibility problem.
(The mentioned issue could be still "fixed" by providing an overriding config option that enforces certain native resolution, as I proposed in the old ticket)
EDIT: forcing the native resolution to 640x360 does not fix the above text problem, so it might be unrelated.

---

In regards to MSVS solution, it has to be upgraded, and is missing couple of include paths, but it's not a big deal and I guess we may fix this separately.

[criezy]

"WoaM" has some problem with the text height calculation, apparently. In the main menu the option texts are cut from below, the line spacing is too small, and the speech text is missing bottom parts of the outlines (I think?):

Do you also have same problem? what about scummvm?

I indeed see the same problem. I also checked with ScummVM and this appears to be a regression in the engine code.
ScummVM 2.5.1 is displaying the text correctly:

But with current code from ScummVM master I see the same issue you saw:

Between those two versions dreammaster and myself have merged a lot of changes from the AGS engine in ScummVM. I will do a bisection to find what is the culprit (it will be easier than to do it on upstream AGS since at least in ScummVM we have a AGSSpriteFont plugin that worked).

However the issue with the menu is specific to AGS:

And in current ScummVM:

The first image seems familiar though, so this is likely an issue that we had in the past in ScummVM as well, but have fixed in the engine since.

On the same screen I also see the version text is cut-off in AGS but not in ScummVM.

However there is a regression in ScummVM on the load screen:
ScummVM 2.5.1:

Current ScummVM (and AGS):

I don't know if this is related, but with "WoaM" there's another problem where engine does not handle its screen size correctly. We had a ticket about it, but in the end decided to not fix this, as it's related to their custom engine: #868.

This is not related. We have this issue in ScummVM as well (and have not fixed it), but as mentioned above the text display used to work.

In regards to MSVS solution, it has to be upgraded, and is missing couple of include paths, but it's not a big deal and I guess we may fix this separately.

Since I don't have Windows I did the updates blindly in a text editor for it. So I am not overly surprised I missed some things.

[ivan-mogilko]

Well, I guess we may merge this then, since plugin itself seem to work. Then fixing these remaining problems may be done separately in the engine. They probably are related to text height and line spacing calculations, which were adjusted within 3.6.0.

[i30817]

That game also has a problem in the 'abilities' that you can see in the intro scenario. If you click on for instance, the analyze ability on a hotspot the text appears in a completely different place from where the popup menu is. It's a bit awkward. Maybe it's that resolution issue linked.

[i30817]

@criezy , this was recently fixed here (the previous fix had some problems), as well as the custom resolution. Could you downstream the fixes to scummvm, or are you waiting to see if it all settles down a bit, since there are still some minor changes happening?