-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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 "dedicated server" export mode which can strip unneeded visual resources #70377
Conversation
0b08db1
to
227e31b
Compare
227e31b
to
0717ed8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really amazing work 🚀 🏆 !
The code overall looks good, I tested it out a bit but it seems to have issues with instantiate scenes which have Editable Children
set.
See this project.
It seems that the SubScene/MeshInstance -> Surface Material Override
still reference the old texture:
ERROR: Resource file not found: res://.godot/imported/icon.png-487276ed1e3a0c39cad0279d744ee560.s3tc.ctex.
at: _load (core/io/resource_loader.cpp:219)
ERROR: Failed loading resource: res://icon.png. Make sure resources have been imported by opening the project in the editor at least once.
at: _load (core/io/resource_loader.cpp:214)
ERROR: Can't load dependency: res://icon.png.
at: load (core/io/resource_format_binary.cpp:699)
ERROR: Failed loading resource: res://.godot/exported/133200997/export-14584830dbc22d3f76a596eed5f4948e-node_3d.scn. Make sure resources have been imported by opening the project in the editor at least once.
at: _load (core/io/resource_loader.cpp:214)
ERROR: Failed loading scene: res://node_3d.tscn
at: start (main/main.cpp:3009)
8cb9390
to
3ec4554
Compare
@Faless The changes I just pushed fix the issue you found with "Editable Children"! If you re-test with the same project, you'll probably need to delete Another note: the I did a more targeted test as well, overriding Unrelated to the "Editable Children" bug, I also added a UI change, where it'll set "res://" as Strip by default when you first switch to doing a dedicated server export (but it won't mess with existing customizations, if there are any). This fits in with the original proposal that wanted to make Strip be the default. And it will prevent users from wondering why their export is still so big after just selecting the mode, but failing to mark any paths as Strip. |
One more thing: I hadn't realized that @reduz added the export plugin system (in #65135) specifically thinking about the dedicated server export! When I first started working on this PR, I couldn't figure out how to make the cascading path-based configuration work as an export plugin. However, I think I'm going to re-explore how this could be done as an export plugin, even if it means making changes to how export plugins work to accommodate it. If this looks better as an export plugin, I'll either push those changes here or make a new PR. In any case, I'll share what comes up in that exploration! |
c51cce7
to
0798f59
Compare
Alright! I just pushed a version that converts as much as I could into an export plugin. I still have a backup of the non-plugin version, in case folks end up wanting to go that way. But the code changes are much smaller in the plugin version (which is always good :-)) so just went ahead and pushed it here right away, rather than making a new PR. There's still things that aren't in the plugin, for example:
... but, overall, with the plugin, the changes to I also had to make some improvements to Please let me know what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much cleaner indeed! And it correctly exports scenes with editable children.
Though I am getting the following message spammed multiple times when exporting:
ERROR: Condition "!is_ancestor_of(p_node)" is true. Returning: false
at: is_editable_instance (scene/main/node.cpp:2010)
Which is weird, because the only usage of is_editable_instance
is in _export_customize_scene_resources
/_is_editable_ancestor
but at a glance it's supposed to work (the p_root
received is not an ancestor of p_node
? How can it be the root then?)
I'll make a MRP and post it below.
Okay, it took me a while because I couldn't get the error reliably (until I realized that you need to change the scene when re-trying to avoid cache to kick in). So, the error is now spammed for scenes without editable children (though the export works correctly). See the attached project. proj_srv_export_2.zip |
@Faless Aha, thanks so much for working that out! I've added your suggested change to the PR :-) |
0798f59
to
b4db65b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great now, amazing work! 💯 🎊
👍 |
b4db65b
to
8205042
Compare
Had a call with @reduz and he requested the following changes:
My latest push implements both things! Here's a screenshot of the UI (the new message appears below the tree of files): @reduz Does this match what you were thinking? |
8205042
to
4b988a5
Compare
On RocketChat, @reduz requested that the "Strip" text in the dropdown be changed to "Strip visuals". My latest push makes this change! I've updated the screenshot in the PR description to reflect the new text. |
4b988a5
to
8592b4b
Compare
904dba7
to
8c70e0f
Compare
@Calinou Thanks for all the review and suggestions! I think I've got them all merged into my latest push |
8c70e0f
to
74458b6
Compare
Thanks! |
This PR implements godotengine/godot-proposals#5970 and supersedes #69546
Based on feedback from @Faless on the old PR, this PR is mostly contained within the export system.
It also includes two suggestions that @jordo gave on the old PR:
A virtual methodPer @reduz's request this isn't a virtual method oncreate_placeholder()
was added toResource
so that it's up to each resource to convert itself into a placeholder if it can. This will make it a little easier to add more placeholder classes going forward.Resource
, it's bound individually on each resource that can make a placeholder.Here's a current screenshot of the UI:
Otherwise, see the proposal for information about how this feature is meant to work!
Bugsquad edit: This closes godotengine/godot-proposals#5970.
🍀 This work has been financed and kindly donated to the Godot Engine project by W4 Games. 🍀