Thanks for the review! It is very much appreciated. You also caught some bugs
that I have missed. Code reviews are such an awesome thing! nearly as good as
pair programming.
> - Why did you hard code the compiler in compile.sh
good catch. gun did that and probably submitted the change by accident. I
didn't see it. Reverted to trunk.
- Why are critter sound_effect split in directory and name and not just a path
to the sound file?
That is legacy. The sound_handler randomly picks one file of bird_*.ogg when
directory/bird is requested. I am not sure where I should document that better.
Changing the sound handler to accept files explicitly (as a list instead of a
glob) is probably also a good idea.
- widelands_streamread.h.THIS should be just .h but is not used anyways. Remove?
Removed.
- - '// TODO "stone" is defined as "granit" in the world': Can this be fixed
easily? Seems to fit in the context of this commit.
that code around that area seems wrong to me. This translates attribute = {
"stone" } as the name of the resource named granit. I think the correct
approach would be to make attributes translatable in the world or let the
building define the out of resource message in its configuration, but I did not
want to touch even more code for now. I expanded the TODO with this information.
- ResourceDescription::get_editor_pic looks very wired. I am not totally sure
what this method does but I am almost sure that there is an easier way with
less complexity to achieve the same result. However, you did not write this
code.
Well, I think I probably did write this code - way back. I agree to its
complexity, I do not fully understand how it does it either, but it essentially
just picks one of the defined pictures for a given resource amount. I did not
want to touch it for now as this change is already too big.
> TerrainTypeFromString.
It is correctly handled like this: it is a stand alone method in a .cc file in
an anonymous namespace. That means that it is only visible in this .cc file
where it is used.
- Uint32 in src/sound/fxset.h: What is the difference between Uint32 and
uint32_t? Should it be changed?
One is the SDL typedef, one the C standard typedef. I agree to change this for
consistency, but we should do it more globally. I opened bug 1330599 for that.
Please take another look.
--
https://code.launchpad.net/~widelands-dev/widelands/one_world/+merge/222708
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/one_world.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
