I think we should have a seprate thread about upcoming or recent trunk changes that may need some discussion or clarification. Especially when they are not related to any particular hot topic, such as modding system.
So, my first complain is about recent commit from Ivan: r3257
While level 9 Dragons are resonable, they are not standard and certainly not disscussed before.
Also, what does 'special" for creature mean? I remember that special heroes do not show up in tavern, but neutral creatures should spawn as well as others. Certainly original RMG is able to spawn them, unlike campaign heroes for example.
My bad. For some reason I thought that Crystal\Rust are lvl 9
Interesting fact is that internally in H3 all dragons have level 7 - not even close to 10
Now that’s interesting. Reason for this change was due to fact that these 6 creatures (dragons + enchanters and snipers) can’t be recruited in Refugee Camp and H3 also does not spawns them as random creatures (despite that it will spawn other neutrals). RMG does not spawns their dwellings either - only creatures themselves.
which creatures should be strictly special (ammoCart, fisrtAidTent, catapult and “gods” certainly should but I`m not sure about about commanders and even ballista)
They are legal in terms that they can be handled by engine. But for example I just found one bug related to such levels: Clone spell won’t work on dragons. This is not how it works in H3.
Those that are normally not available in-game - these objects can be placed manually on map or used in config but they can’t be subject to any random picking and by default - disabled on map. So both commanders and ballistas are “special”
Warmonger, I’m wondering - is this a bug in H3 or a feature?
But in any event I think we shouldn’t add different behavior between RMG and game randomization. So we should either enable these creatures everywhere or disable them instead of something inbetween as in H3.
I would like to rename my added sub-folders in /lib and /client to lowercase e.g. RMG to rmg and Logging to logging. It seems to be that lowercase named folders are more often used and consistency in naming folders and files is nice. I want to do this in a separate SVN commit. All references in project files, cmake, etc… will be updated. If it’s not ok, please tell it:)
I think RMG should stay as it is, as it’s acronym. No need to impose strange naming conversions on files which nobody ever needs to browse.
What worries me more is a rapidly groing number of files that are almost empty. For example CLogFileTarget.cpp has less than 30 lines and new multiple headers are nothing but trivial comments. Constructor, destructor… what’s the use? Everyone can tell the arguments and returned type from function signature and the comments do not add any valuable info to it.
It doesn’t matter if the name is acronym in my opinion. Simply write everything in lowercase and camel-case to separate words. For example CVCMIServer could be written as CVcmiServer or VCMIDirs as VcmiDirs which is more readable. But all in all it has a lot to do with personal preference. I think that the first letter of folder names should simply be lowercase.
Low count of source code lines in h/cpp files isn’t a bad sign as well as high count of source code lines isn’t automatically a good sign. The logging API doesn’t consist of 1 or 2 classes, it is not that trivial. I think different parts of the API are clearly separated and loosely coupled. Would it be that much better to create a file LogTargets / LogTargetClasses / ILogTarget and put all target classes in one compilation unit or to put all logging classes in one file named CLogger.h/cpp? I don’t think so.
The comments are just for the sake of completeness. Simply skip over it, it they don’t provide any valueable information:)
I don’t think that multi-line comments are needed for trivial functions. For example this:
/**
* Sets the color mapping.
*
* @param colorMapping The color mapping.
*/
void setColorMapping(const CColorMapping & colorMapping);
Your description duplicates function name. And description of parameter duplicates parameter definition.
Thus I can get same amount of information by reading one line: function definition instead of 6 lines. And considering that vertical screen on monitor is limited in this case I need to spend more time on reading than I will do when this comment is not present.
In my company I have to follow coding guidelines which says that everything needs to be documented. I think for VCMI I’ll do it not the same way:) So my new rule is, I’ll comment everything except of:
@Warmonger:
To come back to the increasing files count, I have a proposal:
The normal user of the logging API(in 95+% cases) has to work with 2 classes:
CGLogger
CBasicLogConfigurator
The remaining classes are mostly implementation classes or let’s say detail classes where no one is interested in. For an independent logging API which needs to be modular and extensible you can’t do that, but here it’s ok. So my proposal is to move all the remaining classes CLogManager, LogRecord, CLogFormatter and all target classes into CLogger.h/cpp. Would that be fine?
In one of my next commits I’ll remove the rpm folder and the spec file from SVN. Updating the file in /trunk and /tags/xyz is annoying. Anyway, it is not common to store RPM data in SVN. (in SDL, Boost, etc… there is nothing related to a specific package format e.g. rpm). I believe for debian/ubuntu it is different(don’t know). If anyone is interested in the file, please write a PM to me. In far future I’ll try to add the vcmi package to the offical fedora repos. Perhaps then the spec file is somewhere stored on the repo servers.
I’ve refactored the CMapEditManager, so that it is capable of a undo/redo functionality(not implemented). It can be used by the map editor for example.
BTW, is there any class in lib which represents a rectangle as a addition to int3? In client we have a rect class, but it is coupled with SDL and is missing a z component. That’s why I’ve added a MapRect struct in CMapEditManager.