widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

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.