-
-
Notifications
You must be signed in to change notification settings - Fork 20.9k
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
Improve PCK loading filename handling #59527
base: master
Are you sure you want to change the base?
Conversation
I don't like this, it adds unnecessary flexibility and makes it harder to actually detect Godot games for something like SteamDB. I don't see a good rationale for adding this flexibility. We should not encourage filenames with multiple "extensions", that's confusing. |
@akien-mga If users want to ship multiple executables for different architectures, how do you recommend this be done? On Linux there can be The flexibility is optional, we don't have to encourage its use or even document it. If all someone does is use the same names as in the current master and in 3.x, like Aside from being a separate method, the complexity of this code is arguably simpler, since it doesn't have to repeat |
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.
I like the idea of having multiple executables and a single PCK, but not so sure about implementation. Maybe it's better to have some set of pre-defined patterns for the architecture suffix (I would prefer to use something more readable, like "Gama Name (x86_64).exe" instead of multiple extensions).
Or ever do it in opposite direction, something like:
- get the list of PCK in the executable folder first.
- use
exec_filename.begins_with(pck_file.get_file())
to determine which to load.
core/config/project_settings.cpp
Outdated
#ifdef OSX_ENABLED | ||
// Attempt to load PCK from macOS .app bundle resources. | ||
if (_load_resource_pack(OS::get_singleton()->get_bundle_resource_dir().plus_file(exec_filename + ".pck"))) { | ||
return true; | ||
} | ||
#endif |
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.
A bit pointless for macOS, since the executable is inside the .app bundle and its name is not visible to the user, and a single executable can contain any number of architectures, but I guess it's OK for consistency.
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.
The implementation in this PR has the goal of being as simple as possible while allowing these extra use cases. Indeed this will still work fine for macOS. In fact, if the executable is extensionless, this should end up loading on the first try, skipping all of the other logic that would be pointless.
Plus this adds flexibility in the event that someone wanted to not use .app bundles. Even if nobody uses that... it doesn't matter, we don't need to use every possibility of a feature to have a flexible solution be worth it.
I'm not sure what the point of that would be. This suggestion seems to add complexity for no benefit. We shouldn't need to look at any other files in the folder. In my opinion, if we want to have the architecture somewhere in the executable name, we should just have it dot separated like is already the case with file extensions on Linux exports. Or, users could just not have the architecture in the name, and then it would work fine for existing uses. |
6b515ed
to
20f495d
Compare
20f495d
to
d0997f1
Compare
d0997f1
to
ee6d0d2
Compare
ee6d0d2
to
e874189
Compare
e874189
to
0ca6916
Compare
0ca6916
to
e19c653
Compare
e19c653
to
d7637e7
Compare
d7637e7
to
9bc6c89
Compare
9bc6c89
to
722c5f7
Compare
722c5f7
to
a91d28e
Compare
a91d28e
to
4e25a04
Compare
4e25a04
to
65b0b04
Compare
65b0b04
to
2e7f9e3
Compare
2e7f9e3
to
e098b5f
Compare
e098b5f
to
437cc32
Compare
437cc32
to
4d2a19b
Compare
4d2a19b
to
7557309
Compare
7557309
to
e2cde26
Compare
27bdab6
to
fe521ed
Compare
fe521ed
to
726196a
Compare
726196a
to
312790e
Compare
Yeah, this would be nice.
Being able to have more options to have Godot runtime find the .pck file would be really useful... |
312790e
to
f608b4e
Compare
This PR changes the code for loading a PCK file, specifically how it finds the
.pck
file. Discussed on RocketChat.The existing behavior of having
mygame.exe
andmygame.pck
still works fine. The code in this PR is actually simpler while still having the functionality of the current master and more, although there are more lines of code because I abstracted it into a separate method and added more comments.The code continues stripping the file name of extensions until it finds a PCK, or until there are no more extensions to strip. For example, let's say our executable is "mygame.x86_64.exe". First we look for "mygame.x86_64.exe.pck". If that fails, we look for "mygame.x86_64.pck". If that fails, we look for "mygame.pck". This allows having multiple executables for a game that share a specific PCK file, or providing different PCK files for different architectures or other custom situations. This PR also moves this logic into its own method.
Aside from allowing stripping more extensions, this PR does change the order of how things are searched, which changes behavior, but only in a minor way, and in my opinion the new behavior is much better. It will now search for the fullest names first in all locations before trying a shorter name. For example, let's say we have an executable named
mygame.exe
, in the current master it will search for.pck
files in this order:In this PR, the order will instead look like this:
I have already done some testing, and it works exactly as expected.