Assets are slightly optimized

src/dusk/asset/asset.c

@@ -59,20 +59,29 @@ assetentry_t * assetGetEntry(
     entry++;
   } while(entry < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
   
-  // We did not find one existing, Find first available slot.
-  entry = ASSET.entries;
-  do {
-    if(entry->type != ASSET_LOADER_TYPE_NULL) {
-      entry++;
-      continue;
-    }
+  // We did not find one existing. Find first available slot, reaping
+  // zero-ref entries to make room if none are immediately available.
+  bool_t reaped = false;
+  for(;;) {
+    entry = ASSET.entries;
+    do {
+      if(entry->type != ASSET_LOADER_TYPE_NULL) {
+        entry++;
+        continue;
+      }
 
-    if(entry->state == ASSET_ENTRY_STATE_NOT_STARTED) {
-      assetEntryInit(entry, name, type, input);
-      return entry;
-    }
-    entry++;
-  } while(entry < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
+      if(entry->state == ASSET_ENTRY_STATE_NOT_STARTED) {
+        printf("Creating new asset entry for %s\n", name);
+        assetEntryInit(entry, name, type, input);
+        return entry;
+      }
+      entry++;
+    } while(entry < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
+
+    if(reaped) break;
+    reaped = true;
+    errorCatch(assetReapUnused());
+  }
 
   assertUnreachable("No available asset entry slots.");
   return NULL;
@@ -191,6 +200,32 @@ void assetUnlockEntry(assetentry_t *entry) {
   assetEntryUnlock(entry);
 }
 
+errorret_t assetReapUnused(void) {
+  assertIsMainThread("assetReapUnused must be called from the main thread.");
+
+  // Repeatedly find and dispose zero-ref LOADED entries until none remain.
+  // This handles dependency chains where an entry (e.g. a model) holds refs
+  // on child entries (mesh, texture): dispose parents first so child ref
+  // counts drop to zero, then pick up the children on the next pass. Without
+  // this, a forward-only scan fails when a shared child entry appears before
+  // a parent that still holds a ref to it.
+  bool_t any;
+  do {
+    any = false;
+    assetentry_t *entry = ASSET.entries;
+    do {
+      if(entry->type == ASSET_LOADER_TYPE_NULL) { entry++; continue; }
+      if(entry->state != ASSET_ENTRY_STATE_LOADED) { entry++; continue; }
+      if(entry->refs.count > 0) { entry++; continue; }
+      errorChain(assetEntryDispose(entry));
+      any = true;
+      entry++;
+    } while(entry < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
+  } while(any);
+
+  errorOk();
+}
+
 errorret_t assetUpdate(void) {
   assertIsMainThread("assetUpdate must be called from the main thread.");
 
@@ -324,30 +359,6 @@ errorret_t assetUpdate(void) {
     }
   } while(loading < ASSET.loading + ASSET_LOADING_COUNT_MAX);
 
-
-  // Reap unused entries.
-  entry = ASSET.entries;
-  do {
-    if(entry->state != ASSET_ENTRY_STATE_LOADED) {
-      entry++;
-      continue;
-    }
-
-    if(entry->type == ASSET_LOADER_TYPE_NULL) {
-      entry++;
-      continue;
-    }
-
-    if(entry->refs.count > 0) {
-      entry++;
-      continue;
-    }
-
-    // consolePrint("Reaping asset %s", entry->name);
-    errorChain(assetEntryDispose(entry));
-    entry++;
-  } while(entry < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
-
   errorOk();
 }
 
@@ -413,26 +424,7 @@ errorret_t assetDispose(void) {
   assertIsMainThread("Must be called from the main thread.");
   threadStop(&ASSET.loadThread);
 
-  // Drain-dispose: repeatedly find and dispose zero-ref LOADED entries
-  // until none remain. This handles dependency chains where an entry
-  // (e.g. a model) holds refs on child entries (mesh, texture): dispose
-  // parents first so child ref counts drop to zero, then pick up the
-  // children on the next pass. Without this, a forward-only scan fails
-  // when a shared child entry appears before a parent that still holds a
-  // ref to it.
-  bool_t any;
-  do {
-    any = false;
-    assetentry_t *e = ASSET.entries;
-    do {
-      if(e->type == ASSET_LOADER_TYPE_NULL) { e++; continue; }
-      if(e->state != ASSET_ENTRY_STATE_LOADED) { e++; continue; }
-      if(e->refs.count > 0) { e++; continue; }
-      errorChain(assetEntryDispose(e));
-      any = true;
-      e++;
-    } while(e < ASSET.entries + ASSET_ENTRY_COUNT_MAX);
-  } while(any);
+  errorChain(assetReapUnused());
 
   // Cleanup zip file.
   if(ASSET.zip != NULL) {

src/dusk/asset/asset.h

@@ -23,8 +23,8 @@
 #define ASSET_FILE_NAME "dusk.dsk"
 #define ASSET_HEADER_SIZE 3
 
-#define ASSET_LOADING_COUNT_MAX 20
-#define ASSET_ENTRY_COUNT_MAX 128
+#define ASSET_LOADING_COUNT_MAX 32
+#define ASSET_ENTRY_COUNT_MAX 64
 
 typedef struct asset_s {
   zip_t *zip;
@@ -112,6 +112,16 @@ void assetUnlock(const char_t *name);
  */
 void assetUnlockEntry(assetentry_t *entry);
 
+/**
+ * Frees every currently unreferenced (zero-ref) loaded asset entry. Repeats
+ * until a full pass frees nothing further, since disposing a parent entry
+ * (e.g. a model) may drop a child entry's (e.g. a mesh) ref count to zero,
+ * making it eligible for reaping too.
+ *
+ * @return An error code if any entry could not be disposed properly.
+ */
+errorret_t assetReapUnused(void);
+
 /**
  * Requires an asset entry to be loaded. This will block until the asset entry
  * is fully loaded.

src/dusk/asset/loader/chunk/assetchunkloader.c

@@ -111,6 +111,7 @@ errorret_t assetChunkLoaderSync(assetloading_t *loading) {
   size_t offset = 8;
 
   size_t tileSize = CHUNK_TILE_COUNT * sizeof(tile_t);
+  out->tiles = memoryAllocate(tileSize);
   memoryCopy(out->tiles, data + offset, tileSize);
   offset += tileSize;
 
@@ -157,6 +158,12 @@ errorret_t assetChunkDispose(assetentry_t *entry) {
   assertIsMainThread("Must be called from the main thread.");
 
   assetchunkoutput_t *out = &entry->data.chunk;
+
+  if(out->tiles != NULL) {
+    memoryFree(out->tiles);
+    out->tiles = NULL;
+  }
+
   for(uint8_t m = 0; m < out->meshCount; m++) {
     if(out->modelEntries[m] == NULL) continue;
     assetUnlockEntry(out->modelEntries[m]);

src/dusk/asset/loader/chunk/assetchunkloader.h

@@ -34,7 +34,7 @@ typedef struct {
 } assetchunkloaderloading_t;
 
 typedef struct {
-  tile_t tiles[CHUNK_TILE_COUNT];
+  tile_t *tiles;
   uint8_t meshCount;
   char_t modelNames[CHUNK_MESH_COUNT_MAX][CHUNK_MESH_NAME_MAX];
   vec3 meshOffsets[CHUNK_MESH_COUNT_MAX];

src/dusk/rpg/overworld/map.c

@@ -157,9 +157,10 @@ void mapChunkUnload(chunk_t *chunk) {
     chunk->dcfEntry = NULL;
   }
 
+  // modelEntries are borrowed pointers, not independently locked - the
+  // chunk asset entry (released above) is what actually holds the ref on
+  // each model, so nothing to unlock here, just drop our own copies.
   for(uint8_t m = 0; m < chunk->meshCount; m++) {
-    if(chunk->modelEntries[m] == NULL) continue;
-    assetUnlockEntry(chunk->modelEntries[m]);
     chunk->modelEntries[m] = NULL;
   }
   chunk->meshCount = 0;
@@ -197,9 +198,23 @@ errorret_t mapChunkLoad(chunk_t *chunk) {
 
   assetentry_t *entry = assetLock(name, ASSET_LOADER_TYPE_CHUNK, NULL);
   assertNotNull(entry, "Failed to get chunk asset entry");
+  chunk->dcfEntry = entry;
+
+  // The entry may already be resident from an earlier load that hasn't been
+  // reaped yet - in that case onLoaded/onError already fired once and never
+  // will again, so handle the terminal state directly instead of waiting on
+  // a subscription that would never trigger.
+  if(entry->state == ASSET_ENTRY_STATE_LOADED) {
+    mapChunkLoaded(entry, chunk);
+    errorOk();
+  }
+  if(entry->state == ASSET_ENTRY_STATE_ERROR) {
+    mapChunkLoadError(entry, chunk);
+    errorOk();
+  }
+
   eventSubscribe(&entry->onLoaded, mapChunkLoaded, chunk);
   eventSubscribe(&entry->onError, mapChunkLoadError, chunk);
-  chunk->dcfEntry = entry;
   errorOk();
 }
 
@@ -331,14 +346,17 @@ void mapRebuildChunkOrder() {
 void mapChunkLoadError(void *params, void *user) {
   assertNotNull(params, "mapChunkLoadError: params cannot be NULL");
   assertNotNull(user, "mapChunkLoadError: user cannot be NULL");
+  assetentry_t *entry = (assetentry_t *)params;
   chunk_t *chunk = (chunk_t *)user;
-  if(chunk->dcfEntry != (assetentry_t *)params) return;
+  if(chunk->dcfEntry != entry) return;
   consolePrint(
     "Chunk load error: %d %d %d",
     (int32_t)chunk->position.x,
     (int32_t)chunk->position.y,
     (int32_t)chunk->position.z
   );
+  eventUnsubscribe(&entry->onLoaded, mapChunkLoaded);
+  eventUnsubscribe(&entry->onError, mapChunkLoadError);
   assetUnlockEntry(chunk->dcfEntry);
   chunk->dcfEntry = NULL;
   memorySet(chunk->tiles, 0x00, sizeof(chunk->tiles));
@@ -385,10 +403,19 @@ void mapChunkLoaded(void *params, void *user) {
     vec3 pos;
     glm_vec3_add(wpf, scaledOffset, pos);
     glm_translate_make(chunk->meshModels[m], pos);
+    // Borrow the pointer rather than stealing it - the chunk asset entry
+    // keeps its own lock on each model (taken once while it loaded) and we
+    // keep the chunk asset entry itself locked (see below), so the models
+    // stay valid for as long as this chunk_t is using them. The entry may
+    // now be reused by a later mapChunkLoad for a different chunk_t once we
+    // eventually unlock it in mapChunkUnload, at which point its
+    // modelEntries must still be intact for that next reuse to copy from.
     chunk->modelEntries[m] = entry->data.chunk.modelEntries[m];
-    entry->data.chunk.modelEntries[m] = NULL;
   }
-  assetUnlockEntry(chunk->dcfEntry);
-  chunk->dcfEntry = NULL;
+  eventUnsubscribe(&entry->onLoaded, mapChunkLoaded);
+  eventUnsubscribe(&entry->onError, mapChunkLoadError);
+  // Deliberately keep chunk->dcfEntry locked and set - it is what keeps the
+  // chunk asset entry (and therefore its model locks) alive for as long as
+  // this chunk_t is displaying it. Released in mapChunkUnload instead.
   chunk->meshCount = meshCount;
 }

test/asset/test_asset.c

@@ -468,6 +468,102 @@ static void test_requireLoaded_propagates_error(void **state) {
   assert_int_equal(memoryGetAllocatedCount(), 0);
 }
 
+// ============================================================
+// assetReapUnused tests
+// ============================================================
+
+static void test_update_does_not_reap_automatically(void **state) {
+  assetentry_t *entry = assetGetEntry("test.locale", ASSET_LOADER_TYPE_LOCALE, NULL);
+
+  assetEntryLock(entry);
+  assetUpdate();
+  assetUpdate();  // slot freed
+  assert_int_equal(entry->state, ASSET_ENTRY_STATE_LOADED);
+  assetEntryUnlock(entry);
+
+  // Unlike the old behavior, assetUpdate no longer reaps zero-ref entries on
+  // its own  -  a LOADED entry must survive further updates untouched.
+  assetUpdate();
+  assetUpdate();
+  assetUpdate();
+
+  assert_int_equal(entry->type, ASSET_LOADER_TYPE_LOCALE);
+  assert_int_equal(entry->state, ASSET_ENTRY_STATE_LOADED);
+
+  errorret_t ret = assetEntryDispose(entry);
+  assert_true(errorIsOk(ret));
+
+  assert_int_equal(memoryGetAllocatedCount(), 0);
+}
+
+static void test_reapUnused_disposes_zero_ref_loaded(void **state) {
+  assetentry_t *entry = assetGetEntry("test.locale", ASSET_LOADER_TYPE_LOCALE, NULL);
+
+  assetEntryLock(entry);
+  assetUpdate();
+  assetUpdate();
+  assert_int_equal(entry->state, ASSET_ENTRY_STATE_LOADED);
+  assetEntryUnlock(entry);
+
+  errorret_t ret = assetReapUnused();
+  assert_true(errorIsOk(ret));
+  assert_int_equal(entry->type, ASSET_LOADER_TYPE_NULL);
+
+  assert_int_equal(memoryGetAllocatedCount(), 0);
+}
+
+static void test_reapUnused_ignores_referenced_entries(void **state) {
+  assetentry_t *entry = assetGetEntry("test.locale", ASSET_LOADER_TYPE_LOCALE, NULL);
+
+  assetEntryLock(entry);
+  assetUpdate();
+  assetUpdate();
+  assert_int_equal(entry->state, ASSET_ENTRY_STATE_LOADED);
+
+  // Still locked - a reap must leave it alone.
+  errorret_t ret = assetReapUnused();
+  assert_true(errorIsOk(ret));
+  assert_int_equal(entry->type, ASSET_LOADER_TYPE_LOCALE);
+
+  assetEntryUnlock(entry);
+  errorret_t disposeRet = assetEntryDispose(entry);
+  assert_true(errorIsOk(disposeRet));
+
+  assert_int_equal(memoryGetAllocatedCount(), 0);
+}
+
+static void test_getEntry_reaps_when_pool_full(void **state) {
+  // Fill every entry slot with a zero-ref LOADED entry.
+  for(int i = 0; i < ASSET_ENTRY_COUNT_MAX; i++) {
+    char_t name[ASSET_FILE_NAME_MAX];
+    snprintf(name, sizeof(name), "full%d.locale", i);
+    assetentry_t *entry = assetGetEntry(name, ASSET_LOADER_TYPE_LOCALE, NULL);
+    assetEntryLock(entry);
+    assetUpdate();
+    assetUpdate();
+    assert_int_equal(entry->state, ASSET_ENTRY_STATE_LOADED);
+    assetEntryUnlock(entry);
+  }
+
+  // The pool is now completely full of zero-ref LOADED entries with no
+  // ASSET_LOADER_TYPE_NULL slots left. Requesting one more must trigger an
+  // implicit reap instead of asserting.
+  assetentry_t *fresh = assetGetEntry("fresh.locale", ASSET_LOADER_TYPE_LOCALE, NULL);
+  assert_non_null(fresh);
+  assert_int_equal(fresh->state, ASSET_ENTRY_STATE_NOT_STARTED);
+
+  assetEntryLock(fresh);
+  assetUpdate();
+  assetUpdate();
+  assert_int_equal(fresh->state, ASSET_ENTRY_STATE_LOADED);
+  assetEntryUnlock(fresh);
+
+  errorret_t ret = assetEntryDispose(fresh);
+  assert_true(errorIsOk(ret));
+
+  assert_int_equal(memoryGetAllocatedCount(), 0);
+}
+
 // ============================================================
 // main
 // ============================================================
@@ -503,6 +599,12 @@ int main(void) {
     cmocka_unit_test_setup_teardown(test_requireLoaded_already_loaded, asset_setup, asset_teardown),
     cmocka_unit_test_setup_teardown(test_requireLoaded_spins_to_loaded, asset_setup, asset_teardown),
     cmocka_unit_test_setup_teardown(test_requireLoaded_propagates_error, asset_setup, asset_teardown),
+
+    // assetReapUnused
+    cmocka_unit_test_setup_teardown(test_update_does_not_reap_automatically, asset_setup, asset_teardown),
+    cmocka_unit_test_setup_teardown(test_reapUnused_disposes_zero_ref_loaded, asset_setup, asset_teardown),
+    cmocka_unit_test_setup_teardown(test_reapUnused_ignores_referenced_entries, asset_setup, asset_teardown),
+    cmocka_unit_test_setup_teardown(test_getEntry_reaps_when_pool_full, asset_setup, asset_teardown),
   };
   return cmocka_run_group_tests(tests, NULL, NULL);
 }