With latest revision it is not only possible to view campaign bonus information when clicking “show scenario information” during a campaign game, but to restart or to start another map with different bonus / difficulty settings which was not available in OH3.
There is still a bug when you want to start another campaign map. It crashes during the loading screen. It’s really confusing, e.g. it’s not possible to restart campaign outside the SDL event loop which resides in CMT. (crashes somewhere later in player interface / update) Starting a new campaign map is not possible because the loading screen needs a pregame instance (there are so many hidden dependencies). I’m getting into the mechanics of VCMI gui handling / server-client architecture / threading model / player interface,… more and more while fixing bugs or implementing missing features. I try to keep refactoring to a minimum, understand the API and to use it the best way.
There is one major thing what could be done better, this is event handling / threading (especially synchronization) / processing net packages. It works fine now, but it’s complicated. I think single-threaded client and server processes would be possible. Therefore processing of net packages needs to be asnychron. We would need threads for loading and AI only. There is one event processing machinery which processes user input and incoming net packages. No need for synchronization, mutexes, locks and all that stuff. IMO Concurrency is fine for completely independent things, but if threads needs to share information heavily or frequently it’s better to do it the asynchron way. Tow, would my proposal be possible at all? I don’t want to change anything here, because I think now we have most parts running and this part would be a major refactoring (needs much time and it would be likely that new bugs arise).
I asked Tow several times to describe synchronisation scheme in VCMI (as well as Bonus System), but it’s still not there. Good thig you’re getting through it. Synchronising client and server with netpacks has been the source of all evil since I joined the project.
Hi
I don’t think the current threading design can be significantly simplified without losing some of its relevant qualities.
There are several requirements:
You can get user I/O events only from main thread
You must guarantee that main thread won’t be blocked by some expensive operation. If the events aren’t accepted for a while, Windows budges in with “not responding” dialog.
Server communication (send -> receive result -> process) is expensive.
AI needs callback calls to be synchronous. (after callback request call returns, results must be already processed)
GUI needs callback calls to be asynchronous. (otherwise interface is too laggy)
(likely some more that I don’t recall ATM)
I guess some threads could be in theory merged but in my opinion single-threaded asynchronicity would lead to much more convoluted code. From my experience it ends in hiding state in the call-stacks and continuation handlers, convoluting the main execution flow.
IMO it is much cleaner to have separate threads with clearly stated responsibilities and straight flow, rather than trying to interweave everything what engine does in a common thread.
Synchronization is ugly but concentrated to a few places (glue code that rarely is touched). Single-threaded asynchrony usually spreads ugliness across the whole codebase. Still, it might be a matter of personal taste.
My bad.
I’m too busy to work on VCMI at the moment but I should at least provide information to help you.
It’d be easier to me to get to writing that description, if you could provide some questions that I should answer or areas that are particularly unclear.
I never looked at it this way.
My top5 sources of evil:
Internal gamestate structures exposed to player interfaces. (abuse possibility, or they can be changed/deleted while interface maintains pointer to them)
Slow rendering, we need OpenGL.
Raw pointers and manual memory management.
C++ doesn’t support serialziation.
Packs from server to client that expresses gamestate aren’t atomic.
Single-threaded asynchronicity is not better per se, it has to be done wisely. Perhaps it is also a matter of personal taste. I don’t like the asynchronicity of e.g. Actionscript 3 / Client-Server RPCs calls, I always wanted to have synchronity and the possibility to use threads. You have to save temporary state somewhere, use recursive calls, … to do something easy, not complex task which is really evil.
Nevertheless, I think it would be possible to fulfill all requirements concerning client-server communication, AI callback handling, GUI handling, etc. of VCMI in a clear and robust way, if you have the possibility to design everything from the ground up as compared to AS3 / Flash. I think it would be a interesting task to design a concept for it and to test it in a smaller-scaled prototype to see if it works.
Another question, when will GUI objects be deleted from memory? I see GH.popInt or popInts() often, but when will they be deleted?
If you restart a map / campaign in Heroes 3 the random seed of the previous map won’t be used, e.g. you have different stacks in your starting hero. Whereas the random seed of the previous game in VCMI will be used. I think the same random seed shouldn’t be used. Currently RMG maps are generated every time you restart the map with the same seed. The map shouldn’t be generated when restarting, therefore no seed of the previous game has to be used.
Random seed needs to be remembered during restart so that all random objects on the map are initialized in same way. This also is true when restarting the random map with randomized objects on it - not only creatures or artifacts, but also diffeent objects such as Warrior’s Tomb.
Of course random map shoudl not be generated again upon restart, it should be stored somehow and reused with same seed.
beegee,
There are two “types” of GUI objects - windows (objects without parent) and widgets on window (window was set as parent)
For windows:
popInt() will only remove window from stack while popIntTotally() also deletes the object. Usually you can just use popIntTotally and don’t think about memory. Check code of these popInt* calls - all of them are quite small.
For widgets:
they can be automatically captured during construction using OBJ_CONSTRUCTION* macros. In this case parent will automatically handle all objects and will pass all signals to them (activate/show/delete - controlled by recActions/defActions members).
Frankly I was too lazy to figure out differences between those macros so I just use OBJ_CONSTRUCTION_CAPTURING_ALL everywhere.
Old code (read - most of pregame) does not uses this system so objects must be handled manually and then deleted in destructor. If you wish - you can rewrite code with object capturing in mind. This would be great but can be time-consuming.
Now that you’ve mentioned it - Tow, while I’m OK with current code in this area but can you explain what’s the purpose of CIntObject base classes? Right now they’re pretty much unused and all code uses CIntObject’s.
And why there are two methods that handle screen updates (CPlayerInterface::update() and CGPregame::update()). I know that one is active during pregame and another one during gameplay but why not move common code into one method in GuiHandler?
Warmonger, but does H3 preserves random seed on restarts? It looks that H3 preserves only starting options - starting town/faction remain same even if it was random but all randomized objects do not keep their state on restart.
Ivan, thanks for your info! CLoadingScreen and CPrologEpilogVideo will simply be deleted in CPlayerInterface::playerStartsTurn with a call to GH.popInts (GH.listInt.size());. GH.popInts not only removes windows from the stack, but also deletes them, whereas GH.popInt only removes the specified window from the stack. -> pretty clear, isn’t it? I thought that I would find delete calls after loading has been finished or video has been played fully in event handlers, but ok. You can always set a breakpoint in the d-tor to see if the object will be removed:)
It would be nice if CPlayerInterface::update() and CGPregame::update() could be moved to the GuiHandler. Event handling, updating animations, cursor handling, drawing, etc… should be part of CGuiHandler or CEventLoop class like in Qt. An example: In the campaign bonus screen (when ingame) it is possible to start another map, so current game has to be stopped, video be shown and the loading screen be shown. Player interface is down after the game ends. So if you want to play the video of the campaign you have to re-create an instance of CPreGame from which you only use the update functionality. (Event handling should be always available)
This is what I noticed recently. Randomized objects, stacks in heroes, … don’t keep their state.
C++ doesn’t support well asynchronicity.
Even with boost::future & friends you end up in a .then-continuation spaghetti hell. And it only works so far. It’s hard to force various handlers to return within given time limit or to suspend their work and finish it later.
There are actually a few quite interesting attempts to tackle this problem. Microsoft recently implemented resumable functions (mostly based on await/async feature from C#) and pushes towards their standarization. See the presentation or proposal paper. This is something I find pretty impressive, making the asynchronous code as clean as its synchronous coutnerpart.
Possibly similar things can be acheived right now with the Boost.Coroutine library. I haven’t checked it out yet but it seems interesting as well.
OBJ_CONSTRUCTION_CAPTURING_AL ensures that new objects will accept all kinds of redirected events from parents (defActions and recActions set to accept all).
OBJ_CONSTRUCTION relies on the defaults as defined in GUIHandler::defActionsDef — that depends on the current phase (in-game or pregame had different values AFAIR).
The distinction was to ease a transitional peried when some parts of interface are rewritten to the new GUI style and some are not. I doubt it makes much sense now. We should get rid of that distinction, as all the GUI should use the new system.
Actually pregame was the first part of GUI I had rewritten to use the new system. Almost all “delete” refer to def handlers or other non-GUI objects.
The worst place is likely the adventure map.
Mostly history. Once there was more significant distinction between interfaces (like adventure map interface, castle interface, battle interface) and GUI objects that were contained within them.
Now there is only CPlayerInterface left as the only updatable interface that is not a GUI object.
The classes could be merged without any harm. I never did it, because I didn’t consider beneficial. And CIntObject already felt heavy.
History and optimization and… details.
History — for a long time pregame and rest of the GUI used different systems and such distinction felt useful (the update() methods weren’t so duplicated).
Optimization — pregame uses different method of updating screen (only the top widget), while player interface performs simple or total redraw.
Details — there are number of differences in the methods. Player interface needs to lock GS and pim mutexes and possibly push queued info windows before messages are processed.
Merging these methods is reasonable, yet pretty non-trivial if we want to maintain their current workings. I always meant to do that as soon as OpenGL branch is merged. Well… it didn’t work that well.
I feel ashamed.
Bah, I believe GUI could benefit pretty much on replacing manual memory management with smart pointers. There are still some memory leaks I didn’t had strength to fix.
My bad. What I don’t like with pregame UI is huge number of hidden dependencies & size of most classes/methods. Splitting them into smaller parts will make code much more manageable.
Another reason is use of blitSurface/renderText methods instead of CPicture/CLabel classes.
Yeah. CIntObect already reached its size limit. I thought about refactoring in this area - merge base classes & split it anew into several classes but never got to a point where I would consider this necessary.
Can you remember any particular examples of memory leaks? I don’t remember anything like this in our UI.
Adventure map interface (adventureInt*) was not deleted when returning to the main menu. Once I even encountered a “new day” animation in the main menu. I am not sure though if it is still an issue.
I had to disable stack serialization by id when sending hero instance with all data from server to client. Pointer to stack instances were always 0x0. Why couldn’t the stack instance be de-serialized? Client didn’t end connection nor deallocated player interfaces. We can also move campaign scenario finish handling from server to client. (not required now) Client which have won knows end of campaign and has all required information. (client-server communication has no additional benefit) Additionally we can handle campaign / map end centrally in player interface gameover method.
Does the hero strength depends on attack / defense skill value only? So knowledge and power skill values aren’t counted. (it’s important for replacing hero placeholders in camapaigns, I’m just wondering, that magic heroes are so handicapped)
When exactly does this sending takes place in relation GameState creation/deletion?
Stack serialziation by ID relies on Connection having access to the GameState that stores hero with that stacks. The call to addStdVecItems sets this up.
If the sending happens when there is no GS or after a new GS has been set, then it won’t work.
I’d say it is more about components responsibilities. The moving forward logic belongs to server.
Code should not assume that client has all the data.
Use case (not very probable ATM but I’d like to keep it possible): cheat-proof MP campaign. Server stands on a remote machine. Clients play campaign without having beforehand access too all the maps or possibility to tamper with campaign advancing logic (eg. by altering hero state).
I agree that is not fair for magic heroes, yet the power formula was taken from H3. At least for neutral monster behaviour logic (fight/join/run). I’m not sure if campaign hero placement algorithm actually relied on the same notion of “power”. It might be best to check behaviour of the original game.
GameState is alive on both sides client and server. Server doesn’t shut down as long as there are open TCP sockets and therefore GameState won’t be deleted. Client closes socket when it received package UpdateCampaignState which holds hero instance data including stack data.
OK, in this case, yes. I already moved generating and sending player warning messages from server to client as it was easier to handle that stuff in client only. I think this should be OK as those messages aren’t gameplay relevant and only an information for the player. Additionally I can’t think of any use case where the client doesn’t know about its town count and days without town count.
I believe the case here is that you don’t want to use stack serialziation by id at all.
It means that no stack data is serialzied, serializer just passes the stack’s owner object (hero/town) id and slot position and performs the appropriate lookup on the other side. For this to work both client and server need to have the same copy of hero and its stacks.
If one side (eg server) changes locally hero or stack state, the changes won’t be serialized (it sends only id, not state).
Additionally, this feature requires smart vector member serialziation to be turned on. [sending hero by ids, not state.] I guess there should be a check for this and clear diagnostic message.
Exactly.
Client should be assumed to know everything that player interfaces it handles are allowed to know, that includes PlayerState. State presentation details (like displaying GUI window) belong to PlayerInterface.
Check and message would be nice, I think the problem is that smart vector member serialization is turned off. I didn’t write that part of code, so I better leave it at that.
I thought that I would find delete calls after loading has been finished or video has been played fully in event handlers, but ok. You can always set a breakpoint in the d-tor to see if the object will be removed:)