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 "Pack Project as ZIP..." to Project menu #99781

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

Meorge
Copy link
Contributor

@Meorge Meorge commented Nov 28, 2024

Closes godotengine/godot-proposals#227.

This PR adds a menu item, "Pack Project as ZIP...", to the Project menu in the menu bar. Clicking it opens a file save location dialog, which defaults to the same directory as the project's folder. Clicking "Save" packs the contents of the project, except for the .godot folder, into a ZIP file at that location.

Godot – Pack Project as ZIP Menu Item
Godot – Pack Project as ZIP Save Popup
Godot – Pack Project as ZIP File in Finder
Godot – Pack Project as ZIP Comparison of Folders

Most of the code for this is from the WebToolsEditorPlugin class, since it was already implementing most of this functionality. The relevant code has been pulled out of that class and into a new class with static methods, ProjectZIPPacker. Both WebToolsEditorPlugin and this new menu item simply use ProjectZIPPacker to accomplish their task.

Notes

  • I specifically used the term "Pack" here instead of "Export". I feel like "Export" implies that the output will be in a different, runnable format, whereas this is simply creating a copy of the project itself.
  • While the .godot folder is not present, other hidden files/directories like .git, .gitignore, etc are included.
    • We could potentially add checkboxes to the save dialog to decide whether to include certain types of hidden files like Git configuration/data.
  • "Date Modified" metadata is not preserved (you can see in the fourth screenshot that the "Date Modified" on all of the files is November 30, 1979 at 12:00 AM).
  • The behavior of "Download Project Source" for the web editor should be unchanged, although I haven't yet been able to test it. Assuming it works correctly, this will still mean that the web editor will have two menu items with different names that perform very similar tasks. If possible, "Pack Project as ZIP" should probably be modified so that when run in the web editor, it simply downloads to the user's device (replicating the behavior of the existing "Download Project Source"), and then we can remove the menu item for "Download Project Source".

To Do

  • Confirm that web export functionality ("Download Project Source") still works
  • Attempt to merge "Pack Project as ZIP" with web export functionality (look into using #defines for this perhaps?)

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

This improves usability and export by exposing something hidden in the export system.

Did not do a thorough review and did not do testing.

@akien-mga akien-mga requested review from akien-mga and a team November 28, 2024 15:13
@adamscott
Copy link
Member

Interesting. Especially when asking for MRPs.

@Meorge
Copy link
Contributor Author

Meorge commented Nov 28, 2024

I was hoping to test the web editor today, but unfortunately ran into some strange issues with running it. I've asked for assistance with it in some other places, and will give a shot at building from master just in case my PR introduced bugs, but otherwise I won't be able to test the web editor for now 😅

@badsectoracula
Copy link
Contributor

I tried it here and from what i can see in the code it looks fine to me, with three notes:

  1. It seems to just be copying everything, but perhaps the exp directory (or at least the generated files from configured exports) could be ignored. The project i tried to "pack" had a previously (and outdated) build i made a few days ago i had forgotten and yet it took 99% of the ZIP file's space. Perhaps there should be some files it should ignore (maybe also parsing .gitignore for globs to ignore?)
  2. I'm not very familiar with that part of the code but isn't ERR_FAIL_COND_MSG(f.is_null(), "Unable to create ZIP file.") a bit too "raw" for error reporting? I see other surrounding code use show_accept for that.
  3. PR is made up of multiple commits, before it can be merged it should be squashed with a rebase and force pushed to be a single commit, check https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

Aside from the above, i think it'd be useful functionality to have. I also tried to build the web-based editor but for some reason it wouldn't run (both Firefox Developer Edition and Falkon wouldn't work, stopping at godot's logo with an error about trying to call send_message from an undefined object, so something failed to load or initialize), so i couldn't test this for now.

@Meorge
Copy link
Contributor Author

Meorge commented Nov 29, 2024

Thanks for testing it!

To address your notes:

perhaps the exp directory (or at least the generated files from configured exports) could be ignored

If these are pre-defined/hard-coded directories that would exist in any project that's been exported, then I definitely agree they should be excluded by default. I was thinking of having it ignore Git-related files as an option as well. Parsing .gitignore itself to determine files to exclude would be clever too. I feel like that might be a bit out of scope for this PR - given that one of the big uses of it will be easily creating MRPs, I think it might be better to aim for the fine-grained exclusion options in a later PR.

I'm not very familiar with that part of the code but isn't ERR_FAIL_COND_MSG(f.is_null(), "Unable to create ZIP file.") a bit too "raw" for error reporting? I see other surrounding code use show_accept for that.

This was copied from the "Download Project Source" code, and I think I've also seen some other functions in Godot that just output their error messages to the console window. That being said, a show_accept (or similar nice GUI response) does sound better, and like it wouldn't increase the scope of the PR much, so I could look into adding that.

PR is made up of multiple commits, before it can be merged it should be squashed with a rebase and force pushed to be a single commit

Yep, I'm aware. I'm happy with the current overall state of the PR (that is, I don't foresee myself needing to check intermediate commits), so I'll probably squash what I have so far. I often prefer to keep the commits separate so I can check intermediate versions, then squash them once the PR is closer to being merged.

I also tried to build the web-based editor but for some reason it wouldn't run (both Firefox Developer Edition and Falkon wouldn't work, stopping at godot's logo with an error about trying to call send_message from an undefined object, so something failed to load or initialize), so i couldn't test this for now.

That may have been the same error message I was getting, that prevented me from testing the web editor! If I don't see an issue open for it, I'll open one.

Thanks again!

@Meorge Meorge force-pushed the pack-project-as-zip branch from ab02185 to fdf2937 Compare November 29, 2024 02:23
@Meorge Meorge force-pushed the pack-project-as-zip branch from 8e0c6b4 to 6c4c76a Compare November 30, 2024 03:23
@Meorge Meorge force-pushed the pack-project-as-zip branch from 6c4c76a to ab4e824 Compare November 30, 2024 20:46
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally with the Platformer 3D demo, it works as expected.

@Meorge
Copy link
Contributor Author

Meorge commented Dec 1, 2024

Thanks for the review! 😄

I'm hoping to check what the result of a project export is (i.e., if there are extra folders created that we may want to automatically exclude), but it looks like I may be running into a bug with loading export templates. I'm going to try rebasing on a recent commit from master and if it still persists, I'll file an issue for it.

Edit: Indeed, it seems I'm unable to add an export template - issue has been filed #99907

@Meorge
Copy link
Contributor Author

Meorge commented Dec 2, 2024

I was finally able to get export templates working, but in my case (doing a macOS testing export) it doesn't appear to have created any exp folders or files other than the intended .dmg file. As such, I'm guessing that each platform will have its own output artifacts, and maybe something like using a .gitignore for determining which files to ignore would make the most sense. Again, though, I think that may be best for a follow-up, since it greatly increases the complexity of the feature.

@Meorge Meorge force-pushed the pack-project-as-zip branch from a33b17c to d8d9c50 Compare December 2, 2024 15:10
@adamscott
Copy link
Member

For the Web platform, we will need to ask the OS first if they want to handle it first. Because we need the native file saver instead of the internal one, as saving the file internally doesn't make the file downloadable necessarily, which defeats the purpose.

And the PR should remove the existing "Pack project as ZIP" that exists for the Web platform (in Tools).

@Meorge
Copy link
Contributor Author

Meorge commented Dec 7, 2024

Those sound like good changes to make, to me. Unfortunately it looks like with the branch based off of the latest master and with #99963 temporarily merged in, the web editor is still broken (same Cannot read properties of undefined (reading 'msg_send_queue') error as before).

I saw on the RocketChat you'd reported this bug to them: emscripten-core/emscripten#23046 but it appears to only have been assigned so far, so I may need to wait for that to be fixed before I can make further progress on this PR...

@akien-mga
Copy link
Member

Those sound like good changes to make, to me. Unfortunately it looks like with the branch based off of the latest master and with #99963 temporarily merged in, the web editor is still broken (same Cannot read properties of undefined (reading 'msg_send_queue') error as before).

Is this still the case? I tested 4.4.dev7 on Linux/Firefox and it seems to run ok for me.

@Meorge
Copy link
Contributor Author

Meorge commented Jan 6, 2025

Those sound like good changes to make, to me. Unfortunately it looks like with the branch based off of the latest master and with #99963 temporarily merged in, the web editor is still broken (same Cannot read properties of undefined (reading 'msg_send_queue') error as before).

Is this still the case? I tested 4.4.dev7 on Linux/Firefox and it seems to run ok for me.

I just tried it again on 1aaf20b, and this time got the error:

Uncaught (in promise) LinkError: WebAssembly.instantiate(): Import #170 "env" "getaddrinfo": function import requires a callable
    at godot.editor.js:13875:17

This is running in Google Chrome Version 131.0.6778.205 (Official Build) (x86_64) on macOS Sequoia 15.2, where the web editor was compiled with Emscripten 3.1.64:

➜  godot git:(master) emcc -v                         
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.64 (a1fe3902bf73a3802eae0357d273d0e37ea79898)
clang version 19.0.0git (https:/github.com/llvm/llvm-project 4d8e42ea6a89c73f90941fd1b6e899912e31dd34)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /Users/malcolmanderson/emsdk/upstream/bin

The command used to compile was:

scons platform=web target=editor

@adamscott
Copy link
Member

I just tried it again on 1aaf20b, and this time got the error:

Uncaught (in promise) LinkError: WebAssembly.instantiate(): Import #170 "env" "getaddrinfo": function import requires a callable
    at godot.editor.js:13875:17

This is running in Google Chrome Version 131.0.6778.205 (Official Build) (x86_64) on macOS Sequoia 15.2

I'm having the same issue. I'm investigating.

Apply suggestions from code review

Co-authored-by: A Thousand Ships <[email protected]>

Fix includes

Update editor/editor_node.cpp

Co-authored-by: Hugo Locurcio <[email protected]>
@Meorge Meorge force-pushed the pack-project-as-zip branch from d8d9c50 to 6b33037 Compare January 10, 2025 04:37
@Meorge
Copy link
Contributor Author

Meorge commented Jan 10, 2025

Thanks for looking into it - I'm happy to hear, at least, that it isn't a weird issue isolated to my computer 😅

I rebased the branch and fixed merge conflicts. The now-redundant web ZIP export still needs to be removed, which we can do once we have the base web editor build working again.

I'll be out of town until the 14th, and I may not be able to do a lot of work on it until I return. I will certainly try my best to do whatever I can, though!

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Jan 11, 2025
@akien-mga
Copy link
Member

Let's merge this for now, and solve the redundant option for the Web editor once we actually get the Web editor to run again.

@Meorge
Copy link
Contributor Author

Meorge commented Jan 11, 2025

Sounds good, thanks!

@akien-mga akien-mga merged commit f431419 into godotengine:master Jan 11, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Meorge Meorge deleted the pack-project-as-zip branch February 3, 2025 17:28
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.

Expose "Download Project Source" option on platforms other than HTML5
9 participants