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

Make the project data directory customizable #52548

Merged
merged 1 commit into from
Oct 12, 2021

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Sep 10, 2021

This PR makes the default project data directory (.godot) customizable.

This is useful on platforms where certain directory patterns are disallowed (e.g: some Android build tools disallow hidden directories in the generated APK).

@@ -121,6 +121,7 @@ public Client(string identity, string godotProjectDir, IMessageHandler messageHa
this.messageHandler = messageHandler;
this.logger = logger;

// TODO: Need to fetch the project data dir name from ProjectSettings instead of defaulting to ".godot"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@godotengine/mono What would be the proper way to fetch the project data directory name from the ProjectSettings and pass it here instead of the default value (.godot)?

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

[member application/config/project_data_dir_name][code]/imported/[/code]

That is basically unreadable across all the docs changes in this PR. It should be rephrased everywhere.

I think it's better to leave the original phrasing referring to .godot and only mention the configuration property in parenthesis, like (see [member application/config/project_data_dir_name]).

@YuriSizov
Copy link
Contributor

This is useful on platforms where certain directory patterns are disallowed (e.g: some Android build tools disallow hidden directories in the generated APK).

I'm not sure that this a good reason to make the path customizable. We could probably rename the (virtual) directory for the exported project on such platforms during the export.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

This is useful on platforms where certain directory patterns are disallowed (e.g: some Android build tools disallow hidden directories in the generated APK).

I'm not sure that this a good reason to make the path customizable. We could probably rename the (virtual) directory for the exported project on such platforms during the export.

That sounds more complex than this approach. As you can see from the changed files, the project data directory is referred in multiple locations throughout the codebase.
What you're suggesting seems to imply we'd need to coordinate platform specific logic with actual references to the project data directory in the codebase. As the codebase grow, this would only get more troublesome.

@pycbouh Trying to understand your feedback, what's the issue with the current approach?

@YuriSizov
Copy link
Contributor

what's the issue with the current approach?

It just creates an opportunity for everyone to shoot their foot because some platforms have limitations. I feel like that solution has greater effect than the problem it tries to solve, i.e. it's too wide while the issue is very specific. That folder is used for all kinds of meta information, so allowing users to freely configure it can be dangerous.

@m4gr3d m4gr3d force-pushed the customize_metadata_dir_master branch from 472c924 to 1153aa3 Compare September 10, 2021 19:24
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

[member application/config/project_data_dir_name][code]/imported/[/code]

That is basically unreadable across all the docs changes in this PR. It should be rephrased everywhere.

I think it's better to leave the original phrasing referring to .godot and only mention the configuration property in parenthesis, like (see [member application/config/project_data_dir_name]).

Done.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

what's the issue with the current approach?

It just creates an opportunity for everyone to shoot their foot because some platforms have limitations. I feel like that solution has greater effect than the problem it tries to solve, i.e. it's too wide while the issue is very specific. That folder is used for all kinds of meta information, so allowing users to freely configure it can be dangerous.

I guess I disagree in that regard.

  1. For one, the settings is part of the Advanced Settings, so it's not visible to the user until they actively go looking for it.
  2. Second, most of the paths used by the engine are already configurable (or at least platform specific), so here I'd argue we're being consistent by making the project data directory configurable as well.
  3. The content in the project data directory is automatically generated by the engine. When that directory is changed and the editor restarted, the content again is automatically re-generated, so I don't see the risk in modifying that setting.

The alternative you proposed seems more complex in my opinion, but if you have a proposal (or PR) to show that it isn't, I'm open to take a look.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 10, 2021

The content in the project data directory is automatically generated by the engine. When that directory is changed and the editor restarted, the content again is automatically re-generated, so I don't see the risk in modifying that setting.

It's not just for import.

Second, most of the paths used by the engine are already configurable (or at least platform specific), so here I'd argue we're being consistent by making the project data directory configurable as well.

.godot is a folder for the editor, not a project folder. It's used for local editor information, like in any given IDE. Imported resources were added to it for better organization and uniformity.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

The content in the project data directory is automatically generated by the engine. When that directory is changed and the editor restarted, the content again is automatically re-generated, so I don't see the risk in modifying that setting.

It's not just for import.

I know. I've taken a look at the code behind it, and tested the behavior, and it behaves as expected when the directory is updated.

In addition, from 3.x to 4.x, that directory was updated from .import to .godot (plus lots of additional changes). Having it configurable in the first place would have made that process easier. Case in point, the update was missed in the platform/javascript/api/javascript_tools_editor_plugin.cpp file (updated in this PR), which would have certainly caused some subtle bugs.

@YuriSizov
Copy link
Contributor

Having it configurable in the first place would have made that process easier. Case in point, the update was missed in the platform/javascript/api/javascript_tools_editor_plugin.cpp file (updated in this PR), which would have certainly caused some subtle bugs.

I'm not against removing hardcoded values from code. I'm just not in favor of making it user-configurable. It's internal stuff, used for various editor data for the project. If we have problems with specific platforms, we should solve those problems behind the scenes, and making users fix it for us by changing internal parameters of the editor behavior is a wrong approach.

If the current folder structure breaks some exported projects, we can just find a solution that works everywhere. There is no concrete reason for the imported folder to be specifically .godot/imported in exported projects. It can be anything. It's not a real folder anyway.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

Having it configurable in the first place would have made that process easier. Case in point, the update was missed in the platform/javascript/api/javascript_tools_editor_plugin.cpp file (updated in this PR), which would have certainly caused some subtle bugs.

I'm not against removing hardcoded values from code. I'm just not in favor of making it user-configurable. It's internal stuff, used for various editor data for the project. If we have problems with specific platforms, we should solve those problems behind the scenes, and making users fix it for us by changing internal parameters of the editor behavior is a wrong approach.

This is assuming that all Godot users (the vast majority is indeed) are using the editor, and that we are aware of all the platforms and their quirks.
This feature addresses a real observed need in a production environment that use and integrate the Godot engine, but not the editor. As such, if there is another option for updating that directory (without having to rebuild the engine) that's still accessible to advanced users, I'm all ears.

If the current folder structure breaks some exported projects, we can just find a solution that works everywhere. There is no concrete reason for the imported folder to be specifically .godot/imported in exported projects. It can be anything. It's not a real folder anyway.

If there's a proposal on the table that addresses the points I have made, I'm open to see it.
I've already discussed this feature with @reduz and the approach of making the project data directory configurable via ProjectSettings was the one suggested.

@YuriSizov
Copy link
Contributor

If there's a proposal on the table that addresses the points I have made, I'm open to see it.

I obviously don't have a proposal for something that I've only just learnt is a problem. Either way, I've made my case, and since it's pre-approved by reduz, I don't see any point deliberating further.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 10, 2021

If there's a proposal on the table that addresses the points I have made, I'm open to see it.

I obviously don't have a proposal for something that I've only just learnt is a problem. Either way, I've made my case, and since it's pre-approved by reduz, I don't see any point deliberating further.

@pycbouh I appreciate the feedback, it's just hard to argue in a vacuum (e.g: without a counter-proposal).
So apologies if it sounds like I'm dismissing your feedback, that's not the case.

@akien-mga
Copy link
Member

akien-mga commented Sep 15, 2021

This is useful on platforms where certain directory patterns are disallowed (e.g: some Android build tools disallow hidden directories in the generated APK).

Can we know more about what tools exactly have an issue with the current pattern?

I'm not against the solution, but we need to know why we change it specifically.

If it's for some specific tools but actually not a requirement of the platform itself, maybe those tools are wrong and should be fixed to follow the same specification as the platform they target. We've used such hidden paths since 3.0 and it was not a known problem prior to this PR.

If it's the platform itself, instead of making it configurable by the user (which means they need to be aware of platform quirks and handle them manually in each project), we should maybe make this a platform-specific hardcoded path (like EditorPaths path) so that the config is always appropriate for each platform.

I.e. most of the code in this PR would be reused, but instead of making it a user decision to change that path, it should be an engine decision based on knowing platform requirements and handling them for the user. It does imply having to rename/copy the folder with a different name on export to the affected platform(s), and might be tricky since the *.import have a reference to their imported filepath (though it can also be fixed on export).

I'm not strictly against making this user configurable, but it does create potential issues:

  • Plugins that assume the presence of this folder will break. Now they can query ProjectSettings to get the path and that's good to have (even if it were kept hardcoded/constant), but old plugins may break in ways that the user may not be aware of.
  • This complicates documentation, as seen already here for the few mentions in the class reference, but there's likely a lot more of those in the online docs and in various Q&A answers, GitHub issues, etc. We'll now have to handle the case of "res://.godot or the custom path you configured in ...".

So all in all, I really need to understand the exact problem before approving a solution which does IMO make things more complex.

Edit: Apart from this, the implementation in this PR seems good to me from a technical standpoint.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

This is useful on platforms where certain directory patterns are disallowed (e.g: some Android build tools disallow hidden directories in the generated APK).

Can we know more about what tools exactly have an issue with the current pattern?

I'm not against the solution, but we need to know why we change it specifically.

If it's for some specific tools but actually not a requirement of the platform itself, maybe those tools are wrong and should be fixed to follow the same specification as the platform they target. We've used such hidden paths since 3.0 and it was not a known problem prior to this PR.

Build tools for the Android platform by default strip hidden directory out of the apk's assets directory.
We've been fortunate enough that both ant and gradle allow escape hatch out of that behavior (see #39049), but that's not the case for all Android build tools (e.g: bazel, buck), so users not using the default build tool very quickly run into this breaking limitation (the app can't load or run since the project data directory is missing).
It wasn't a problem before because the previous approach (legacy build) to generating Android apks was to just push all of the compiled project data directly into the assets directory and zip the apk back up. Since that build system is being phased out (e.g: lack of AAB support, etc...), we're losing this option as well.

If it's the platform itself, instead of making it configurable by the user (which means they need to be aware of platform quirks and handle them manually in each project), we should maybe make this a platform-specific hardcoded path (like EditorPaths path) so that the config is always appropriate for each platform.

I.e. most of the code in this PR would be reused, but instead of making it a user decision to change that path, it should be an engine decision based on knowing platform requirements and handling them for the user. It does imply having to rename/copy the folder with a different name on export to the affected platform(s), and might be tricky since the *.import have a reference to their imported filepath (though it can also be fixed on export).

I'm not strictly against making this user configurable, but it does create potential issues:

  • Plugins that assume the presence of this folder will break. Now they can query ProjectSettings to get the path and that's good to have (even if it were kept hardcoded/constant), but old plugins may break in ways that the user may not be aware of.

The current approach indeed ensures that it's consistent for a project since the user is likely to edit on a desktop platform, then run the project on another platform. The option you proposed of making it platform specific would lead to a similar set of changes, but in addition would make the platform export logic fairly complex and prompt to bugs (e.g: any update to the project data dir structure would need to mirrored in the corresponding platform).

In addition, it would not solve the issue you highlighted above as the plugin will still break when running on the platform with the modified path.

I would also assume that projects with old plugins would not see a need to modify the default since they are and have been already working well with it.
For the 3.x branch, we could add a note in the documentation warning that old plugin may break if that setting is modified.

  • This complicates documentation, as seen already here for the few mentions in the class reference, but there's likely a lot more of those in the online docs and in various Q&A answers, GitHub issues, etc. We'll now have to handle the case of "res://.godot or the custom path you configured in ...".

That's a valid point for the 3.x branch. With so much changing for 4.x, I would expect this not to be an issue and I would expect the documentation for this setting to be similar to the other platform specific directories that we have (e.g: user data directory).

So all in all, I really need to understand the exact problem before approving a solution which does IMO make things more complex.

Edit: Apart from this, the implementation in this PR seems good to me from a technical standpoint.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

I think it would be better to just make the path res://godot on android or something like that rather than making it customizable. The path could still be provided via ProjectSetting.
Making it customizable will cause problems for external tools which need to access this folder. They would now need to read project.godot, which is a non-standard format only Godot parses, in order to get the location.

The issue with that approach is you can't make it platform specific as it significantly increases code complexity (which could lead to subtle bugs creeping in), and introduce an additional (lengthy step for large projects) where all the .import file have to be rewritten for the platform to match that platform specific location.

@YuriSizov
Copy link
Contributor

YuriSizov commented Sep 15, 2021

The root of the problem is that we have a metadata folder (.godot) which is also used for essential project data, such as imported resources. I wouldn't want us to unhide the metadata folder because that would be an unconventional solution across other development tools and their metadata folders. Even the imported folder itself should be considered hidden in development environment, as it does not contain any data that should be tracked or manually edited.

But if we do not want to introduce platform-specific complications, I would suggest we rename the folder but not in the working directory, but in the exported data for all the platforms. So working .godot/imported (or .import in 3.x) becomes exported godot_imported. We can blacklist the name so that users don't use it accidentally for their folder, and we can document that properly as well. But I think it's the best bet to keep the compatibility and follow the metadata folder convention while solving this platform-specific problem.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

I wouldn't want us to unhide the metadata folder because that would be an unconventional solution across other development tools and their metadata folders.

@pycbouh Can you clarify what you mean by unhiding the folder?
Because we can easily hide the directory within the Godot editor with .gdignore file, or even at the source code level.
And on platforms like Windows, . directories are not considered hidden.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

So working .godot/imported (or .import in 3.x) becomes exported godot_imported

My issue with this approach is how much it would increase export time for large projects as all the .import file would need to be rewritten at export time.

@YuriSizov
Copy link
Contributor

And on platforms like Windows, . directories are not considered hidden.

Even on Windows tools consider them hidden if they are prefixed with a period. It's a universal convention used by virtually all development tools I've used, and I'm only working from Windows. It's also a signal to developers themselves not to touch those files manually. IDEs, VCSs all use that pattern.

My issue with this approach is how much it would increase export time for large projects as all the .import file would need to be rewritten at export time.

Export time is hardly an important metric, unless it increases several times over. But we don't even need to edit the files themselves, if we can help it. It's important that the folder is not hidden, not what is written in files. We can dynamically adjust res:// paths to understand .godot/imported as godot_imported. Or we can change those res:// paths to import:// paths which would auto-resolve to whatever they need to resolve.

@akien-mga
Copy link
Member

akien-mga commented Sep 15, 2021

To clarify, the res://.godot folder contains (so far):

  • res://.godot/editor/ which are editor metadata and should a priori not be relevant for exports.
  • res://.godot/imported/ files, which are referenced by path in their matching *.import files (e.g. path="res://.godot/imported/icon.png-487276ed1e3a0c39cad0279d744ee560.stex"). To remap those, we'd have to either:
    • Change to an import:// prefix that can be remapped at runtime.
    • Do search and replace on *.import files when copying during the export (it's all text files so it shouldn't too long, but still annoying to have to do such path wrangling).
    • Do runtime search and replace on all path queries, which might have a significant perf cost depending on how it's implemented.
  • res://.godot/shader_cache/, to be checked but this might need to be present in the export so it would also need to be remapped. Thankfully the engine is already set for this as there's a Engine::get_singleton()->set_shader_cache_path() method to set this path at runtime.
  • res://.godot/uid_cache.bin, this file should also be in the export. It's currently tracked via a constant (ResourceUID::CACHE_FILE), so that could also be made a getter if we want it to be different in Android exports.
    • To be checked, does the uid_cache.bin include hardcoded paths to res://.godot/imported/?
  • One of the main points of renaming res://.import to res://.godot is so that it can hold more than just imported files. So there might be more there in the future, and in particular, it's a path where thirdparty content creation plugins might place stuff too if they want. Those would be hard to remap on export.

So export-time remapping might be doable, but it would need a proof of concept to validate if it's really a better approach than what is proposed here. Both have significant cons.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

@akien-mga In the meantime, since we agree that providing a getter is a good option, i split that logic into a separate PR: #52711

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Sep 15, 2021

@akien-mga In the meantime, since we agree that providing a getter is a good option, i split that logic into a separate PR: #52711

3.x version of that PR.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

After discussion, given the scope of the changes and the fact that documentation can make it clear that this is reserved for advanced use cases, I'm fine with the change (as it's needed by some users and there's no possible workaround aside from a hard fork).

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 11, 2021

@akien-mga I've addressed the feedback, and tested the changes, all good to go!

@akien-mga akien-mga merged commit 1bd6a2f into godotengine:master Oct 12, 2021
@akien-mga
Copy link
Member

Thanks!

@Listwon
Copy link
Contributor

Listwon commented Oct 12, 2021

FYI We've discussed some issues in 3.x version of this PR #52556

In short:

  • you can set the path to empty string (I'm not sure about all of the consequences, but I don't think it should be allowed)
  • setting it to an empty string causes begins_with() to ignore all of the folders in project path (I've created Test folder and it wasn't shown in the FileSystem, moving files to folder makes them invisible to the editor) and import assets directly in the root path
  • it will be the same if you set the path to imported and then name other folders imported_models or imported_from_psd etc. all of the folders starting with imported will be ignored by editor

I don't think it was a good decision to replace == with begins_with() as it's not needed (no performance gain over equal), but introduces bugs as it's too broad in this case.

@m4gr3d m4gr3d deleted the customize_metadata_dir_master branch October 12, 2021 19:00
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 12, 2021

@Listwon Thanks for the feedback! I'll work on a another PR to address them.

I don't think it was a good decision to replace == with begins_with() as it's not needed (no performance gain over equal), but introduces bugs as it's too broad in this case.

This was done to handle scenarios where the path would have a trailing / (I ran into such a bug while testing since == would return false in that case).

There are probably other approaches to solve this that don't introduce the issues you observed with begins_with().
Let me know If you or @akien-mga have any feedback on other approaches to try.

@YuriSizov
Copy link
Contributor

User supplied paths should be sanitized to either include or exclude the trailing slash for every input, depending on how the value is used. begins_with() is indeed not required here, we should only look for exact match, and it's the input that must be validated better instead.

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.

5 participants