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

Fix command line --editor --quit not properly making .godot/ import folder. #68461

Conversation

RevoluPowered
Copy link
Contributor

@RevoluPowered RevoluPowered commented Nov 9, 2022

The problem we had which this fixes:

We wanted to generate the ./.godot/ for a large game.

  • The CICD would run ./godot.editor.binary --editor --headless --quit

This command would then run and quit on first iteration, this is wrong for the context as the scanning is still running in the background. Meaning you'd have a half completed .godot/ import folder.

This means you can now do this, by simply waiting until its ready to exit.

@RevoluPowered RevoluPowered requested a review from a team as a code owner November 9, 2022 20:12
@RevoluPowered RevoluPowered force-pushed the fix-editor-import-exiting branch 2 times, most recently from 1337699 to c628ea2 Compare November 9, 2022 20:29
@Chaosus Chaosus added this to the 4.0 milestone Nov 9, 2022
@RevoluPowered RevoluPowered force-pushed the fix-editor-import-exiting branch 2 times, most recently from d6da29c to b2cd4dc Compare November 10, 2022 12:09
@akien-mga
Copy link
Member

Looks good, but I wonder if this would break some workflows where users actually want to quit ASAP regardless of import status.

An alternative could be to implement an actual --import option that would trigger the imports in headless mode without even starting the editor UI. This would also be more discoverable than having to figure out that --editor --headless --quit can be used for this.

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Nov 17, 2022

Looks good, but I wonder if this would break some workflows where users actually want to quit ASAP regardless of import status.

An alternative could be to implement an actual --import option that would trigger the imports in headless mode without even starting the editor UI. This would also be more discoverable than having to figure out that --editor --headless --quit can be used for this.

Agreed, I think @reduz should take a look actually too just to see if this is 100% correct too (my approach in the PR)

@Calinou
Copy link
Member

Calinou commented Nov 17, 2022

I'd also prefer having an explicit --import option, as proposed in godotengine/godot-proposals#1362.

@justinwash
Copy link
Contributor

justinwash commented Nov 27, 2022

Just wanted to chime in here with my really stupid workaround for this problem:

monitor-import.sh:

#!/bin/sh

while [ "$(printf "%.0f\n" $(top -n 1 -b | awk '/^%Cpu/{print $2}'))" -gt 10 ];
do
echo "$(top -n 1 -b | awk '/^%Cpu/{print $2}')";
done
echo "Finished importing. Killing godot editor process";
kill -9 $(pgrep -o godot)

Then in our dockerfile:

...usual setup...

RUN godot ./project.godot --editor --headless & sleep 5 && ./monitor-import.sh

...continue on to exporting the .pck...

Basically we open the project in the editor, wait a few seconds, then monitor CPU usage until it falls below 10%, at which point we can be fairly certain importing has finished so we kill the process and then build as normal.

Like I said, it's stupid, but it's working for us so far. Would love for this to be unnecessary.

@RevoluPowered
Copy link
Contributor Author

I'll remake with the --import option tomorrow ™️

@RevoluPowered
Copy link
Contributor Author

There IS still a texture packing issue that I haven't been able to fix with --headless

@aaronfranke
Copy link
Member

aaronfranke commented Jan 4, 2023

We tried merging this downstream on our fork of Godot, and it broke things. We likely need a better approach here. The proposed --import option would be excellent.

LinuxUserGD pushed a commit to LinuxUserGD/gdscript-transpiler-bin that referenced this pull request Jan 17, 2023
@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Jan 18, 2023

Yeah, we need to fix another two issues and open proper issues with MRP:

  • opening scene files from command line fails if the CICD is running this (even if imported) - I made several patches for this but to no avail.
  • some textures don't pack properly in exported binaries when using headless to import things. .godot/ folder appeared fully generated, even when running it many times. Some UI textures work some don't. Seemed to be random which textures were broken.

To bypass the second issue we are using a GPU runner to allow us to run the game without headless. The first issue didn't have a workaround.

@BjoernAkAManf
Copy link

Only marginally related to the PR (as i've only tested on Beta14), but i noticed that fonts especially pose an issue when importing.
For whatever reason the TTF files are not importing at all, causing their dependent files (in my case a Theme files) to not be imported as well.

Fortunately versioning (or downloading) only files within the .godot Folder from these"root-causes" makes the build process work again. Obviously less than ideal, but at least automation is restored that way easily.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.x Feb 17, 2023
@RevoluPowered
Copy link
Contributor Author

Tomorrow has come, I'm now resuming work on this.

@RevoluPowered
Copy link
Contributor Author

RevoluPowered commented Feb 20, 2023

Doing more on this today and should update this PR.

I found an issue where it will hang the editor headlessly but not with a renderer

@akien-mga
Copy link
Member

See #73595 which might partially overlap with what this PR aims to do.

@myaaaaaaaaa
Copy link
Contributor

myaaaaaaaaa commented Jun 27, 2023

@RevoluPowered Are you still working on this? If not, I might take a stab at this using an alternative approach suggested here, where --import would essentially be an alias to:

godot --headless --export-pack null /path/to/nonexistent/file

@aaronfranke
Copy link
Member

@myaaaaaaaaa No, we are not working on this, you can take it. An --import option would be very welcome, it would be a huge help for everyone in need of importing project resources on CI. Ideally do it in the last janky way possible, not just an alias for exporting to null. Also ideally make it not require a GPU or screen to be connected.

@akien-mga
Copy link
Member

Superseded by #90431. Thanks for the contribution!

@akien-mga akien-mga closed this Apr 9, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Apr 9, 2024
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.

10 participants