widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

I have very little time this weekend, but wanted to give you quick feedback right away.

[moving of dirs and splitting graphic into sub directories]
great change. 

> So, I decided to do some overhaul:

Would have been better to do that in trunk, so that the data move is independent. Do you think that is feasible still or whould that be a ton of work?

Also, I am a bit confused about the direction you took with your overhaul. Could you give some comments on the design choices you made?

I think addressing files by their name relative to the data directory (which used to be the root directory of widelands, but now will be data) is fine. It is similar to how we include headers, always from the root. 

1. Introducing sub directories for sound and so on seems cleaner, but might limit us in the future. For example what about sound effects specific to one building - should that not live in the same directory where the building is defined? Think about the ability to define a new building in a scenario for example, there everything will be in the same directory for sure. I argue this should be the same for bundled data. 

2. We are still not free to rename stuff because of Lua/Scenarios. And the fiddling with the filenames example could have been avoided without the image_catalogue too - for example in the one fix you posted, you could have introduce a string get_fsel_pic_for(int player) which would have killed the repetition.

I see some disadvantages for the image_catalogue:
- It is similar to the image_cache and it becomes confusing when to use which.
- The code has now one more indirection whenever you need a picture and is harder to read ("pics/blub.png" is easier than kBlubImageFileName).
- The loading adds more dependence on the g_gr singleton whenever you want an image.
- Also, what about the enums? Are they send over the network or saved? If so, we must never change their ordering and cannot ever delete an entry. That is also not very flexible. 

On the other side I see no real advantage to it, since we still cannot move images around. How do you think this improved the code?

3. we'll figure it out :)

4. It was to be expected that the data move would be a huge change, so it is perfectly fine to get that right first and then do a separate change for the shaders. 
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1397500 into lp:widelands.