-
Notifications
You must be signed in to change notification settings - Fork 521
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
support LUS::Archive::LoadFileRaw
no longer being exposed
#3999
support LUS::Archive::LoadFileRaw
no longer being exposed
#3999
Conversation
## what i did * moved `tts.cpp` away from using `LoadFileRaw` by creating a new `RawJson` resource type * added `SpeechLogger` so i could verify everything was still working (we don't have a speech synth on linux)
auto t = archive.LoadFileRaw("portVersion"); | ||
auto t = archive.LoadFile("portVersion", std::make_shared<LUS::ResourceInitData>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only SoH side change that isn't part of #3998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So LoadFileRaw
is essentially replaced by LoadFile
with a non-null initdata passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more or less, if you look at LoadFile
in archive now https://github.com/Kenix3/libultraship/blob/d70fd7de2315388c9a99742fd6c634b2cd8be138/src/resource/archive/Archive.cpp#L206-L241 you'll see
std::shared_ptr<File> fileToLoad = nullptr;
if (initData != nullptr) {
fileToLoad = LoadFileRaw(filePath);
fileToLoad->InitData = initData;
} else {
// [...]
}
return fileToLoad;
but using it directly still feels hacky.
i'm also not sure if kenix wants to move LoadFile
itself away from being public. if that's the case i'll need to refactor ReadPortVersionFromOTR
to use a temporary ResourceManager
(which also feels hacky) and that will definitely take a bit more code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it gets to that point, then we could probably just re-think how portVersion
is represented in the archive itself. As long as the update with it lands on a major/minor update then our version detection logic would enforce a regeneration anyways, so the user experience would be the same either way without us having to maintain a deprecated solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it gets to that point, then we could probably just re-think how
portVersion
is represented in the archive itself.
for sure, it's really just a matter of figuring out how we want to handle needing to read from archives when we don't have a resource manager yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just asked kenix about LoadFile
Public is fine, but you're encouraged to use ResourceManager instead of either ArchiveManager or Archive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a reasonable way to handle this use case, if we think of a better one we can figure out how we want to implement it, but we definitely won't need to hack together a temporary resourcemanager based solution
LUS::Archive::LoadFileRaw
no longer being exposed
Depends on:
LoadFileRaw
fromArchiveManager
, move to protected inArchive
Kenix3/libultraship#459Build Artifacts