Skip to content

Conversation

@Mickeon
Copy link
Member

@Mickeon Mickeon commented Jan 8, 2024

This PR fills in the class reference of GDExtension and GDExtensionManager. It also links to the existing tutorials.

I don't know what else to say.

More feedback and useful insight is necessary from actual GDExtension developers. A few of these descriptions are unfortunately still barren.

@Mickeon Mickeon requested a review from a team as a code owner January 8, 2024 18:51
@Mickeon Mickeon force-pushed the reduz-mystery-meat branch from c115260 to 80aa3f0 Compare January 8, 2024 19:01
@dalexeev dalexeev added this to the 4.3 milestone Jan 8, 2024
@dalexeev dalexeev requested a review from a team January 8, 2024 23:14
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks for being the first person brave enough to attempt to document these classes :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
Initializes the library bound to this GDextension.
Initializes this GDExtension at the given initialization level.

This should probably also have a note saying that developers shouldn't call this under normal circumstance, it will be handled by GDExtensionManager.

I'm actually not entirely sure why we've exposed this at all? It's especially weird considering we haven't exposed deinitialize_library(), its paired method.

Copy link
Member Author

@Mickeon Mickeon Jan 9, 2024

Choose a reason for hiding this comment

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

I'm actually not entirely sure why we've exposed this at all? It's especially weird considering we haven't exposed deinitialize_library(), its paired method.

It may be worth discussing this with the rest of the GDExtension team. There have been times before where Reduz and/or others accidentally exposed methods to the documentation & scripting language "just in case" (hence the name of the PR).
After a lot of time has passed, only then you can tell there's a flaw in the API when struggling to write documentation for these.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added an item to the agenda of tonight's GDExtension meeting, to discuss if this method should even be bound in the first place

@Mickeon Mickeon force-pushed the reduz-mystery-meat branch 2 times, most recently from 11406b0 to c07ac11 Compare January 9, 2024 00:34
@Mickeon
Copy link
Member Author

Mickeon commented Jan 9, 2024

Addressed all of the above feedback and suggestions. Thank you very much for the insight

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

@Mickeon Mickeon force-pushed the reduz-mystery-meat branch from c07ac11 to d8c3159 Compare January 9, 2024 09:53
@Mickeon
Copy link
Member Author

Mickeon commented Jan 9, 2024

Thank you, I'm very happy to have this as soon as possible.

@akien-mga akien-mga requested a review from dsnopek January 9, 2024 10:02
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

While I personally think we should probably unbind GDExtension::initialize_library(), and hence don't need docs for it, that doesn't necessarily need to hold up this PR.

This looks great!

@Mickeon Mickeon force-pushed the reduz-mystery-meat branch from d8c3159 to c4d7d7c Compare January 10, 2024 21:02
@Mickeon
Copy link
Member Author

Mickeon commented Jan 10, 2024

Accepted the above suggestion

@akien-mga akien-mga merged commit ea83a12 into godotengine:master Jan 11, 2024
@akien-mga
Copy link
Member

Thanks!

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.

6 participants