I think the way how the RMG map is generated, that means the organization that every client generates a map, is problematic. In a multiplayer game there can be clients which use different implementations of the boost random API. This can result in different maps, which will lead to undefined behaviour. This solution was quick and easy, but it seems to be that it isn’t the most stable one. Boost random API changed a lot in v1.4.7 as you can see here: boost.org/users/history/version_1_47_0.html (most changes belong to random api) There are two solutions IMO. 1. Commit all required boost random header files to SVN. That way it’s sure that every build uses the same API. We have no additional dependency. But it’s not the nicest solution, I know:) 2. Generate map on the server/host and send generated map object to all clients. That will lead to much I/O traffic. Some code changes need to be done.
Shouldn’t this API produce same results everywhere? (assuming the seed is same). They may have renamed some classes but I don’t see any references to changes in algorithms that may affect us.
If not - then RMG won’t be the only problem - right now most of map initialization is done separately by both client and server using boost random.
From what I can see boost::random headers depends on common boost headers. May be messy.
AFAIK that c++11 standard “feature” - return type autodetection works only if lambda body consist from single return statement.
And this is how VS works. gcc\clang behavior is less strict here.
I know people are sensitive when it comes to their coding style but looking through RMG I had some troubles reading some parts, so I can but wonder if you are not overdoing it?
And a side-question… I find the code much, much harder to read with a map instead of a simple std::vector. What are the advantages of using a map in this particular case?
/** Typedef of the players map, so that boost foreach can be used. */
typedef std::map<TPlayerColor, CPlayerSettings> tPlayersMap;
Another not related side note:
Maybe I am getting “older” but I prefer any time a good old readable for {if} -> return to lambdas like this:
They prevent pollution of global namespace. I’d prefer using strong enums but not all compilers support it.
What’s so hard about std::map? It’s a logical choice because not always all players are present.
for + if is an old, imperative style. Imperative style is all about optimization – we all should write programs declaratively, and, if that cannot be done efficiently, then in a functional way, and imperatively if we really have to. Unfortunately lack of so-called polymorphic lambdas and container.begin(), container.end() syntax makes this code look less appealing than it would look in a real functional language (Haskellish pseudocode:
Boost.Range provides range overloads for STL algorithms, we have it in PCH.
BOOST_FOREACH works with auto&, map typedef is not really needed.
why not use tPlayersMap::value_type as lamda argument type (instead of pair<>)?
instead of writing
sth = unique_ptr<Type>(new Type(...))
you can have
sth = make_unique<Type>(...)
Type is not repeated then. [you may need to add 3-arg overload, I didn’t need it so far :P]
checking if map contains element (eg in CMapGenerator::getNextPlayerColor): instead of players.find(i) == players.end() you can have just !players.count(i). Or use vstd::contains that works on “all” containers.
It’s VC10 issue, Visual 2012 works with deduced returned type.
Can be written pretty much the same in C++ modulo polymorphic lambdas.
Though… for me the beatufiful thing in Haskell syntax is it lack of parenthesis in function call syntax. After reading Haskell, C-based languages start looking… Lispy.
But it isn’t really shorter and makes an additional assumption that EWaterContent members are contiguous. (though it’s possible that I’d written this such way if it were simple to implement)
They are. Otherwise how would tell whether NORMAL is EWaterContent::NORMAL EMonsterStrength::NORMAL?
However you are likely the last one really needing that namespaces.
After we drop support for Visual 2010 we get strongly typed enums that create a name scope of their own. That namespaces will be gone then.
I assure that equivalent code based on vector would be worse. I know. Once we stored player settings in vector and it was source of endless problems and bugs. You either ened to have “empty, invalid” entries (and ifs for them everywhere) or separate “serial” and “color” IDs, either way it is horrible.
I believe it’s matter of getting accustomed to such style and its syntax. At the beginning }); felt so wrong to me. Support for functional-style programming elements in C++ is a new thing. It can be hard to read at the beginning. It gets better.
The difference is that algorithm+lambda allows you to clearly express intent and makes impossible to make a stupid mistake in counting algorithm.
When I look at the code I immediately recognize count_if part and I know what it is doing. In the traditional for-loop+if-approach it would take me longer — I would have to read through all the loop and make sure that it isn’t doing anything else and does not have side-effects.
Ty for your answers and yes you understood me right: it is not intended as critique is intended as inquiry.
Like I’ve said I learned and used C++/VB some years ago coming from java. Maybe my hard time comes from not being use to BOOST or from the new syntax.
Either way, maybe it is because I am expecting that as languages evolve, code will become more and more human readable and not more cryptic.
I was interested in the RMG part and here some example code I found. I cannot help but find it well structured, well commented and very easy to understand. LINK (where to put it is written in a language that I do not know).
One last question
Why would there be a problem if setWaterContent() would have an integer as an argument? You pass a simple integer and handle the rest inside the setWaterContent().
Because we talk only about 2 small enums, to me it seems that a small workaround would produce more easy to read code: NORMAL_WATER, NORMAL_MONSTERS could be used. It will take a little more care when you write it, but the result is a more readable code where the focus will be on implementation and not on discerning between namespaces.
They are. I find C++11 lambdas much more readable than workarounds that were used instead of them. Same goes to most of the other additions.
Well, there are some expert-level features like r-value references but they can be safely ignored. And their presence encourages more readable code.
ActionScript. Funny, right now I’m developing a project in this language. Not as fun as C++, not as evil as Java.
In the code you linked there is a number of lambdas (anonymous local functions with access to outer scope). Their syntax is quite similar (“function(args):ret{}” instead of ](args)->ret{}). The code is written in quite functional way: we have functions generating functions, building a vector of stages to perform, etc.
I find it similar to what we’ve embraced with C++11.
It would work. And I think really lots of functions in VCMI are written that way (taking int when they could’ve enum). Well, in programming there are usually many valid answers to the problem.
However — taking arguments as an enums have some advantages over taking by int.
There is concept called “type-rich interfaces”. Basically it is about using meaningful types in interfaces. Thanks to that:
we know what arguments are just by knowing function signature
If I have to use setWaterContent1 I have no idea what the “int value” is? Percent of water area? Fixed constants? Count of tiles? Something else? I need to check the comment next to function definition. I may need to look into another file to get list of defined constants. It is an additional step.
With setWaterContent2 I know that it takes a EWaterContent enum. So I can start writing a function call like "gen.setWaterContent2(EWaterContent:: ". And at that points magic happens and Visual gives me a list of possible values to be used.
Another thing, if I do something stupid like gen.setWaterContent1(750), it will happily compile and things will blow in runtime. Function taking enum would give a proper compile error. If the function accepts only several values, it shouldn’t allow being called with anything else.
These issues are not that significant (I would never call such function with 750! :P) but for such reasons I prefer to work with codebase where argument is taken by anum than by int. I find such functions more readable and less error-prone.
Don’t forget about NORMAL week type, NORMAL button state and NORMAL battle result. NORMAL is a very popular name for constant.
I don’t think that NORMAL_WATER or NORMAL_MONSTERS are easier to read than Water::NORMAL or Monsters::NORMAL. It’s just :: vs _.
I don’t quite understand what you meant by “code where the focus will be on implementation and not on discerning between namespaces”. In both cases you need to discern between two “NORMAL” constants. I don’t see why you see issue with “discerning between namespaces” and don’t see it with “discerning between constant suffixes”. For me it looks totally symmetrical. You need to provide information which NORMAL you want either way.
Why I consider namespace is better?
because function signature tells me from which namespace argument type comes. I don’t think about it, I just rewrite it. [1] With _WATER sufix again it would need an extra checking step.
It is neat. Values that logically are related (eg. various water levels) are also bound together in the codebase. I’d say that having adding various suffixes is focusing on implementation detail, while packing related constants into namespaces as a way of organizing code and clearly expressing the idea behind it.
Plus… Let’s have a call “setWaterContent(EWaterContent::NORMAL)”. If at any point I get bored with “NORMAL” I just place cursor before it, press ctrl+space and I get list of alternatives. Though… with WATER_ prefix it’s basically the same.
[1] Actually IDE does it for me. Just like displaying function signature and comments next to it on hovering. Not the point.
Ty again for your explanations. While I understand them and you make some very good points, for some cases I still remain at my conviction that while things sound good in theory in practice are just superfluous.
For example:
“Taking arguments as an enums have some advantages over taking by int”
Because you randomly generate an int hard-coded to be inside EWaterContent range I would say that you already know what you are doing anyways, so the whole casting thing is like friction on a wooden leg like a pirate would say.
While I know my skill limits, I will share this, maybe it convinces someone.
I was convinced some time ago that it is worth it simplifying things as much as possible over safety, so that when someone else (or me later) reads the code knows from a glance what it does. That makes it worth the risk of doing a “beginner” mistake because of that simplification. (Risk that in practice is ~0 anyways).
The specific problem I had was a code cluttered with Setter and Getter functions for everything. While they make sure of encapsulation, in many cases you just end up cluttering your code with them. The argument I received against this silly practice was that “No one in his right mind will unintentionally access a global variable by accident from a completely different part of a project”. So in practice dealing directly with a public member made life much more easier with no **real **risk increase by doing that.
Actually we don’t like (trivial) getters and setters either. There is no real difference between accessing a field and using its setter/getter, so why bother with uglier syntax?
Static type safety, on the other hand, is a completely different matter. It eliminates real risks and errors. To put it clearer:
...
int field;
void setField(int _field) {field = _field};
int getField() {return field};
...
then a.field = x is semantically and type-theoretically isomorphic to a.setField(a) and int g = a.field is to int g = a.getField().
Discussed example from VCMI breaks type-theoretic isomorphism.
HTH
Set/get functions make sens only in case when they check additional conditions or control access to some resource. There is no point to add functions that do nothing just for clarity of code… or lack of it.
You’re right, but it works the other way round. Forcing enums makes code clear, as beginner will understand these enums imediately and the only value used will be always named constant - no risk to make a mistake here, and every doubious case can be spot easily.
We could use quite a lot of improvements from HD Mod, but someone woudl have to contact its creators and ask for permission. It’s not nice to rip someone elses work
I do not really see the issues here. They themselves created a mod without any agreement (I guess). And they also had a ton of ideas coming from different places.
And since we talk about ideas only, those can be used with no problems. The thing remains that that screen is an improvement to the better for H3 so it should be present in VCMI.
But yes you are right, an e-mail (and mention in credits) would be nice.
ps. I am surprised that there is no real discussion about the RMG in general. Creating it is no trivial thing and he took a heavy burden by attempting it. But… the original H3 concept can/should be improved before it is too far ahead in development.
I am thinking about better and more detailed templates, better rules for maps (skill,artifacts, heroes, spells, buildings, towns restricted).
vcmi/lib/Mapping/CMapEditManager.cpp:261: warning: C++11 requires lambda with omitted result type to consist of a single return statement -Wlambda-extensions]
That is exactly how I read the static_cast: “now there will be some expression that I, programmer, personally guarantee to sensibly fit into that type” == “I know what I am doing here”.
And, as I always say, consider the alternative. You suggested taking argument by int. But that doesn’t mean that static_cast will disappear. It will just move to the inside of the function. Either way you have somewhere that cast. [Unless you store the class field as int but then you throw away useful type information completely.]
The difference is that it is better to guarantee that expression will fit the range near the expression itself. In the function I have much less certainty about the sensibility of the arguments. After all, cast is “I know what I am doing here”, not “I know what I am doing there”.
You are right.
When I saw that code doesn’t work with VC10 and works with VC11 I thought it must have been the bugfix. It is not, at least when the standard letter is considered. Apparently MS implemented proposed (not adopted) resolution to DR 975: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#975
I assume that GCC and Clang did the same (+warning), so it is quite portable, though non-standard.
I thought it was a known issues, or easy to find so I have not posted this Link. The bug was fixed in VC11. So that is why it is working VC11 and not in VC10, or?
So many posts were written after my last post, wow! Luckily I haven’t to answer that much.
You’re right! I’ve understood it wrong. I’ve thought it would work if there is only one return statement among other statements, not that the function body should consist only of one return statement. Thanks for clarity here.
Regarding RMG and random number generation, I’ll leave it as it is if we haven’t any problems. As pointed out by Ivan, map/gamestate initilization works almost the same way.(e.g. selecting random town,…)
@Tow:
Thanks. Your remarks are minor but valuable! I’ll update my code.
Me too:)
There will be a more detailed discussion at the right time for sure. The way how a RMG map will be generated is mostly the same as in the original H3 as you said it. Warmonger posted a presentation of the original author in the first page of this thread. The development will be based on that concepts, but aren’t fixed.
The first and most important step is to get a basic RMG done with all phases from terrain placement to the last phase obstacle placement / road, rivers placement. At least some sort of terrain placement should be done till end of april. Additions, improvements and fancy stuff can be done later and have a low priority. One thing I’ve learned from software development is not to plan everything far ahead in every detail. The rough picture of the concepts should be planned.
The next steps are to fix those two issues which I’ve named earlier. Most important is the correct handling of the terrain views. Then I’ll think about templates & zones and share you my ideas about it:) @krs Then you can add your ideas as well.
