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

Improve handling of archives when installing assets #81358

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 5, 2023

Fixes #81311. Closes #75712.

So #75712 has been a problem for quite some time now. As discussed, there are two key points:

  • Installing ZIPs manually is treated differently than installing assets from the AssetLib (despite handling the same ZIPs internally);
  • All ZIPs are expected to have a root directory in them, because that's how GitHub, etc pack their ZIPs.

And #81311 is a new finding, as far as I know. It seems some ZIP archives can skip the directory structure information and just contain record of files without intermediate folders. We fail at handling those gracefully.


So I set to fix these problems, and along the way I tweaks smaller details here and there. Here's a list of changes:

  • Implicitly create folders when parsing ZIP files to make sure we have the entire structure available regardless of the source ZIP.
  • Add a checkbox that allows to skip the root directory when extracting files, if such directory exists.
  • Fix formatting of the "Files failed to extract" error message that was missing a line break.
  • Fix some errors not being displayed at all (also remove the unnecessary dialog and use toasts instead).
  • Improve the status bar to better highlight the title of the asset and the status on the conflicting files (also use TTRN for that last message).
  • Rename the window to explain what it's for.
  • Various refactoring for code clarity and slightly better organization (this is clearly a very old file, and it leaves a lot to be desired, so I kept the changes to rather minimal).
godot windows editor dev x86_64_2023-09-05_23-54-53 godot windows editor dev x86_64_2023-09-05_23-54-28 godot windows editor dev x86_64_2023-09-06_00-05-32

Most of it should be self explanatory. Regarding the checkbox, it tries to be smart. First of all, it's disabled if there is no single root folder in the asset archive. It only becomes available if your asset has a folder structure similar to what GitHub creates. This may backfire when your ZIP file contains, for example, only the addons folder — but if that becomes an issue we should be able to easily add an exception.

I've decided to preserve the current dual logic, where assets installed through the AssetLib have their root folder skipped, while archives installed manually don't behave this way. However, this is no longer a hardcoded behavior. Instead, the checkbox is either checked by default or not, but can still be changed by the user.

Here's everything in action (note that the image loading error is related to the broken asset and is not a part of this PR):

godot.windows.editor.dev.x86_64_2023-09-05_23-53-18.mp4

@Trey2k
Copy link

Trey2k commented Sep 6, 2023

Would it be possible to modify what info the editor retries from the asset library to verify if the repo host is custom or not? I think that could potentially be a cleaner approach as then we would know for certain how it should be handled in either case.

EDIT: This would not resolve issues with manual imports still, but it would help the checkbox be more accurate in assumptions for direct downloads.

@YuriSizov
Copy link
Contributor Author

A custom host doesn't mean the asset doesn't follow this structure. And a custom host can still be used to point to a GitHub archive.

Giving users control is, IMO, the best option.

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.

Looks fine to me from a very cursory code review, and the PR rationale sounds good.

Could likely use a more in depth review/testing by @KoBeWi or @Calinou.

@fire
Copy link
Member

fire commented Sep 7, 2023

@lyuma this was a big problem for our extensions and offers alternative for our problem of requiring the root to be project root and not directory addons root

@YuriSizov
Copy link
Contributor Author

this was a big problem for our extensions and offers alternative for our problem of requiring the root to be project root and not directory addons root

I'm not sure this PR would solve your issue. But after this little refactor I'm quite confident I can implement the "export to this folder" feature, so archives can be exported to places other than the root.

@IceflowRE
Copy link
Contributor

Tested regarding #81311, and it works now.

@YuriSizov YuriSizov force-pushed the assets-install-however-you-want branch from 86f5573 to 639aba4 Compare September 8, 2023 18:46
@YuriSizov
Copy link
Contributor Author

All concerns should be addressed, I think.

@akien-mga akien-mga merged commit 7343ad9 into godotengine:master Sep 8, 2023
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the assets-install-however-you-want branch September 9, 2023 01:01
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.

Addon is empty after downloading from asset lib Editor treats custom repository host zips differently.
6 participants