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

EditorExportPlugin: Call _export_file for all resource types #93878

Merged

Conversation

allenwp
Copy link
Contributor

@allenwp allenwp commented Jul 3, 2024

If this PR is merged, it supersedes #93820 because that PR is based on the other previous fix that is reverted by this PR.

I'm currently keeping this in draft with separate commits for the purpose of review. I can switch this over to a final pull request and squash the commits into one when others have had a moment to look at the changes. (Or leave some as separate, like the revert for example. Not sure what the preferred practice would be.)

@allenwp
Copy link
Contributor Author

allenwp commented Jul 3, 2024

I've tested this with all the scenarios I can think of... It seems that remapping imported resources in the _export_file function works as expected (the same as it works for non-imported resources). It's not a use case that I expect people will want because the file you can remap to won't be imported, so I've added a note about exactly that in the documentation.

Testing is very welcome, of course!

@AThousandShips
Copy link
Member

AThousandShips commented Jul 3, 2024

Has #90365 really been reverted? Can you link the PR reverting it? It hasn't been marked as reverted or linked in any revert PR

Edit: Or do you mean that this reverts that PR?

@allenwp
Copy link
Contributor Author

allenwp commented Jul 3, 2024

Has #90365 really been reverted? Can you link the PR reverting it? It hasn't been marked as reverted or linked in any revert PR

Edit: Or do you mean that this reverts that PR?

Yes, I meant that this PR reverts that other PR. I’ve updated the PR comment to have more clear wording about this.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I did some testing and the files are correctly skipped or included. Could be nice to test some plugins that use customization (I don't have one).

@allenwp allenwp force-pushed the EditorExportPlugin_export_file_90365 branch from 65fc19b to f3f4a7d Compare July 5, 2024 03:24
@allenwp allenwp changed the title EditorExportPlugin: _export_file is called for all resource types EditorExportPlugin: Call _export_file for all resource types Jul 5, 2024
- Alternate fix to godotengine#67844 that calls `_export_file` for all resource types instead of implementing `skip()` for customize functions.
- Fixes godotengine#93823.
- Moved logic surrounding "Skip" and "Keep" imported files to happen before resource customization. Fixes godotengine#93825.
- Also fixes an issue that I suspect might exist where progress bars during export were incorrect due to imported files in the project that are configured as "Keep" or "Skip".
@allenwp allenwp force-pushed the EditorExportPlugin_export_file_90365 branch from f3f4a7d to 8e65966 Compare July 5, 2024 03:37
@allenwp allenwp marked this pull request as ready for review July 5, 2024 03:39
@allenwp allenwp requested a review from a team as a code owner July 5, 2024 03:39
@allenwp
Copy link
Contributor Author

allenwp commented Jul 5, 2024

I've left the commit that reverts the 4.3 beta 2 approach as a separate commit. Please let me know if I should squash this to be a part of my commit or if it would be better for a maintainer to revert it separate from my PR (not sure what the best practice is for Godot).

@akien-mga akien-mga merged commit 932c191 into godotengine:master Jul 5, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@allenwp allenwp deleted the EditorExportPlugin_export_file_90365 branch July 7, 2024 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants