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

Exclude unexposed classes from the extension_api.json #80852

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Aug 21, 2023

@Bromeon noticed that GDScriptNativeClass was included in the extension_api.json but that it wasn't possible to actually get that class at runtime. Looking at the code, it appears that GDScriptNativeClass isn't exposed (ie. there's no GDREGISTER_CLASS(GDScriptNativeClass) anywhere).

This PR adds a ClassDB::is_class_exposed() check in GDExtensionAPIDump::generate_extension_api() to ensure that unexposed classes aren't included in the extension_api.json.

Up until now, I think unexposed classes were mostly not included in the extension_api.json by accident: most simply weren't in ClassDB by the time that we were generating the extension_api.json.

Aside from GDScriptNativeClass, this change also removes the following other unexposed classes (at least for me - I did a diff of the extension_api.json with and without this PR):

  • FramebufferCacheRD
  • GDScriptEditorTranslationParserPlugin
  • GLTFDocumentExtensionPhysics
  • GLTFDocumentExtensionTextureWebP
  • GodotPhysicsServer2D
  • GodotPhysicsServer3D
  • IPUnix
  • MovieWriterMJPEG
  • MovieWriterPNGWAV
  • ResourceFormatImporterSaver
  • UniformSetCacheRD

I'm not familiar with all of these classes, but I think they are all meant to be internal, and not interacted with directly.

@dsnopek dsnopek added bug topic:gdextension cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 21, 2023
@dsnopek dsnopek added this to the 4.x milestone Aug 21, 2023
@dsnopek dsnopek requested a review from a team as a code owner August 21, 2023 15:09
@dsnopek dsnopek force-pushed the extension-api-exclude-unexposed branch from 25adcee to c73135b Compare August 21, 2023 15:58
@dsnopek dsnopek requested a review from a team as a code owner August 21, 2023 15:58
@akien-mga
Copy link
Member

akien-mga commented Aug 29, 2023

I think this would resolve this FIXME in 4.0-stable.expected, which can then be removed:

Misc
----
Validate extension JSON: API was removed: classes/FramebufferCacheRD
Validate extension JSON: API was removed: classes/UniformSetCacheRD

FIXME: These aren't written when dumping the interface with a headless build
(since there's no RD backend in use). We need to fix this inconsistency somehow.

(those two files can be listed among the others which were intentionally unexposed)

See also:

The same check may be needed in other places, such as the EditorHelp, or GDScript code completion.

I'm still unclear on when we want a class to be registered in ClassDB, but not exposed. I wonder if those are indeed meant to be skipped as done here, or whether they should not be in ClassDB in the first place. I guess their existence in ClassDB comes from using GDCLASS?

@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 29, 2023

I'm still unclear on when we want a class to be registered in ClassDB, but not exposed. I wonder if those are indeed meant to be skipped as done here, or whether they should not be in ClassDB in the first place. I guess their existence in ClassDB comes from using GDCLASS?

An unexposed class is a class that has GDCLASS() but isn't registered via ClassDB::register_class(). It will still get added to ClassDB when the first instance of the class is created (which is how GDExtensionAPIDump ends up encountering these ones, they just happen to have instances created before it runs), but it's ClassInfo is added with the exposed flag set to false.

Personally, I think it makes sense for all classes descending from Object to use GDCLASS(). This way an instance of the class will be identified as being the right class (ie obj.get_class() returns the correct class name), as opposed to Godot acting as if it was an instance of its parent class. But I know there's classes in Godot that omit GDCLASS() on purpose, so that's just my opinion and is up for debate. :-)

However, for the purposes of this PR, I don't think we need to figure out if these specific classes should have used GDCLASS() or not. Unexposed classes are a feature of ClassDB, so even if it's not these specific classes, there can always be unexposed classes in ClassDB and we don't want any of them in the extension_api.json because unexposed classes can't be used from GDExtension.

@dsnopek dsnopek force-pushed the extension-api-exclude-unexposed branch from c73135b to 39a604c Compare August 29, 2023 13:46
@dsnopek dsnopek requested a review from a team as a code owner August 29, 2023 13:46
@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 29, 2023

I think this would resolve this FIXME in 4.0-stable.expected, which can then be removed

In my latest push, I've removed the "Misc" section and moved those down to be included with the other unexposed classes that are removed via this PR. Until we update the extension_api.json from Godot 4.0 to also exclude those (I guess after this change is cherry-picked?), the exceptions will still be needed.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 29, 2023
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.

Those GLTF classes are indeed internal and do not need to be exposed at all.

@akien-mga akien-mga changed the title Exclude unexposed classes from the extension_api.json Exclude unexposed classes from the extension_api.json Aug 29, 2023
@akien-mga akien-mga merged commit da12106 into godotengine:master Aug 29, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 21, 2023
@YuriSizov
Copy link
Contributor

We've discuss this a bit, and it seems it's better to avoid cherry-picking this. While absolutely beneficial, it does introduce a change to the dumped API, and because of that to our compatibility checker. The changes here would make 4.1's 4.0-stable.expected deviate from master's 4.0-stable_4.1-stable.expected, which should in theory be in sync.

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.

5 participants