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

filesystem: Move pre-compiler checks to interface #673

Closed
wants to merge 2 commits into from

Conversation

bwrsandman
Copy link
Member

@bwrsandman bwrsandman commented Dec 29, 2023

This allows for more interfaces to use custom read functions than android.
Such a usecase would be single ISO reads.

Copy link
Contributor

github-actions bot commented Jan 3, 2024

Uploaded images

Map Debug Release Vanilla
Land1.txt Land1 Debug Land1 Release Land1 Vanilla
Land2.txt Land2 Debug Land2 Release Land2 Vanilla
Land3.txt Land3 Debug Land3 Release Land3 Vanilla
Land4.txt Land4 Debug Land4 Release Land4 Vanilla
Land5.txt Land5 Debug Land5 Release Land5 Vanilla
LandT.txt LandT Debug LandT Release LandT Vanilla
TwoGods.txt TwoGods Debug TwoGods Release TwoGods Vanilla
ThreeGods.txt ThreeGods Debug ThreeGods Release ThreeGods Vanilla
FourGods.txt FourGods Debug FourGods Release FourGods Vanilla
construct.txt TODO TODO construct Vanilla

@bwrsandman bwrsandman enabled auto-merge (rebase) January 3, 2024 12:25
@FalseIlyu
Copy link
Contributor

FalseIlyu commented Jan 3, 2024

Tested on a windows computer :
Normally launching the game works without problem, i tried setting the default filesystem preferedBuffer to true and it seems to cause no issue too and everything seems right to me.

However i wonder if we could not have a way to forgo the check and change this

if (Locator::filesystem::value().PreferBuffer())
{
	lnd.Open(Locator::filesystem::value().ReadAll(path));
}
else
{
	lnd.Open(path);
}

Into and just hide the whether the file is read from a buffer or from the filesystem.

lnd.Open(path);

It seems like it should be feasible without too much hassle but if i'm wrong having the asset read from the iso should be the priority preference in expression be damned.

@bwrsandman
Copy link
Member Author

I had the same thought to reduce duplicate code but couldn't find a satisfying implementation.

The issue is that a static lib like lnd would not have details of custom filesystem read implementations if they took charge of reading.

And since I want components to be independent of each other, I don't want to have some monster reader common class.

@FalseIlyu
Copy link
Contributor

Wouldn't going the other way around and having things written as

lnd.Open(Locator::filesystem::value().ReadAll(path));

Maybe not exactly like this something of the effect of the filesystem is in charge of getting the data to the reader and then reader do its job.

@bwrsandman
Copy link
Member Author

So just always reading the whole file and using the buffer implementation.
I wonder if there are performance implications to consider such as the cost of allocation or read time.
That being said SSDs are fast nowadays.

It's worth considering and would be a non trivial change in the way we interact with our files.

@FalseIlyu
Copy link
Contributor

FalseIlyu commented Jan 3, 2024

void PackFile::Open(const std::filesystem::path& file)
{
	_filename = file;

	std::ifstream stream(_filename, std::ios::binary);

	if (!stream.is_open())
	{
		Fail("Could not open file.");
	}

	ReadFile(stream);
}

void PackFile::Open(const std::vector<uint8_t>& buffer)
{
	assert(!_isLoaded);

	imemstream stream(reinterpret_cast<const char*>(buffer.data()), buffer.size() * sizeof(buffer[0]));

	// File name set to "buffer" when file is load from a buffer
	// Impact code using L3DFile::GetFilename method
	_filename = std::filesystem::path("buffer");

	ReadFile(stream);
}

Seeing as the Open method of many file reader is mostly setting up a stream for ReadFile couldn't we just make ReadFile accessible to LND and charge FileSystem with the job of setting up the stream?
Something like

lnd.Open(Locator::filesystem::value().GetData(path));

Where GetData would return an istream, wouldn't something like 73c4fe6 i did here do ? (The android Filesystem compiles but is blindcoded so i do not guarantee it runs)

@bwrsandman bwrsandman requested review from a team and removed request for handsomematt and raffclar March 24, 2024 23:39
@github-actions github-actions bot added the merge-conflicts PR cannot be merged until it is rebased label Apr 3, 2024
@bwrsandman
Copy link
Member Author

Superseded by #706

@bwrsandman bwrsandman closed this Aug 8, 2024
auto-merge was automatically disabled August 8, 2024 21:00

Pull request was closed

@bwrsandman bwrsandman deleted the buffer-reads branch August 8, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflicts PR cannot be merged until it is rebased
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants