patches for various things

[ghost]

This is not a real issue, or at least, not against wyrmgus but against github which does not allow to send patches without cloning twice (one on a self repo, another one on the computer), pull, merge, push, and finally send a damned request.
For technical reasons, I can not use the pull request system of github (I can't even do a git clone of projects) so I'm using this to send patches I did.

patches_from_a5bc.zip

The 1st patch is a little, quick and dirty patch (~4 lines added) to allow compilation with oaml support disabled. There is obviously a better way to do so, but I hadn't enough time to investigate around lua.
It is a patch against commit a5bc.
It have no impact on other ones and so can be rejected if a better solution is done. I consider it a temporary fix.

The 2nd patch is not useful: it is the result of the merging in my improvised clone (based on tarball downloads) of the last merge (f97c760) in this repo, on which I've built the other patches. I've included it so that you can merge the other ones in an easier way.

The third patch reduces the number of files affected by the autogenerated version informations. My goal was to fasten recompilations, but in the process I've replaced occurrences of hard coded strings by variables related to version information, thus it should make it easier to work on #1 .
Another consequence is to close #116.

The last patch is a fix to clean some warnings from clang, because of code relying on checks about address of a reference, which is always true. There are other in the code, but I could not find a clean way to fix them.

Last thing: all those patches are against CC0 or CC-BY, so feel free to do whatever you want with them.
I'll now work on more important stuff, those ones were just to allow me to work faster with a computer that should be given to a museum :D (really, compiling with less than 200MB or RAM and an i586 monocore cpu at 700MHz is quite a pain, I promise you).

PS: I apologize for the burden.
PPS: I did the compilation and it went fine, but I'm almost sure the 3rd breaks stuff for windows, I have no way to hack around that OS and so do proper things.
PPPS: I could not run the result of my patches, because it seems my compiler generated illegal instructions (I'm almost sure that it compiles for a i686 target and I suppose my CPU is i586). They should not change behavior, but additional checks would not be a bad idea. I'll try to fix the issue on my side asap, but have no clue about when I'll be successful.

[Andrettin]

Thank you!

Patches 1 and 4 look pretty good. I have some questions regarding patch 3, however. genversion shouldn't take that long, I think, to compile. Maybe it's taking longer because of the unused git version code from upstream, which I disabled from having an effect, but didn't completely remove?

In any case, it might be more practical to simply no longer require version-generated.h in version.h, and move the "StratagusMajorVersion" and etc. definitions there. I'm not sure there is any benefit to using genversion, since it's pretty easy to change the define manually. This has the further advantage of not needing duplicate variables.

[Andrettin]

Hmm, I haven't applied git patches before. I've looked for a solution, and seen that "git apply" should do it, but I haven't been able to find where to put the patch files so far.

[ghost]

Maybe it's taking longer because of the unused git version code from upstream

It is more probable that it is because I'm working with a computer that I guess is around 17 years old, a beast "designed for windows Me" haha... unfortunately, I have no better hardware at home atm. Recent hardware problem on my real stuff. I intend to fix that asap, but... well, lot of stuff to think about those days.
Anyway, there were recompilations that could be avoided by reducing dependency on generated-version.h, which is what the 3rd patch does.

In any case, it might be more practical to simply no longer require version-generated.h in version.h

Well, this was the original goal of the 3rd patch: the only file depending on the generated header (crap, I forgot to merge my new version header to version.h...) is now a cpp file, which means no recompiling except for that file.
As I worked on it, I noticed that there were hardcoded instances of strings like homepage, stratagus, gpl v2, etc, so I replaced them with the variables in the process. One of the side effects is that now, it is easy to say that the homepage is the wyrmsun's one, and not stratagus's one. Another is that it is easier to change the pathes where the engine search and write files, making it easier to install both stratagus and wyrmgus (but I have not changed the values, as you can notice: I tried to not change behavior, since it ).
There is still a lot of duplication around, for sure, but this is another problem that I don't think should be addressed in current status of the code (there is a lot more things to fix before simple duplicated output strings).

[ghost]

Hmm, I haven't applied git patches before. I've looked for a solution, and seen that "git apply" should do it, but I haven't been able to find where to put the patch files so far.

I've found that the easier is to use the good old patch command (I've tried it before generating those, I had no idea about how to do it too). This worked for me: patch -p1 < 000x-foobar.patch

[Andrettin]

Where would I place the .patch file when doing that?

[Andrettin]

Another thing about version_generated.h, you should only need to run that one time; after that, you should only need to build the "Stratagus" subproject to the solution.

Well, this was the original goal of the 3rd patch: the only file depending on the generated header (crap, I forgot to merge my new version header to version.h...) is now a cpp file, which means no recompiling except for that file.
As I worked on it, I noticed that there were hardcoded instances of strings like homepage, stratagus, gpl v2, etc, so I replaced them with the variables in the process. One of the side effects is that now, it is easy to say that the homepage is the wyrmsun's one, and not stratagus's one. Another is that it is easier to change the pathes where the engine search and write files, making it easier to install both stratagus and wyrmgus (but I have not changed the values, as you can notice: I tried to not change behavior, since it ).

That's a good idea, as having these things be dynamic would make them easier to change later on. My worry though is that with a lot of references to "NEW_HOMEPAGE" and etc. it may increase the work to later change them to "HOMEPAGE" and such, and it seems to me that keeping using a define instead of const variables for these things would be more practical.

[ghost]

The NEW_* stuff is because I forgot to clean them. It can be fixed by sed -i -e 's/\<NEW_[A-Z]*\>//g' file_list. I apologize for this (this will remove the new_ part of variables, in theory there is no more non new_ variables).
I used the new part to quickly find occurrences of previous symbols, nothing less. Same for the filenames. Those were not meant to be send... Sorry for that noise.

The point to have const variables instead of macros is that they do not need to be instantiated in a header, thus it reduces recompilation time.
Also, it allows compiler to check types, but I admit that it is not a very good point when there is not tons of defines.

About the generated version... I just didn't thought about only make stratagus... damn. It defeats all the compilation argument, then.

For the patch file, you can put it where you want, just put the whole path after the < (this, in bash, means that stdin becomes the file after <)

[Andrettin]

Using "patch -p1 <" didn't work for me (is that a Linux-only command? I used Git Shell within Windows when doing it), but "git apply", followed by the full patch path worked, and with that I've applied patches 1 and 4. It didn't apply the patch as a commit though, it only directly applied the changes.

Regarding the changes in patch 3, I've manually applied most of the dynamization of the engine name/homepage/etc. changes, but I've kept the standard defines (at least for now), to avoid the duplication of variables. Also, apparently the version_generated.h is used by other things (considering the comments in CMakeFiles.txt), so I'm not sure about removing it yet. In any case, I have eliminated the (unused) code which based the version in the define on the git commit.

Thank you for the contributions! :) Should I credit you as "bmorel" or "Morel Bérenger"?

[ghost]

I guess the complete name would be better for the name. And thank you for the game :)

The patch command is an unix one, I think you can use it through git bash on windows, but not sure.
If you did it another way it's ok though.

Have you any remark about things I should enhance in next ones to follow your coding style? I tried to follow but sometimes it's not easy to guess and or follow existing stuff.
Except to be more carefull about temporary var names, of course :S

About the partial integration of patch 3, it's ok, I will simply keep a set of patches locally, as long as I keep that crappy computer, and cancel it before generating patches to send.

[Andrettin]

I guess the complete name would be better for the name.

Done :)

Have you any remark about things I should enhance in next ones to follow your coding style? I tried to follow but sometimes it's not easy to guess and or follow existing stuff.

The code you wrote seemed perfectly in line with the coding style to me.