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

Add PCKReader class #81372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add PCKReader class #81372

wants to merge 1 commit into from

Conversation

yvie-k
Copy link
Contributor

@yvie-k yvie-k commented Sep 6, 2023

Adds a PCKReader class to read the contents of pck files without loading them.

This class exposes the same interface as the ZipReader class. Therefore we now have the same interface as ZIPPacker/ZIPReader for dealing with pck files.

To do so, the PCK implementation of PackedSourcePCK has been extracted into a new PCKFile class. I also split the loading of normal pck files and the loading of embedded pck files. This seemed clearer to me. If you have any issues regarding this, feel free to tell me.

Something to note would be that you cannot load the pck file embedded in the binary, as the PCKReader does not try to open the embedded file. But I'm not sure whether this should be supported as it feels quite hacky.

Furthermore, the current implementation loads the file list to memory when opening. This way we do not need to parse the file list on each access.

Resolves godotengine/godot-proposals#1212

core/io/pck_reader.h Outdated Show resolved Hide resolved
core/io/pck_reader.h Outdated Show resolved Hide resolved
core/io/pck_reader.h Outdated Show resolved Hide resolved
core/io/file_access_pack.cpp Outdated Show resolved Hide resolved
core/io/file_access_pack.cpp Outdated Show resolved Hide resolved
core/io/file_access_pack.h Show resolved Hide resolved
core/io/file_access_pack.h Outdated Show resolved Hide resolved
core/io/file_access_pack.cpp Outdated Show resolved Hide resolved
@yvie-k yvie-k force-pushed the pckreader branch 3 times, most recently from 26ee22d to 87a141b Compare September 6, 2023 13:43
@yvie-k
Copy link
Contributor Author

yvie-k commented Sep 6, 2023

On a side note, I also rewrote the embedded loading of pck files here: yvie-k@a5d5ec8

This would only try to load the embedded pck of the executable and not every opened pack file

I didn't include it as it is a) out of scope for this PR and b) I'm not sure if this is actually wanted

@@ -116,6 +134,25 @@ TEST_CASE("[PCKPacker] Pack a PCK file with some files and directories") {
CHECK_MESSAGE(
f->get_length() <= 27000,
"The generated non-empty PCK file shouldn't be too large.");

// Now check the contents read with PCKReader
Copy link
Contributor

Choose a reason for hiding this comment

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

Full sentences should use punctuation.

tests/core/io/test_pck_packer.h Outdated Show resolved Hide resolved
core/io/file_access_pack.h Outdated Show resolved Hide resolved
@yvie-k
Copy link
Contributor Author

yvie-k commented Sep 6, 2023

I can't manage to build the crashing CI release locally.

The compiler error mesage is unknown pseudo-op: .siz

Does anybody know what could be causing this?

There's also a warning: end of file not at end of a line; newline inserted

Anyways I'll be going to bed now. I will try to continue debugging the crash tomorrow.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

I didn't include it as it is a) out of scope for this PR and b) I'm not sure if this is actually wanted

Unlikely this is usefull for non-executable, and if somone realy want's it it's always possible to load it using offset.

CI crash is not related to your PR, we have it for some time but so far were not able to pinpoint the issue. So if it's happen, it's safe to restart the failed CI jobs.

}

void PCKReader::_bind_methods() {
ClassDB::bind_method(D_METHOD("open", "path", "offset"), &PCKReader::open);
Copy link
Member

Choose a reason for hiding this comment

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

Since PCK reader is user accessible, it might be worth adding the encryption key to the open arguments (and fall back to the embedded key if it's empty).

core/io/file_access_pack.cpp Outdated Show resolved Hide resolved
core/io/pck_reader.cpp Outdated Show resolved Hide resolved
core/io/pck_reader.h Outdated Show resolved Hide resolved
tests/core/io/test_pck_packer.h Outdated Show resolved Hide resolved
@yvie-k yvie-k force-pushed the pckreader branch 2 times, most recently from 21f8f8e to a8a6a0a Compare September 7, 2023 09:43
core/io/pck_reader.h Outdated Show resolved Hide resolved
core/io/pck_reader.h Outdated Show resolved Hide resolved
core/io/pck_reader.h Outdated Show resolved Hide resolved
@yvie-k yvie-k force-pushed the pckreader branch 2 times, most recently from 66a3826 to f0fed34 Compare September 7, 2023 10:00
@ghmart
Copy link

ghmart commented Sep 22, 2023

Hi, Dragoncraft89!

Built your fork and discovered a bunch of problems with it:

1. When saving scene it didn't give it default filename like "main.tscn" if root node called "Main", for example, just empty line (irrelevant to this PR, but still strange). Known bug, already fixed.

2. When exporting PCK it makes it almost 2x size bigger than official version of Godot (or custom builds from master). For example, official version makes PCK of size 4,877,872 against 8,372,048 using this fork (same settings). Sorry, my bad, forgot to exclude several resources.

  1. When exporting PCK it didn't see many folders where it could be stored, as if they're absent in filesystem.

  2. PCKReader didn't see some files (only text files mostly), for example *.png.
    EDIT: Is it possible to load one of the images using PCKReader without loading whole PCK, and, if yes, how?
    EDIT2: Found a way to get image from PCK: use "Keep file (no import)" import option for image in exported PCK, then use appropriate function such as "load_png_from_buffer" to load it after reading.

  3. PCKReader.open() function don't give "offset" parameter default value which should be 0, imo.

My system is Debian 11 (bullseye) if that helps. Tried with original PCK and exported using this fork.

@AThousandShips
Copy link
Member

When saving scene it didn't give it default filename like "main.tscn" if root node called "Main", for example, just empty line (irrelevant to this PR, but still strange).

That is a known and solved bug, rebase your branch and test again:

tests/core/io/test_pck_packer.h Outdated Show resolved Hide resolved
core/io/file_access_pack.cpp Outdated Show resolved Hide resolved
core/io/pck_reader.cpp Outdated Show resolved Hide resolved
tests/core/io/test_pck_packer.h Outdated Show resolved Hide resolved
@yvie-k
Copy link
Contributor Author

yvie-k commented Sep 22, 2023

@ghmart Thanks for testing!

Can you share a PCK file that does not work for you so that I can take a look? I'm not sure how I'd recreate your issue

Regarding 4: I think godot does rename some files on export, so the PCKReader will only see the renamed filenames. Furthermore, text files are not packed into the pck by default. You may have to check your export settings whether txt files are included.

Unfortunately I'm quite busy at the moment, so it'll take a couple of days for me to take a look at this.

@ghmart
Copy link

ghmart commented Sep 23, 2023

Can you share a PCK file that does not work for you so that I can take a look? I'm not sure how I'd recreate your issue

"3." - not about problem inside PCK. That's when you press "Export PCK/ZIP" and "Save a File" dialog opens up - it shows only several folders (random at a glance), not all that exists at that path or when you choose other path in filesystem (file / folder permissions are all identical).

Regarding 4: I think godot does rename some files on export, so the PCKReader will only see the renamed filenames. Furthermore, text files are not packed into the pck by default. You may have to check your export settings whether txt files are included.

You were right, thank you. Resources are stored in PCK in their import form. "Text files" - all *.tscn, *.gd and *.tres files can be loaded using "get_string_from_utf8". Can't figure out how to make "Image" or "ImageTexture" object from *.ctex or *s3tc.ctex file loaded into PackedByteArray by PCKReader.

@yvie-k
Copy link
Contributor Author

yvie-k commented Sep 23, 2023

@ghmart I don't think 3 is due to my patch. As far as I know this is how Godot export always looked like. I think you have to disable the file filter on the export dialogue to see all files

@yvie-k
Copy link
Contributor Author

yvie-k commented Sep 25, 2023

Updated the commit with the suggestion from @ghmart (setting default offset for the open function to 0)

Rebased onto latest master, tested and still working.

@ghmart I think I see what the issue with the file dialogue is. It treats directories the same as files for me.

I couldn't find an open issue for that (or a mention somewhere that this was deliberate). So I will do a git bisect and open an issue for this.

@ObaniGemini
Copy link

Any news on this?

@akien-mga akien-mga requested a review from bruvzg April 19, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method to list the contents of PCK files or resource associated with it
7 participants