widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

> 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?

Why would you want to do this in trunk? The code and data locations depend on each other. In this branch, the dependency is reduced, but it is still there. I have moved everything with bzr move, so it shouldn't cause problems with merging.


> 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?

The basic design choice here it to make the code more resistant to changes like this one, so if we should ever need to move something around, we won't end up with huge changes like this one. For example, I made a last-minute decision to rename "pics" to "images", see how small the diff is:

http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1397500/revision/7331


> 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.

If this case should ever come up, we could do what I did for the images in src/graphic/richtext.cc, like this:

if (!g_fs->file_exists(filename) && !boost::starts_with(filename, "map:")) {
  // prefix the data dir to the filename
} else {
    // don't prefix the data dir to the filename
}

The only case we have had so far is the duck, so I decided it was cheaper to just move the files over.


> 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.

The problem wasn't really the repetition, but rather the direct manipulation of filenames, depending on file name length.


> 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).

Actually, the change is g_gr->images().get("pics/blub.png") -> ImageCatalog::Key::kCategoryBlub in most cases.

The only places where we have a frequent explicit call to g_gr is where we also allow getting the image from lua/conf.


> - The loading adds more dependence on the g_gr singleton whenever you want an
> image.

Does it? It was g_gr->images().get before.


> - 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.

The enum is for UI images only, which are defined in C++ and won't be saved at all.

Maps, tribes etc. still get their images as they used to, because they don't have access to the enum anyway.


> 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?

We can shift filenames around pretty easily now within the data/images directory. On the C++ side, it's just 1 entry we change, rather than a bazillion places in the code. I definitely missed some when I moved things around and ended up with bugs that I needed to notice and fix. On the Lua side, changes are minimal (basically, just the tutorials and developers file).

If we decide not to go for the ImageCatalog, changing it back will probably be a lot easier than implementing it.


> 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.