Refactoring week

So, I'm still working on integration of save files into recording file. On last week I did alot of things to this. After code studying, I saw that every engine supporting states loading, have some kind of fucntion which generates file name by slot's number. Some engines have this function as is a class static method, others like public method, few engines have this function inlined to code of saves-related functions. In other words, we have same behavior and different interface. So after discussion with mentor we decided to unificete interface of save files name generation and make all engines compatible with this interface. Then we produced some requirements to name generation function:

1) It should be accessible without class instancing. Because MetaEngine use it for removeSaveState and querySaveMetaInfo functions.
2) It should be accessible inside of class methods (obviously)
3) It should be accessible outside of class using the pointer of abstract class Engine. It to make possible to use it from eventRecorder
4) Logic of name generation should be in one place. Because it's possible to get error relate to duplicated code. For instance, in current version of queen engine, function QueenEngine::makeGameStateName uses constant string "queen" and function QueenMetaEngine::removeSaveState uses variable target for name generation. This may cause a problems.
5) Preferably to have a fucntion which check number of saving's slot is special (for ecample used for autosave or recording file) and generate name without depending to engine implementation

As most engines generate file name by formula fileName = preffix + "." + pattern, where preffix is a name of engine and pattern is function from slot number (for example "%03d"). Firstly I thought that it would be a nice idea to make a universal method in Engine class. Parameters of this function would be preffix (usualy target name) and pattern to generate extension of save file. This method could look like Engine::getSaveName(prefix, pattern, slot). But some engine need different algorithm of names generation and each time we call this method we should pass to many params to it, which is not very comfortable and can be cause of errors.

Then I decided that it should be good to make and AbstractNameGenerator class with abstract generateSaveName method, and inherited it for each engine. You can see it on diagram:

I would be consistent to all requirements, but create class without state which contains only one method is definitely overengineerng.
So, we decided to make static method and wrap it by virtual function in each engine. And in every virtual function call checking for special slot.

And then fuzzie suggested to  wrap the SaveFileManager's openForLoading function, for the event recorder. And this idea looks very attractive, cause at first, it can be realized without changing of existing codem and at second it corresponds to the logic of other eventrecorder's systems. So at the moment I'm working on implementation this idea

Savefiles embedding

It's needed to make a recording self-sufficient and independent from save files. At the moment, user should  make following sequence of actions to playback loaded game:
1) choose game
2) select playback record
3) press "Playback" button
4) in pop up dialog box select is it need to load from savegame.
5) if loading from savegame choosed, show load game dialog

After I implement this feature, 4th and 5th steps will be removed. Also, will be sufficient only recording file to playback the gameplay. Will not be necessary to share save file.

At first glance, it's easy to implement this feature: just current save file during the recording and give it to
engine during the playback. But it's not so easy to do, using the current engine API.

First of all, every function related to saving and loading works only with slot numbers and save files name generation is different for each engine, so our function can't get filename of current save. I've tried to solve this by adding the virtual funtion getSaveFileName to engine class. Anyway, this function is in every engine in different forms: somewhere it part of loading function, somewhere it's separate function, but it exists in any engine. So, i think it's a good idea to make an unificated interface for it.


Next issue is to load savefile from recording. As I said above,  we can load savefiles only by their slot number. There was to  ways to solve it: write procedure which loads game state from given stream or create some virtual slot used only for recordings. First way requires to large code rewriting, so we chosen second.

I've almost implemented it, and now I'm doing testing and fixing a bugs. Hope to show a demo soon.

Nothing special

This week I've just did little code refactoring and moved all playback file related issues to separate class. Also, following my mentor's advice, I implemented cache for recording dialog. Now It reads every file header before adds it to list, store file's header to array and every time when user update selected record, it doesn't need to reopen the file.
What I want to implement now it's to keep the game save in playback file, i.e. if we load file before recording, it shouldn't ask what record you want to load. It should load it automatically. I'm not sure is it possible, but it may be great feature.

GUI feautures of event recorder

Last week I've added opportunity to view playback file screenshots directly from GUI. How it works you may see on the following video:
http://www.youtube.com/watch?v=trqPXKsAaCs

Also I've added dialog window to edit info about author, visible name and description of playback file.
http://www.youtube.com/watch?v=trqPXKsAaCs

During the work by GUI we've desided to move all operation with file to separate class. It should provide high flexibility and make possible to simple access to playback file. So I've added class PlaybackFile which works with file and provide higher level of abstraction.

Recorder GUI

It's time to make gameplay recording more user friendly, cause rewrite config time every time when we want to record or to playback isn't very fun. So we've begun work on GUI for recording and replay. First of all, I once again changed initialization routine of recorder, totally removed it from engines and call it at same time when the user choose game, so it became even better and transparent.
Then I've change naming of records. Now name for recordfile generates as <game>.rxx, where xx is number of recordfile. For instance: kyra1-cd.r11
Use cases for choosing records are very simmilar to loading from savefiles. So recorder dialog going to be very simmilar to load savefiles dialog:

At the moment, it have minimal functionality: it can record gameplay to new file, playback choosen record and delete existing records. Next week I want to add few more interesting features :)

Almost works

This week I made easy integration of recorder to engine. Now, engines inherited of AdvancedMetaEngine initialize registrator automaticaly. Few other, inherited from MetaEngine (SKY, SCUMM, SWORD1, SWORD2, QUEEN) can be initialized by adding only two strings: 

#include "common/eventrecorder.h" g_eventRec.init(gameName());

I've tried registrator on number of games and at first view it works well. So, seems like it almost works and will ready to testing soon.

Making things better

On this week, I fixed architecture mistakes in event registrator. When I've enabled registering of all time events (when somebody whant to know about system ticks) there strange mistakes occurred and it didn't work at all.  So, I've enabled registering time events only in some places of engine. But even in comments to my gsoc proposal, LordHoto noticed that need something to do with it, because when different parts of VM see different time, this will cause problems. In addition, direct replacement of g_system->getMillis() by the g_eventRec.getMillis() is bad, untransparent, unflexible, etc. At last, after one of commits, my mentor _sev proposed me to liquidate this dirty hack, by getting rid of all g_eventRec calls and doing registration of all g_system->getMillis() calls.
First problem we have found - events might be recorded before full initialization of registrator. It was fixed by simple  flag adding which shows that the registrator completely initialized.
After I solved this problem, I tried to find another bugs during at about two days. Something went wrong, but I couldn't find what exactly. _sev very helped me when suggested to use backtrace macros which will show who and when call getMillis fucntion. I've begun to write this data to log and analyze it. This helped me to find next two problems:
DefaultEventManager::pollEvent called more often durring the playback. And also it calls if user doing any action (moves mouse, presses keyboard keys). I solve this programm by adding a stop and start registrating feature  to registrator class. Now it just skips time messages from pollEvent function.
Last problem was hard to find, but very easy to solve. To describe a problem I should tell how we've developed wrapper for audio registering.
Original audio subsystem can be represented by this sequence diagramm:

I. e, SDL audio subsystem give mixer manager to know about it's readiness to play next chunk of sound and give buffer which must be filled by sound data. Manager give this event to platform-independent mixer which must to fill this buffer. Mixer check is there any free chanal and give this buffer to it. Chanal fill the buffer by sound data and ask the current time from system. System ask for time from events registrator.

This scheme worked perfectly, but had a little problem: it was very hard to synchronize sound with logic. Wasn't possible to determine how often will be calback called and what will be buffer size. _sev proposed to get rid of the dependence from  SDLInternals and replace SdlMixerManager to own wrapper. Obviously, there was needed to use some external events source which  will set how frequently mixer's callback will called. Also, this source must have possibility of sinchronization with registrator. We decided to use processMillis for this purposes.   Regarding to this idea, sequence acquired the following form:

Very easy to see that this sequence resulted in an infinite loop and stack overflow. We decied to make a bypass to disable events recording while mixer's callback executing 

Now, in case of necessary, registrator taken out from sequence and system time became the result of function. It saved from stack overflow and worked quite well. When I began to register all calls of getMillis, problem has arisen where I did not expect it. There is function Chanal::getElapsedTime, which returns the time elapsed from begining of sound playing. It getting first time period in Chanal::mix function (during it bypass works and time is real from system). Another time it gets from events recorder. So difference between them is undefined and different for each replay. After problem was found it was very easy to fix it and make result of getElapsedTime function positive and predictable.