Author Topic: [CODE REVIEW] EFF subdirectories  (Read 3394 times)

0 Members and 1 Guest are viewing this topic.

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
[CODE REVIEW] EFF subdirectories
This patch adds a new optional entry to .eff files which allows the frames to be placed in their own subdirectory. So, if you have effects/boom.eff, you may place the frames in effects/boom/boom_0000.dds, effects/boom/boom_0001.dds and so on, making your effects directory much tidier and easier to navigate. This is intended to work for any .eff file anywhere.

Example of use:
Code: [Select]
$Type: DDS
$Frames: 14
$FPS: 15
$Subdir: YES

Patch (same as attachment):
Code: [Select]
Index: bmpman/bm_internal.h
===================================================================
--- bmpman/bm_internal.h (revision 10988)
+++ bmpman/bm_internal.h (working copy)
@@ -61,6 +61,7 @@
  // stuff for static animations
  ubyte type; // type for individual images
  char filename[MAX_FILENAME_LEN]; // filename for individual images
+ bool in_subdir; // Whether frames are in their own subdirectory
  } eff;
  } ani;
 
Index: bmpman/bmpman.cpp
===================================================================
--- bmpman/bmpman.cpp (revision 10988)
+++ bmpman/bmpman.cpp (working copy)
@@ -38,11 +38,13 @@
 #include "parse/parselo.h"
 #include "network/multiutil.h"
 #include "debugconsole/console.h"
+#include "cfile/cfilesystem.h"
 
 #define BMPMAN_INTERNAL
 #include "bmpman/bm_internal.h"
 
 extern int Cmdline_cache_bitmaps;
+extern cf_pathtype Pathtypes[CF_MAX_PATH_TYPES];
 
 #ifndef NDEBUG
 #define BMPMAN_NDEBUG
@@ -73,7 +75,20 @@
 static int Bm_ignore_duplicates = 0;
 static int Bm_ignore_load_count = 0;
 
-#define EFF_FILENAME_CHECK { if ( be->type == BM_TYPE_EFF ) strcpy_s( filename, be->info.ani.eff.filename ); else strcpy_s( filename, be->filename ); }
+#define EFF_FILENAME_CHECK \
+{ \
+ if ( be->type == BM_TYPE_EFF ) { \
+ strcpy_s( filename, be->info.ani.eff.filename ); \
+ if (be->info.ani.eff.in_subdir) { \
+ if (!set_temp_subdir_pathtype(bm_bitmaps[be->info.ani.first_frame].filename)) { \
+ mprintf(("BMPMAN: Failed to set temporary pathtype for %s: EFF_FILENAME_CHECK failed!\n", be->filename)); \
+ } \
+ } \
+ } \
+ else { \
+ strcpy_s( filename, be->filename ); \
+ } \
+}
 
 
 
@@ -745,9 +760,10 @@
  return -1;
 }
 
-int bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int *nfps, int *key, ubyte *type)
+int bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int *nfps, int *key, ubyte *type, bool *in_subdir)
 {
  int frames = 0, fps = 30, keyframe = 0, rval;
+ bool subdir = false;
  char ext[8];
  ubyte c_type = BM_TYPE_NONE;
  char file_text[1024];
@@ -782,6 +798,9 @@
  if (optional_string( "$Keyframe:" ))
  stuff_int(&keyframe);
 
+ if (optional_string( "$Subdir:" )) {
+ stuff_boolean(&subdir);}
+
  // done with EFF so unpause parsing so whatever can continue
  unpause_parse();
 
@@ -818,6 +837,8 @@
  if (key)
  *key = keyframe;
 
+ *in_subdir = subdir;
+
  return 0;
 }
 
@@ -841,6 +862,7 @@
  char filename[MAX_FILENAME_LEN];
  int reduced = 0;
  int anim_fps = 0, anim_frames = 0, key = 0;
+ bool in_subdir = false;
  int anim_width = 0, anim_height = 0;
  ubyte type = BM_TYPE_NONE, eff_type = BM_TYPE_NONE, c_type = BM_TYPE_NONE;
  int bpp = 0, mm_lvl = 0, img_size = 0;
@@ -930,7 +952,7 @@
 
  // it's an effect file, any readable image type with eff being txt
  if (type == BM_TYPE_EFF) {
- if ( bm_load_and_parse_eff(filename, dir_type, &anim_frames, &anim_fps, &key, &eff_type) != 0 ) {
+ if ( bm_load_and_parse_eff(filename, dir_type, &anim_frames, &anim_fps, &key, &eff_type, &in_subdir) != 0 ) {
  mprintf(("BMPMAN: Error reading EFF\n"));
  return -1;
  } else {
@@ -992,6 +1014,10 @@
  return -1;
  }
 
+ if (in_subdir) {
+ set_temp_subdir_pathtype(filename);
+ }
+
  int first_handle = bm_get_next_handle();
 
  Assert ( strlen(filename) < MAX_FILENAME_LEN );
@@ -1056,7 +1082,11 @@
  bm_bitmaps[n+i].last_used = -1;
  bm_bitmaps[n+i].num_mipmaps = mm_lvl;
  bm_bitmaps[n+i].mem_taken = img_size;
- bm_bitmaps[n+i].dir_type = dir_type;
+ bm_bitmaps[n+i].info.ani.eff.in_subdir = in_subdir;
+ if (in_subdir)
+ bm_bitmaps[n+i].dir_type = CF_TYPE_TEMP_SUBDIR_LOOKUP;
+ else
+ bm_bitmaps[n+i].dir_type = dir_type;
 
  bm_bitmaps[n+i].load_count++;
 
@@ -1733,8 +1763,8 @@
  // read the file data
  if ( gr_bm_lock( be->filename, handle, bitmapnum, bpp, flags, nodebug ) == -1 ) {
  // oops, this isn't good - reset and return NULL
- bm_unlock( bitmapnum );
- bm_unload( bitmapnum );
+ bm_unlock( handle );
+ bm_unload( handle );
 
  return NULL;
  }
@@ -2281,6 +2311,62 @@
 
 extern void multi_ping_send_all();
 
+/**
+ * Stuffs Pathtypes with an additional path leading to a subdir of the same name as the given file (minus file extension).
+ * Used for allowing frames of .eff animations to be found in their own subdirectories.
+ *
+ * Example: if called with "debris.eff" and that file is in data/maps, then Pathtypes[CF_TYPE_TEMP_EFF_LOOKUP].path
+ * will be stuffed with data/maps/debris, allowing data/maps/debris/debris_0000.dds (etc) to be found
+ */
+bool set_temp_subdir_pathtype(const char *filename) {
+ // Gets the absolute path of the .eff file, removes the part that is
+ // identical with the absolute root path and appends the .eff filename
+ // to it (without extension), giving us a relative path that can be
+ // temporarily stuffed into Pathtypes so the frames can be found
+
+ char fullpath[MAX_PATH]; // Absolute path to the given file
+ char fullrootpath[MAX_PATH]; // Absolute root path
+ int size, offset; // Needed for cf_find_file_location, but not used
+
+ cf_create_default_path_string(fullrootpath, sizeof(fullrootpath) - 1, CF_TYPE_ROOT, NULL);
+ if (!cf_find_file_location(filename, CF_TYPE_ANY, sizeof(fullpath)-1, fullpath, &size, &offset)) {
+ mprintf(("BMPMAN: Failed to set temporary pathtype for %s: file not found!\n", filename));
+ return false;
+ }
+
+#ifndef NDEBUG
+ if (strlen(fullpath) < strlen(fullrootpath)) {
+ mprintf(("BMPMAN: Path to %s was shorter than the root path, programmer thought it could never happen!\n%s\n%s\n", filename, fullpath, fullrootpath));
+ return false;
+ }
+#endif
+
+ mprintf(("BMPMAN: Setting temporary pathtype for %s... ", filename));
+
+ char *trimmed_path; // The differing portion of fullpath and fullrootpath
+ trimmed_path = &fullpath[strlen(fullrootpath)];
+
+ char *extpos = strstr(trimmed_path, "."); // Where the file extension begins
+ char basepath[MAX_PATH]; // This will hold the final, relative path
+
+ // Copy the relative path of the given file to basepath, excluding the extension
+ strncpy(basepath, trimmed_path, extpos-trimmed_path);
+
+ // Make sure it's null-terminated right
+ basepath[(int)(extpos-trimmed_path)] = '\0';
+
+ // Free the old string first, if it's not the const initial empty string
+ if (Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path[0] != '\0') {
+ vm_free(Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path);
+ }
+
+ Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path = vm_strdup(basepath);
+
+ mprintf(("OK. Temporary pathtype is now %s.\n", Pathtypes[CF_TYPE_TEMP_SUBDIR_LOOKUP].path));
+
+ return true;
+}
+
 void bm_page_in_stop()
 {
  int i;
@@ -2300,6 +2386,10 @@
  if ( (bm_bitmaps[i].type != BM_TYPE_NONE) && (bm_bitmaps[i].type != BM_TYPE_RENDER_TARGET_DYNAMIC) && (bm_bitmaps[i].type != BM_TYPE_RENDER_TARGET_STATIC) ) {
  if ( bm_bitmaps[i].preloaded ) {
  if ( bm_preloading ) {
+ if (bm_bitmaps[i].type == BM_TYPE_EFF && bm_bitmaps[i].info.ani.eff.in_subdir) {
+ set_temp_subdir_pathtype(bm_bitmaps[i].filename);
+ }
+
  if ( !gr_preload(bm_bitmaps[i].handle, (bm_bitmaps[i].preloaded==2)) ) {
  mprintf(( "Out of VRAM.  Done preloading.\n" ));
  bm_preloading = 0;
@@ -2306,7 +2396,9 @@
  }
  } else {
  bm_lock( bm_bitmaps[i].handle, (bm_bitmaps[i].used_flags == BMP_AABITMAP) ? 8 : 16, bm_bitmaps[i].used_flags );
- bm_unlock( bm_bitmaps[i].handle );
+ if (bm_bitmaps[i].ref_count >= 1) {
+ bm_unlock( bm_bitmaps[i].handle );
+ }
  }
 
  n++;
Index: bmpman/bmpman.h
===================================================================
--- bmpman/bmpman.h (revision 10988)
+++ bmpman/bmpman.h (working copy)
@@ -175,6 +175,9 @@
 // Reload a different image into an existing bmpman slot
 int bm_reload(int bitmap_handle, const char* filename);
 
+// Stuff Pathtypes with an additional path leading to a subdir of the same name as the given file (minus file extension)
+bool set_temp_subdir_pathtype(const char *filename);
+
 /*
  * Example on using bm_create
  *
@@ -284,5 +287,5 @@
 int bm_is_render_target(int bitmap_id);
 int bm_set_render_target(int handle, int face = -1);
 
-int bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int *nfps, int *key, ubyte *type);
+int bm_load_and_parse_eff(const char *filename, int dir_type, int *nframes, int *nfps, int *key, ubyte *type, bool *in_subdir);
 #endif
Index: cfile/cfile.cpp
===================================================================
--- cfile/cfile.cpp (revision 10988)
+++ cfile/cfile.cpp (working copy)
@@ -87,6 +87,7 @@
  { CF_TYPE_INTEL_ANIMS, "data" DIR_SEPARATOR_STR "intelanims", ".pcx .ani .eff .tga .jpg .png .dds", CF_TYPE_DATA },
  { CF_TYPE_SCRIPTS, "data" DIR_SEPARATOR_STR "scripts", ".lua .lc", CF_TYPE_DATA },
  { CF_TYPE_FICTION, "data" DIR_SEPARATOR_STR "fiction", ".txt", CF_TYPE_DATA },
+ { CF_TYPE_TEMP_SUBDIR_LOOKUP, "", ".eff", CF_TYPE_DATA },
 };
 
 
Index: cfile/cfile.h
===================================================================
--- cfile/cfile.h (revision 10988)
+++ cfile/cfile.h (working copy)
@@ -78,8 +78,9 @@
 #define CF_TYPE_INTEL_ANIMS 34
 #define CF_TYPE_SCRIPTS 35
 #define CF_TYPE_FICTION 36
+#define CF_TYPE_TEMP_SUBDIR_LOOKUP 37
 
-#define CF_MAX_PATH_TYPES 37 // Can be as high as you'd like //DTP; yeah but beware alot of things uses CF_MAX_PATH_TYPES
+#define CF_MAX_PATH_TYPES 38 // Can be as high as you'd like //DTP; yeah but beware alot of things uses CF_MAX_PATH_TYPES
 
 
 // TRUE if type is specified and valid
Index: cfile/cfilesystem.cpp
===================================================================
--- cfile/cfilesystem.cpp (revision 10988)
+++ cfile/cfilesystem.cpp (working copy)
@@ -901,6 +901,7 @@
  case CF_TYPE_MULTI_CACHE:
  case CF_TYPE_MISSIONS:
  case CF_TYPE_CACHE:
+ case CF_TYPE_TEMP_SUBDIR_LOOKUP:
  cfs_slow_search = 1;
  break;
 
Index: graphics/generic.cpp
===================================================================
--- graphics/generic.cpp (revision 10988)
+++ graphics/generic.cpp (working copy)
@@ -209,13 +209,19 @@
  ga->previous_frame = -1;
  }
  else {
+ bool in_subdir;
  bpp = 32;
  if(ga->use_hud_color)
  bpp = 8;
- bm_load_and_parse_eff(ga->filename, CF_TYPE_ANY, &ga->num_frames, &anim_fps, &ga->keyframe, 0);
+ bm_load_and_parse_eff(ga->filename, CF_TYPE_ANY, &ga->num_frames, &anim_fps, &ga->keyframe, 0, &in_subdir);
  char *p = strrchr( ga->filename, '.' );
  if ( p )
  *p = 0;
+
+ if (in_subdir) {
+ set_temp_subdir_pathtype(ga->filename);
+ }
+
  char frame_name[32];
  snprintf(frame_name, 32, "%s_0000", ga->filename);
  ga->bitmap_id = bm_load(frame_name);

Since I had to dive into the bowels of bmpman and do some changes there, I'd very much like to see the feature reasonably well tested by others as well. There's a few lines of seemingly buggy bmpman code that I included the fixes for in the patch (the bm_unlock and bm_unload calls) which I'll probably commit separately.

Drawbacks? There's only one that I can think of: the subdirectoried frames cannot be referred to from elsewhere, so you can't for example re-use your effects/bigthruster/bigthruster_0012.dds as a laser glow with @Laser Glow: bigthruster/bigthruster_0012 (not that I know if that works even now), nor will they be found by Lua functions such as gr.loadTexture(). Seems like a fair trade-off, especially when I'll off course document that.

P.S. I believe this would make it very easy to add similar functionality to other file types as well.

EDIT: Updated patch.

[attachment kidnapped by pirates]
« Last Edit: August 11, 2014, 10:56:09 am by zookeeper »

 

Offline Colonol Dekker

  • HLP is my mistress
  • 213
  • Aken Tigh Dekker- you've probably heard me
    • My old squad sub-domain
Re: [CODE REVIEW] EFF subdirectories
This is bloody awesome.
Thanks very much mate. :)
Campaigns I've added my distinctiveness to-
- Blue Planet: Battle Captains
-Battle of Neptune
-Between the Ashes 2
-Blue planet: Age of Aquarius
-FOTG?
-Inferno R1
-Ribos: The aftermath / -Retreat from Deneb
-Sol: A History
-TBP EACW teaser
-Earth Brakiri war
-TBP Fortune Hunters (I think?)
-TBP Relic
-Trancsend (Possibly?)
-Uncharted Territory
-Vassagos Dirge
-War Machine
(Others lost to the mists of time and no discernible audit trail)

Your friendly Orestes tactical controller.

Secret bomb God.
That one time I got permabanned and got to read who was being bitxhy about me :p....
GO GO DEKKER RANGERSSSS!!!!!!!!!!!!!!!!!
President of the Scooby Doo Model Appreciation Society
The only good Zod is a dead Zod
NEWGROUNDS COMEDY GOLD, UPDATED DAILY
http://badges.steamprofile.com/profile/default/steam/76561198011784807.png

 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
Well, I've found one bug myself: in some situations (at least for fireball anims) the images can apparently get loaded through a codepath which doesn't set the temp lookup path, leading to the images to not be found if there are several subdirectoried anims. Will fix, just not sure how, yet.

 

Offline Spoon

  • 212
  • ヾ(´︶`♡)ノ
Re: [CODE REVIEW] EFF subdirectories
I'm all for added organization  :yes:, the effects and hud folder has become increasingly crowded with .eff sequences.
Urutorahappī!!

[02:42] <@Axem> spoon somethings wrong
[02:42] <@Axem> critically wrong
[02:42] <@Axem> im happy with these missions now
[02:44] <@Axem> well
[02:44] <@Axem> with 2 of them

 

Offline Goober5000

  • HLP Loremaster
  • Moderator
  • 214
    • Goober5000 Productions
Re: [CODE REVIEW] EFF subdirectories
Organizing EFF files is useful, but I would be wary of messing with the file system.  The CFILE code is more difficult than it looks.  Have you tested the EFF subdirectory thing when all the files are packaged in a VP?  Or accessed via -mod?  Or both?

 

Offline Mongoose

  • Rikki-Tikki-Tavi
  • Global Moderator
  • 212
  • This brain for rent.
    • Minecraft
    • Steam
    • Something
Re: [CODE REVIEW] EFF subdirectories
Speaking of, has there been any word from the APNGASM guys about getting animated PNG support into the engine?  We did collectively shell out a pretty hefty sum to make it happen.

 
 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
Organizing EFF files is useful, but I would be wary of messing with the file system.  The CFILE code is more difficult than it looks.  Have you tested the EFF subdirectory thing when all the files are packaged in a VP?  Or accessed via -mod?  Or both?

I've only tested a test mod accessed via -mod (no VPs) and in a TC setting (no VPs) so far.

Anyway, this doesn't really mess with the filesystem code as such. It just stuffs the relevant new relative path into the array of paths things are looked for in, but as mentioned in my previous post, that part's still imperfect.

 

Offline mjn.mixael

  • Cutscene Master
  • 212
  • Chopped liver
    • Steam
    • Twitter
Re: [CODE REVIEW] EFF subdirectories
Organizing EFF files is useful, but I would be wary of messing with the file system.  The CFILE code is more difficult than it looks.  Have you tested the EFF subdirectory thing when all the files are packaged in a VP?  Or accessed via -mod?  Or both?

I've only tested a test mod accessed via -mod (no VPs) and in a TC setting (no VPs) so far.

Anyway, this doesn't really mess with the filesystem code as such. It just stuffs the relevant new relative path into the array of paths things are looked for in, but as mentioned in my previous post, that part's still imperfect.

Le wut... no, you should probably test all the typical uses. Famous last words for FSO patches.. "it shouldn't break that part of the code".
Cutscene Upgrade Project - Mainhall Remakes - Between the Ashes
Youtube Channel - P3D Model Box
Between the Ashes is looking for committed testers, PM me for details.
Freespace Upgrade Project See what's happening.

 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
Le wut... no, you should probably test all the typical uses. Famous last words for FSO patches.. "it shouldn't break that part of the code".

Yes, hence this thread.

 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
Ok, looks like I managed to fix the aforementioned issue. First post updated with the new patch (only thing I changed was the EFF_FILENAME_CHECK macro in bmpman.cpp).

 

Offline fightermedic

  • 29
  • quite a nice guy, no really, i am
Re: [CODE REVIEW] EFF subdirectories
i love you for doing this
great idea
>>Fully functional cockpits for Freespace<<
>>Ships created by me<<
Campaigns revised/voice-acted by me:
Lightning Marshal 1-4, The Regulus Campaign, Operation: Savior, Operation: Crucible, Titan Rebellion, Fall of Epsilon Pegasi 1.1Aftermath 2.1,
Pandora's Box 2.2, Deep Blood

Other Campaigns I have participated in:
The Antagonist, Warzone, Phantoms & Echo-Gate

All the stuff I release is free to use or change in any way for everybody who likes to do so; take whatever you need

 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
So, anyone willing to give this more testing? VP files, multimods, replacing anims from retail or other mods, that sort of thing.

I have it in my branch for FotG use, but before it can go in trunk (probably only after 3.7.2 in any case, I'm afraid) it'd really need more testing in various mod environments.

 

Offline zookeeper

  • *knock knock* Who's there? Poe. Poe who?
  • 210
Re: [CODE REVIEW] EFF subdirectories
Maybe these windows builds (based on current master) will elicit some testing of the aforementioned kind (VP's, multiple mods, etc):

https://dl.dropboxusercontent.com/u/63964618/fs/eff_subdirs_build/fs2_open_3_7_3_SSE2.exe
https://dl.dropboxusercontent.com/u/63964618/fs/eff_subdirs_build/fs2_open_3_7_3_SSE2-DEBUG.exe