Unifying the filesystem interface (eg. no more check) #706

FalseIlyu · 2024-04-05T19:22:29Z

The purpose of this PR is to remove the need of checking what type of filesystem to use before reading a file either at compile type (as we have currently to handle android) or at runtime (as proposed in #673).

It divides responsibility as follows :

Parsers get data from a stream and build game asset from it.
The filesystem look for the data and organise it as a istream.
The stream handles direct access to the data.

As a first draft i moved void ReadFile(std::istream& stream) in all parsers from a private to a public method.
Added a std::unique_ptr<std::istream> GetData(const std::filesystem::path& path) as the method to ask access to data from the filesystem.
Replaced place where we had to choose between Open(Locator::filesystem::value().ReadAll(path)); and Open(path) with ReadFile(*Locator::filesystem::value().GetData(path));
Copied the membuf from the component into android and made it own a copy of the file data (mostly a lazy way to ensure the data was there while being read by the buffer) i also allowed it to be initialized by a vector of byte.

I propose this as draft as i wish to have comment about change that would be necessary or better to implement this (and if such a change is agreeable). Once/while implementation detail are discussed apps and parsers still need to be changed before this gets ready for merge (removing the possibility to read file from parsers and the implied changes it will need on apps).

This would replace #673 if merged before. I didn't look if it would be a pain to modify #674 to make it work with the changes brought here if it is merging #674 should take priority and this PR should be responsible to make it work (tbc i presume it means i will do it).

This PR has been tested by launching the game on an android emulator and windows (both x86_64).

src/FileSystem/AndroidFileSystem.cpp

bwrsandman · 2024-04-05T20:17:28Z

I'm not sure why the Files in components are virtual. They should not be.
Another thing I'm worried about is language binding. The original intent of splitting off file loading code was they they would be easier to call from C code and to make language bindings. We shouldn't lose sight of this.

FalseIlyu · 2024-04-06T09:36:26Z

Another thing I'm worried about is language binding. The original intent of splitting off file loading code was they they would be easier to call from C code and to make language bindings. We shouldn't lose sight of this.

This mostly concerns components, no ? Because in this case nothing really force us to remove the void Open method and use this in engine to make thing more straightforward for use (with the idea that with the actual file reading decoupled from the filesystem it is going to make it easier to test thing when we actually go forth with looking performance issue). Even in the case of the filesystem we can keep the readAll method accessible as something easier to work with from C (as for the moment we use it to build the stream in Android and i don't see it being not usefull to us in any case, it would just mean keeping it public).