Add "dedicated server" export mode which can strip unneeded visual resources (superseded) #69546
Conversation
dsnopek
force-pushed
the
server-export
branch
2 times, most recently
from
43051e5 to
8d75691
Compare
dsnopek
force-pushed
the
server-export
branch
from
8d75691 to
d0b991a
Compare
|
My latest push fixes the issue with the file export cache! When checking if the cached file is usable, it's is now looking at the mtime for the .import file as well. This matches the md5 check, which was already checking both the source file and the .import file. Even though I'm still not super confident about the approaches I chose in this PR, I'm going to take it out of draft since there isn't anything in particular that I'm planning to change - at least until I get some feedback that is :-) |
dsnopek
changed the title
|
What do you think about the term |
|
Is the defaulted export type strip or keep? |
| if (const Texture2D *texture = Object::cast_to<Texture2D>(p_resource.ptr())) { | ||
| Ref<PlaceholderTexture2D> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_size(texture->get_size()); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (const Texture2DArray *texture = Object::cast_to<Texture2DArray>(p_resource.ptr())) { | ||
| Ref<PlaceholderTexture2DArray> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_size(Size2i(texture->get_width(), texture->get_height())); | ||
| placeholder->set_layers(texture->get_layers()); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (const Texture3D *texture = Object::cast_to<Texture3D>(p_resource.ptr())) { | ||
| Ref<PlaceholderTexture3D> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_size(Vector3i(texture->get_width(), texture->get_height(), texture->get_depth())); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (const Cubemap *cubemap = Object::cast_to<Cubemap>(p_resource.ptr())) { | ||
| Ref<PlaceholderCubemap> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_size(Size2i(cubemap->get_width(), cubemap->get_height())); | ||
| placeholder->set_layers(cubemap->get_layers()); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (const CubemapArray *cubemap = Object::cast_to<CubemapArray>(p_resource.ptr())) { | ||
| Ref<PlaceholderCubemapArray> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_size(Size2i(cubemap->get_width(), cubemap->get_height())); | ||
| placeholder->set_layers(cubemap->get_layers()); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (Object::cast_to<Material>(p_resource.ptr()) != nullptr) { | ||
| Ref<PlaceholderMaterial> placeholder; | ||
| placeholder.instantiate(); | ||
| return placeholder; | ||
| } | ||
|
|
||
| if (const Mesh *mesh = Object::cast_to<Mesh>(p_resource.ptr())) { | ||
| Ref<PlaceholderMesh> placeholder; | ||
| placeholder.instantiate(); | ||
| placeholder->set_aabb(mesh->get_aabb()); | ||
| return placeholder; | ||
| } |
There was a problem hiding this comment.
The logic here, (instantiating a placeholder resource which derives from a source resource), I don't think this is the best place for it ( dedicated_server_export_plugin.cpp). It seems like each class itself should be responsible for understanding how to create a placeholder version of itself... Perhaps each of these resources should have a to_placeholder() or similar function, and dedicated_server_export_plugin.cpp can be consumer of those interfaces instead of dedicated_server_export_plugin.cpp requiring knowledge of these details and being the implementer for each type?
There was a problem hiding this comment.
This is a great idea! That would certainly make it easier to progressively add more placeholders. I'll work this into the next revision.
| audio_driver = "Dummy"; | ||
| display_driver = "headless"; |
There was a problem hiding this comment.
Would be great if these weren't magic strings... (Not sure if there is a global somewhere, but would be a great refactor to add one if there isn't)/
There was a problem hiding this comment.
These same magic strings are hardcoded for the "--headless" CLI option:
We could maybe have some constants for these at least?
There was a problem hiding this comment.
Ya, that's what I suspected, which is why I was thinking thinking it would would be a good refactor & consolidate these into a common shared constant.
|
There was a problem hiding this comment.
See the technical comment below regarding the API consistency.
Regarding the design, I'm not particularly fond of this approach either, and I think we could try something differnt which moves the responsability completely to the exporter, leaving the importers untouched. Let me try to explain.
I think in the long run we could aim at something like we have for the resource exporter:
I'm not the best UX designer, but the idea is to have at least the following options:
- Dedicated server export: on/off
- Replace supported assets with placeholders: All/Selected
- Resources to preseve/replace (a Tree node with the resource list like in the above screenshot)
Then the DedicatedServerExportPlugin would simply check the export option, and the inclusion/exclusion list to decide if a resource with a valid placeholder should be replaced.
We could then iterate on this and maybe add a separate option which allows you to select classes or filter by path if the needs arise.
Does this sound reasonable?
| #ifdef TOOLS_ENABLED | ||
| ClassDB::bind_method(D_METHOD("set_dedicated_server_export_type", "server_export_type"), &Resource::set_dedicated_server_export_type); | ||
| ClassDB::bind_method(D_METHOD("get_dedicated_server_export_type"), &Resource::get_dedicated_server_export_type); | ||
| ADD_PROPERTY(PropertyInfo(Variant::INT, "resource_dedicated_server_export_type", PROPERTY_HINT_ENUM, "Strip,Keep"), "set_dedicated_server_export_type", "get_dedicated_server_export_type"); | ||
|
|
||
| BIND_ENUM_CONSTANT(DEDICATED_SERVER_EXPORT_STRIP); | ||
| BIND_ENUM_CONSTANT(DEDICATED_SERVER_EXPORT_KEEP); | ||
| #endif |
There was a problem hiding this comment.
As far as I know this breaks the API compatibility between editor and templates.
Godot has 2 API levels, a "core" one, and and "editor" one (see ClassDB::API_CORE and ClassDB::API_EDITOR usage in main.cpp).
The 2 API levels should not be mixed, and the ClassDB::API_CORE should be the same between editor and templates.
If we go with this approach of special methods which only works in editor, we should still bind the methods and constants in templates, and leave them with an empty implementation.
There was a problem hiding this comment.
Yeah, this was me attempting to not weight the templates down with APIs that they don't need. If we end up keeping something on Resource, I'll dig into ClassDB::API_CORE vs ClassDB::API_EDITOR and see if there's a better way to make these properties editor only.
|
@jordo Thanks for the review!
I took "dedicated_server" from the proposal, but I'm fine with whatever the consensus ends up being! I think I like @Calinou suggestion of "headless_server" better than "gameserver", since the distinction here is that this is only a server, not a client that also has a server mode or something like that.
The default is "strip" per the proposal. |
|
@Faless Thanks for the review!
That makes sense, and would definitely handle getting out of the importer, making this feature touch fewer systems, which is nice. However, how would we handle embedded resources (ie. like a mesh that's part of a scene, not saved to an external .tres file)? From a technical perspective, we are able to replace embedded resources with placeholders just fine, and this is something we probably want to have for 3D, since it's not uncommon to have mesh data embedded in a .tscn. (EDIT: In fact, I think that's the most common case, since if you drop a .glb into your project the imported .tscn will have meshes embedded by default.) How do we allow developers to configure which embedded resources to strip or keep? The current approach in the PR is nice in that external and embedded resources are "unified". It's the We could certainly keep the extra property added to Or, we could try to include embedded resources in the Tree node in the exporter UI? But then we'd have load every scene in the project and walk through its scene tree looking for embedded resources, which seems impractical. |
dsnopek
changed the title
|
This PR is superseded by #70377 |
dsnopek
changed the title
This implements a little more of godotengine/godot-proposals#2756 (but not all of it - this will take a few PRs), including:
OS.has_feature("dedicated_server").Engine.is_dedicated_server(), but I think a feature tag is better, because then you can override project settings using the tag (ie. using "setting.dedicated_server"), and features are commonly used to identify things about the platform. If others feel strongly, though, I can addEngine.is_dedicated_server()as well.--headlesson the CLI)This is was marked as a draft (although, no longer is) because:
There is an issue with the "file export cache" that I haven't solved yet. If you change the "Dedicated Server Export Type" on an imported asset, re-import and then export, it will export it as-if it still had the previous setting. However, if I delete.godot/exported/*/file_cacheand then re-export, it gets picked up fine.EditorExportPluginis the right choice? A lot of the code for this is scattered all over the place (because it has to be), and sodedicated_server_export_plugin.cppis actually pretty small. So, the plugin seems not totally worth it.