diff --git a/src/dusk/asset/asset.c b/src/dusk/asset/asset.c index f914092c..1eaeb0f0 100644 --- a/src/dusk/asset/asset.c +++ b/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) { diff --git a/src/dusk/asset/asset.h b/src/dusk/asset/asset.h index 2520f94d..04468023 100644 --- a/src/dusk/asset/asset.h +++ b/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. diff --git a/src/dusk/asset/loader/chunk/assetchunkloader.c b/src/dusk/asset/loader/chunk/assetchunkloader.c index 871f64c0..505126c6 100644 --- a/src/dusk/asset/loader/chunk/assetchunkloader.c +++ b/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]); diff --git a/src/dusk/asset/loader/chunk/assetchunkloader.h b/src/dusk/asset/loader/chunk/assetchunkloader.h index d7936b04..db591a7f 100644 --- a/src/dusk/asset/loader/chunk/assetchunkloader.h +++ b/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]; diff --git a/src/dusk/rpg/overworld/map.c b/src/dusk/rpg/overworld/map.c index 5d54cebf..984f4299 100644 --- a/src/dusk/rpg/overworld/map.c +++ b/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; } diff --git a/test/asset/test_asset.c b/test/asset/test_asset.c index bdac1404..e0424a4e 100644 --- a/test/asset/test_asset.c +++ b/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); }