|
|
|
This thread is locked; no one can reply to it.
|
| PHYSFS Add-On al_change_directory() Patch |
|
Todd Cope
Member #998
November 2000
 |
Attached is a patch which implements al_change_directory() in the PHYSFS add-on. I have been working on porting one of my games to Android and have come to the decision that I will be using PHYSFS to access my game's data files. In my quest to reduce platform-specific code as much as possible, I have implemented in my framework a resource locating function which relies on al_change_directory() to make loading data as platform independent as possible. Basically, I check a few places to see where the data is actually located and change to that directory. On Android I had a few options for loading my data. I have been using the APK file interface for a while, but that is turning out to be less than ideal for my needs. I turned to PHYSFS because of this post. I think it will solve the remaining issues I am having with my project. Now the problem, and the reason I made this patch, is that the data for Android projects is stored in a particular subdirectory inside of a ZIP file. Without a functioning al_change_directory() I would need to come up with a solution to modify the filenames of all files I access in my project depending on which platform I am working with. The patch implements al_change_directory() by storing a current working directory and appending relative paths to it when accessing the file system or files. The current working directory is the root directory by default. Calls to al_change_directory work like you would expect. Relative and absolute paths are supported. |
|
Thomas Fjellstrom
Member #476
June 2000
 |
You could use the ALLEGRO_PATH API instead of changing the working directory. Its fully platform agnostic. It was primarily meant for this type of use case. This of course means you're building absolute paths to files. If you're not using absolute paths it might be something to consider in the future. -- |
|
Todd Cope
Member #998
November 2000
|
I know the path API is there for this type of thing, but it adds a lot of complexity to my loading phase. Instead of just loading a file with a C string, I now have to allocate multiple paths for each file I want to load. I will illustrate the difference: 1/* load without path API */
2bitmap = al_load_bitmap("data/mybitmap.png");
3if(!bitmap)
4{
5 // error handling code
6}
7
8/* load with path API */
9final_path = al_create_path_for_directory(SYSTEM_DEPENDANT_BASE_PATH);
10if(!final_path)
11{
12 // error handling code
13}
14sub_path = al_create_path("data/mybitmap.png");
15if(!sub_path)
16{
17 al_destroy_path(final_path);
18 // error handling code
19}
20al_join_paths(final_path, sub_path);
21bitmap = al_load_bitmap(al_path_cstr(final_path, '/'));
22al_destroy_path(sub_path);
23al_destroy_path(final_path);
24if(!bitmap)
25{
26 // error handling code
27}
That is a lot of extra code for each load. I feel that implementing al_change_directory() in the PHYSFS add-on brings Allegro closer to complete platform independence. Otherwise, that is just one more gotcha that people will run into when trying to port their games. At the very least, implementing a previously unsupported function seems like a win to me. I see no reason why this shouldn't be supported wherever possible. |
|
Thomas Fjellstrom
Member #476
June 2000
|
Yeah. I do think implementing that function for physfs is probably a good idea. Todd Cope said: That is a lot of extra code for each load. Sure, but doing that would be silly when you could stick it in a function and call it from multiple places. Quote: final_path = al_create_path_for_directory(SYSTEM_DEPENDANT_BASE_PATH); More like: final_path = al_get_standard_path(ALLEGRO_RESOURCES_PATH); -- |
|
Todd Cope
Member #998
November 2000
|
Thomas Fjellstrom said: Sure, but doing that would be silly when you could stick it in a function and call it from multiple places. True, I was just illustrating how much extra code it is. Here is what it looks like if you do that: 1ALLEGRO_PATH * my_get_absolute_path_to_file(const char * path)
2{
3 final_path = al_create_path_for_directory(SYSTEM_DEPENDANT_BASE_PATH);
4 if(!final_path)
5 {
6 return NULL;
7 }
8 sub_path = al_create_path("data/mybitmap.png");
9 if(!sub_path)
10 {
11 al_destroy_path(final_path);
12 return NULL;
13 }
14 al_join_paths(final_path, sub_path);
15 al_destroy_path(sub_path);
16 return final_path;
17}
18
19/* load using path API with helper function */
20file_path = my_get_absolute_path_to_file("data/mybitmap.png");
21bitmap = al_load_bitmap(al_path_cstr(file_path, '/'));
22al_destroy_path(file_path);
23if(!bitmap)
24{
25 // error handling code
26}
That's still a lot of extra work for someone who runs up against this issue with a program that loads data from lots of files. You could simplify it further by adding helper functions for each kind of file you want to load. al_change_directory() was the first solution I tried because it greatly simplified things. Quote: More like: final_path = al_get_standard_path(ALLEGRO_RESOURCES_PATH); Not so. If I want my program to be installable from a repo for, say, Ubuntu, I need to store the data in a place that is specifically mentioned in the manual as being my responsibility to handle manually. In other words, I have to do it manually if I want to support Linux properly. |
|
Peter Wang
Member #23
April 2000
|
That global is kind of ugly, and a mandatory memory leak. One solution is to use a static buffer (thus limiting the directory name; maybe physfs has an internal limit anyway). Building the paths with the ustr routines or standard C functions ought to be simpler than ALLEGRO_PATH here. The code seems ambivalent as to whether fs_phys_cwd can be NULL or not. There's no way to set it back to NULL.
|
|
Todd Cope
Member #998
November 2000
|
Peter Wang said: That global is kind of ugly, and a mandatory memory leak. Not sure what you mean by global, it is already declared as static. Quote: One solution is to use a static buffer... Building the paths with the ustr routines or standard C functions ought to be simpler than ALLEGRO_PATH here. That's probably what I'm going to end up doing. I was wanting to do this originally, I'm just used to using Allegro's path routines any time I need to manipulate paths. Append: The updated patch is attached to the first post. The current working directory is now stored in a static buffer. |
|
Peter Wang
Member #23
April 2000
|
Hi Todd, al_change_directory with an absolute path doesn't work correctly. e.g. al_change_directory("a"); al_change_directory("/b"); produces the cwd "/a//b/" The al_ref_buffer call has a wrong length argument. It should be al_ref_cstr. fs_phys_return_path is not thread-safe. AFAICS you can stack allocate it.
|
|
Todd Cope
Member #998
November 2000
|
Peter Wang said: al_change_directory with an absolute path doesn't work correctly. e.g. al_change_directory("a"); al_change_directory("/b"); produces the cwd "/a//b/" The al_ref_buffer call has a wrong length argument. It should be al_ref_cstr. Alright, I've fixed these issues. Quote: fs_phys_return_path is not thread-safe. AFAICS you can stack allocate it. I assume you want each thread to have its own copy of fs_phys_return_path[]. I don't really know how to accomplish this. I've been looking up information about thread-local storage, but I don't know if that is what I need to be doing here. |
|
Peter Wang
Member #23
April 2000
|
You just need to declare a local array in the functions and return the resulting path through that.
|
|
Todd Cope
Member #998
November 2000
|
I've attached a new patch to the first post. I have tested this one more thoroughly so it should be working correctly now. The final path is now constructed on the stack so it should work properly across threads as well. |
|
Peter Wang
Member #23
April 2000
|
Applied, thanks. I made some simplifications (and a fix, I think) to the code. Let me know if I broke something.
|
|
|
