Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Asset management #378

Merged
merged 16 commits into from
Dec 29, 2023
Merged

Asset management #378

merged 16 commits into from
Dec 29, 2023

Conversation

rherriman
Copy link
Member

This seems to work perfectly for loading/storing manifests, default.avarascript files, HULL resources (though I still need to implement/call AssetManager::ReviewPriorities for those...), and BSP files. Still need to implement sound loading and preprocessing (combining sample data and sample header data) and related updates to CSoundHub.

I've somewhat separated the concept of an asset "repository" (which
provides a listing of available level sets) from asset "storage" (where
files are stored locally). The idea is that a "RemoveAssetRepository"
type may hold a reference to local storage, where downloaded assets will
actually be saved.

Lastly...the game builds!
It seems to be working well for BSPs and ALF files, but it seems to be
having occasional trouble parsing Avarascript for some reason!
As I suspected the C-string pointer was being invalidated, occasionally
leading to garbage data. This stupid version just copies the string's
inner C-string data into a separate character array that lasts as long
as it needs to in order to run the script. There has to be a better way,
but even loading the script into a C-string initially seemed to present
the same bugs as I was seeing before.
Also rework `RunThis` method of `Parser` to accept `std::string` instead
of C strings.
Wanted to commit before I rip out the old stuff.
HSND base rates were being loaded incorrectly.
@rherriman
Copy link
Member Author

This branch should now have feature parity with main, and all the old (non-resource fork related) functions have been removed from Resource.h.

Audio code updates were more disruptive than I expected but I seem to have smoothed out the initial kinks, which ultimately resulted in a nicer and potentially speedier design.

Memory usage appears to be on par with main, potentially even a tiny bit lower.

These changes also allow level sets to include audio and hull resources from other sets, which was previously not yet possible in main.

}

CBasicSound *CSoundHubImpl::Aquire(short kind) {
CBasicSound *CSoundHubImpl::Acquire(short kind) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely we can nuke the rest of this file with some careful pruning. I'm curious to know what still uses BundlePath and OSTypes

gHub->PreLoadSample(incarnateSound);
gHub->PreLoadSample(winSound);
gHub->PreLoadSample(loseSound);
auto _ = AssetManager::GetOgg(incarnateSound);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really nitpicky, but explicit discard of the function return value has a vaguely threatening energy. the previous function name made really clear what we were doing here, and i think this could use a comment like //Pre-load sounds, and at that point it's fine to implicitly discard and not make anyone look at stuff like auto _ = foo();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this before I realized exactly how many places you put auto _ =. I don't really want to send you chasing all those down, i can get over myself

Copy link
Member

@assertivist assertivist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An incredibly cleanly done work. a vast step forward towards remote level loading. An all around handsome PR.

I also changed the way HullConfigRecords are loaded by the
PlayerManifest, using `.value<T>(...)` instead of `[]` operator syntax.
Partly due to my experience with HSND's base rate, but mostly for
consistency. (According to the debugger the end result is the same.)
@rherriman rherriman merged commit 06c2346 into avaraline:main Dec 29, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants