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

Expose GLTFDocument.get_registered_gltf_document_extensions #100783

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lyuma
Copy link
Contributor

@lyuma lyuma commented Dec 24, 2024

These APIs were not exposed, but I found I had a lot of difficulty using GLTFDocumentExtensions, particularly at runtime, without the ability to see and modify the current static list of extensions. My game only needs get_registered_gltf_document_extensions.

I explained some context for this change from chat:

I was trying to make a node with a gltf URI as a property. It would load the referenced gltf document as a sibling.
...And I wanted all the logic contained within the node
But then I discovered that the vrm extensions were not registered at runtime... and so I said oh that's easy, I will just add the extensions when I import and remove them when I finish import to avoid doubling up, but then I wanted to do it in a thread and sometimes they weren't getting cleaned up etc
And sometimes i had more than one instance of the node
But I didn't want to require users of my node to modify their project autoload settings
And VRM maybe should register the extensions at runtime in an autoload but I didn't because when I wrote VRM I didn't realize extensions list was static so all the example code is wrong and it is still wrong because there is no way to do it right for now
So that's the root cause of this discussion: I was trying to figure out how a node, without knowing anything else, can make sure the VRM extensions are added to the static gltf document extensions list
Even the VRM viewer code (which I maintain) technically does it wrong by registering it in the load function.
https://github.com/V-Sekai/TOOL_model_explorer/blob/main/vsk_model_explorer/core/ModelExplorer.gd#L90

I also improved the documentation, particularly to warn against the pitfalls I encountered, notably not realizing the list was static, and registering multiple copies of the same extension.

Also, a note about get_supported_gltf_extensions(): I know this API already exists, but it only works in cases where a GLTFDocumentExtension implements a glTF extension, and it also doesn't allow for getting a reference to the extension itself if it needs to be unregistered. Not all extensions, notably the built-in GLTFDocumentExtensionConvertImporterMesh, implement a glTF extension, so they will not appear in get_supported_gltf_extensions. That's why I want a new API to return the instances themselves.

@lyuma lyuma added this to the 4.4 milestone Dec 24, 2024
@lyuma lyuma requested review from a team as code owners December 24, 2024 11:20
@lyuma lyuma force-pushed the expose_get_clear_gltf_document_extensions branch 3 times, most recently from 9614754 to 3bf3312 Compare December 25, 2024 08:34
@lyuma lyuma changed the title Expose get_all_gltf_document_extensions and unregister_ APIs Expose GLTFDocument.get_registered_gltf_document_extensions Dec 25, 2024
@lyuma lyuma force-pushed the expose_get_clear_gltf_document_extensions branch from 3bf3312 to d13d101 Compare December 25, 2024 08:39
@lyuma lyuma force-pushed the expose_get_clear_gltf_document_extensions branch from d13d101 to 7ff6321 Compare December 25, 2024 13:09
@akien-mga akien-mga changed the title Expose GLTFDocument.get_registered_gltf_document_extensions Expose GLTFDocument.get_registered_gltf_document_extensions Jan 7, 2025
@akien-mga akien-mga requested a review from aaronfranke January 7, 2025 22:39
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

The feature is good, but I have to nitpick the name and docs.

<method name="get_supported_gltf_extensions" qualifiers="static">
<return type="PackedStringArray" />
<description>
Returns a list of all support glTF extensions, including extensions supported directly by the engine, and extensions supported by user plugins registering [GLTFDocumentExtension] classes.
[b]Note:[/b] If this method is run before a GLTFDocumentExtension is registered, its extensions won't be included in the list. Be sure to only run this method after all extensions are registered. If you run this when the engine starts, consider waiting a frame before calling this method to ensure all extensions are registered.
[b]Note:[/b] This method is static: Registered instances will be shared by all [GLTFDocument] instances. Consider using this method to check if a given [GLTFDocumentExtension] is already registered. If this method is run before a GLTFDocumentExtension is registered, its extensions won't be included in the list. Be sure to only run this method after all extensions are registered. If you run this when the engine starts, consider waiting a frame before calling this method to ensure all extensions are registered.
Copy link
Member

Choose a reason for hiding this comment

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

These should be 2 separate notes. Also, specifically, this method isn't useful for checking for GLTFDocumentExtension classes (that's what the other one is for, the new method you're exposing), this is only for checking the list of supported glTF extensions, which is separate from the list of Godot implementations via GLTFDocumentExtension.

Suggested change
[b]Note:[/b] This method is static: Registered instances will be shared by all [GLTFDocument] instances. Consider using this method to check if a given [GLTFDocumentExtension] is already registered. If this method is run before a GLTFDocumentExtension is registered, its extensions won't be included in the list. Be sure to only run this method after all extensions are registered. If you run this when the engine starts, consider waiting a frame before calling this method to ensure all extensions are registered.
[b]Note:[/b] If this method is run before a GLTFDocumentExtension is registered, its extensions won't be included in the list. Be sure to only run this method after all extensions are registered. If you run this when the engine starts, consider waiting a frame before calling this method to ensure all extensions are registered.
[b]Note:[/b] This method is static: Registered instances will be shared by all [GLTFDocument] instances. This method may be used to check if a given glTF extension is already supported by Godot's glTF importer, such as via a registered [GLTFDocumentExtension] class.

@@ -85,6 +85,7 @@ class GLTFDocument : public Resource {
static void unregister_gltf_document_extension(Ref<GLTFDocumentExtension> p_extension);
static void unregister_all_gltf_document_extensions();
static Vector<Ref<GLTFDocumentExtension>> get_all_gltf_document_extensions();
static TypedArray<GLTFDocumentExtension> get_registered_gltf_document_extensions();
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just a script binding version of the existing get_all_gltf_document_extensions function, we should use the same name for both (one can be suffixed with _bind and exposed without the _bind in ClassDB::bind_static_method).

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.

3 participants