Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Yes, that looks much better :) @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/284574 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Maybe a silly question: With this change we have some piece of C++ code living in directory data/. As i understand this folder would be regularly installed in a whole (from an installer or when installing over a ppa). So they get installed without usage? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/284574 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1397500 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Sorry, answered myself... the files are needed after compiling and when starting widelands. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/284574 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1397500 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Review: Approve test local build & appveyor Does work now. @bunnybot merge -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Review: Approve I just tested this on Mac OS X and made a slight adaptation to the build script we use. This is ready to go IMHO. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
how about: ui_sfmenu -> widelands/ui/menus wui -> widelands/ui/game/ editor/*all_ui_stuff/ -> widelands/ui/editor/ This would move us (further) towards separating non-widelands dependant stuff and widelands dependant stuff? I wonder if that should done in a separate PR though, because this is already unwieldy and hard to understand what was actually going on. I would suggest getting this one in ASAP and then do the small rename adjustments in smaller PRs after that. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
+1 for finetuning the file structure later. I have added SirVer's idea to the bug. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
No, an absolute path begins with a slash (/) (afaik) and "data/" does not. But i am not really sure though... But you could investigate the path for your self. Here on arch-linux the datadir path with the current setting is: "/some/directory/widelands/./data/" And that is wrong, instead it is right if you rename it in CMakeLists.txt to "data/". -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Now I get what you mean - fixed :) @Tino: does this help with Windows? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Thanks for testing! Yes, the naming is based on the source code, fs = fullscreen in this case. If we rename anything here, it should be done both in the source code and the images - naming should be identical. how about: ui_sfmenu -> ui_menu wui -> ui_game? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Sorry, the first statement was too short :-S What should we do with the folder data/unused/ and his content? I would suggest to move this into widelands-media repo. Thanks for collecting these unused pics :-) The structure (naming) of the folders is a bit uncommon. F.e. "data/images/ui_fsmenu"... ui = User interface, but fs? "File selection"? No... and "data/images/wui"? Widelands user interface? I believe the naming convention is based on the wording used in code and this is ok. But for interested users who doesn't know the code it maybe should become more self-explaining. Just my opinion... Currently i found no malfunction :-) I want to test also the option "--datadir=" but this could take some time. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
I had a look at http://bazaar.launchpad.net/~widelands-dev/widelands/bug-1397500/view/head:/utils/win32/mingw/config.h and I don't know if the . in the datadir needs removing or something. SInce I can't test on Windows, it's hard for me to play with stuff. The variable that needs setting is datadir_ in wlapplication.h/cc. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Review: Needs Fixing
Sorry, it does not work: If started without --datadir Widelands assumes
{execdir}\data correctly, but does not build the path correctly:
$ ./widelands
Set home directory: C:\Users\mit\.widelands
Widelands executable directory: C:\bin\Widelands
Adding directory: C:\bin\Widelands./data/
Caught exception (of type '17FileNotFoundError') in outermost handler!
The exception said: FileSystem::create: could not find file or directory:
C:\bin\Widelands./data/
This should not happen. Please file a bug report on version
bzr7353[bug-1397500](Release).
and remember to specify your operating system.
With --datadir it is fine:
$ ./widelands --datadir=./data
Set home directory: C:\Users\mit\.widelands
Adding directory: C:\bin\Widelands\./data
selected language: (system language)
Graphics: Try to set Videomode 800x600
Graphics: OpenGL: Version "4.3.0 - Build 10.18.14.4139"
Graphics: SDL_GL_RED_SIZE is 8
Graphics: SDL_GL_GREEN_SIZE is 8
Graphics: SDL_GL_BLUE_SIZE is 8
[...]
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860
Your team Widelands Developers is subscribed to branch
lp:~widelands-dev/widelands/bug-1397500.
___
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
I made the regression tests work in this branch. Could you take it from here? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Before I consitune working on this, we need to fix the issues with the regression suite. I have been unable to crack this, and it's the reason this branc is still sitting here. Would you like to take this on for me? Here's what I wrote about the problem: I have changed the global datadir to "./data", and it is working fine, except for the tests which are broken, because the map loader won't find the maps now. I tried moving the tests to data as well, but this will in turn break regression_test.py and I didn't manage to fix this. So, can somebody please look into fixing the tests or give me some pointers? Again, I have added NOCOM comments to point out the important code changes. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Review: Needs Fixing Let's get this branch in before it starts to smell from sitting around too long :) I think it is a good improvement, making data/ the only directory that we need to install. However there are a couple of things that need fixing: - this fails to compile on gcc 4.7 (see travis link) - this needs fixes to the texture atlas generation in graphics. The atlas must contain at least roads & textures and ideally as many of the often used UI graphics as possible. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Yes, I would like to have this. I don't know how to fix the Python stuff for the test suite, which is why this branch is still waiting. I think other things are highter priority though - I'd be happy to target this for Build 20. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Widelands doesn't have a locale sk_SK@UTF-8 like Linux does, for example, but we use simply sk. So, we move from back to front until we find a matching locale. So, this is just some console output that you can ignore - It is sometimes important for me when I am working on i18n stuff, so I'd rather we keep this message in. This has nothing to do with this merge request though ;) The problem I have is that the path that the Python script wants for the test suite isn't the same path that we use in-game in this branch, and I didn't manage to update the Python script accordingly. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
OK, I understand this is out of scope, but generaly locale recognition should be more verbose - to allow common user to understand why the game does not run localized. Though advanced users would not care if localized or no... I dont understant exactly what you mean but the line in regression_test.py: 180: for wlmap in glob(os.path.join(test, maps, *)): says that it check test/maps/* for test suite maps (relative path), so you can change it, or (more complicated design) introduce new argument to regression_test.py -p/--path or something like this. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
@GunChleoc, this is maybe out of scope of this change, but what defines the language of widelands (when running the game). It seems it is insensitive to environmental variables, LC_ALL=slovak, LC_ALL=Sk_sk are ignored. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
could introducing something like m_testsdir be solution? And pushing it to m_filesystems. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
when I manually moved test to data, regression test went fine... hm? -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
one comment added Diff comments: === modified file 'src/wui/buildingwindow.cc' --- src/wui/building_statistics_menu.cc 2014-11-30 18:49:38 + +++ src/wui/building_statistics_menu.cc 2015-06-06 14:58:59 + @@ -149,8 +149,8 @@ new UI::Button I think that m_datadir + /locale is (was) fine, just we must make sure that locale dir is indeed copied to datadir. Now I got /var/widelands/bug-1397500/build/compile/locale - that is nonsensical place, because build directory is only temporary one, is not it? (this, previous_constructed, JUMP_PREV_BUTTON_X, IN_BUILD_Y, 24, 24, - g_gr-images().get(pics/but4.png), - g_gr-images().get(pics/scrollbar_left.png), + g_gr-images().get(images/ui_basic/but4.png), + g_gr-images().get(images/ui_basic/scrollbar_left.png), _(Show previous), false); m_btn[PrevConstruction]-sigclicked.connect -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
further comments, I compiled with -DCMAKE_INSTALL_PREFIX=/var/widelands/installed-bug-1397500/ \ -DWL_INSTALL_DATADIR=share/widelands It works with but see on part of console output: Adding directory: /var/widelands/installed-bug-1397500/share/widelands = my datadir - OK Realpath: /var/widelands/bug-1397500/build/locale = nonexistant dir selected language: (system language) = but I have LC_ALL=slovak, probably it wants to set slovak but directory with locales is not available, perhaps some error/warning would be fine here using locale en_US The game runs regardless, so player has no idea something is broken Also regresion test does not run as discussed above, of course if you move 'test' to (as) $DATADIR/test/, they works. Also the exemption that you get when test directory is missing can be improved to: throw wexception(LayeredFileSystem: unable to create sub filesystem for %s,dirname.c_str()); to give a player a hint what might be wrong. (layered_filesystem.cc) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
oh, I found that Widelands checks LANG variable and LC_ALL Never mind, now I can make widelands run in my langugage, but what I dont understand is this in output: No corresponding locale found - trying to set it via LANGUAGE=sk, LANG=sk WTF, why it complains if localization works? :) -- https://code.launchpad.net/~widelands-dev/widelands/bug-1397500/+merge/243860 Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1397500. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Sub filesystems aren't a solution - sometimes, world or tribe images will be used as GUI strings, e,g, in the Terrain Tool. I will go back to using full paths, based on the data directory. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
Sgrìobh SirVer na leanas 24/02/2015 aig 06:18: For GUI images, we would then still need to prefix data to the image files, which sort of means that we are redefining the same information all the time all over the code base. Why? They load their graphics through load_image, which uses g_fs too. If we only register data/ into g_fs, it will just work and no mention of data/ is ever needed. Or am I missing something? I'm not talking about data, I am talking about images in data - the default path for images isn't data, it's data/images. I would like to have this encoded once in the source code instead of over and over again with each call to g_gr. So, I would like the calls to look like load(buttons/button01.png) rather than load(images/buttons/button01.png) -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
So, I would like the calls to look like load(buttons/button01.png) rather than load(images/buttons/button01.png) If you desire this to be more concise, I think a better approach is to create a sub-filesystem that tracks the images directory. So instead of pulling out the constants into a file, you basically create multiple filesystems: g_fs = createFilesystem(data/); Tribes::SetGlobalFilesystem(g_fs-create_sub_filesystem(tribes)); World::SetGlobalFilesytem(g_fs-create_sub_filesystem(world)); Ui::SetGlobalFilesystem(g_fs-create_sub_filesystem(images)); Though most of these functions should not be static, instead the constructor of World and Tribes should take the sub filesystem to search for their data so that ownership is clearly defined. This does not solve the directory layout inside of the different sub directories of course. I also believe that this might take separation too far. Needing to list the full path allows more flexibility - i.e. you can use headquarters/menu.png also as the background of a window and you can use tribe graphics from the ui by just specifying the full path. There _is_ a directory layout, I think we have to acknowledge it is there and accept that having a full path to the files brings repetition, but has advantages and disadvantages. -- 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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1397500 into lp:widelands
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. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
