Skip to content

Uses StringName in GDExtension perf critical instance creation & method/properties setter/getter#896

Merged
touilleMan merged 7 commits into
godotengine:masterfrom
touilleMan:stringname-in-gdextension-api
Nov 8, 2022
Merged

Uses StringName in GDExtension perf critical instance creation & method/properties setter/getter#896
touilleMan merged 7 commits into
godotengine:masterfrom
touilleMan:stringname-in-gdextension-api

Conversation

@touilleMan

Copy link
Copy Markdown
Member

@bruvzg bruvzg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks really good.

A few missing things:

  • _alloc_and_copy_cstr helper functions can be removed from wrapper.hpp, class_db.hpp and method_bind.hpp.
  • There's GDNativeExtensionScriptInstanceToString which is defined to return const char *, but in the engine its return is assigned to String, probably should be changed as well.
  • Also, you have missed __constant_get_bitfield_name (in the type_info.hpp).

@touilleMan

Copy link
Copy Markdown
Member Author

@bruvzg thanks for the quick feedbacks 😃

I've updated the PR with your remarks

@touilleMan
touilleMan force-pushed the stringname-in-gdextension-api branch from c368bde to 40d2e45 Compare October 23, 2022 12:47
@touilleMan

Copy link
Copy Markdown
Member Author

@bruvzg While I'm at at it, I wonder why in Godot the ScriptInstance have a get_property_list/get_property_type/get_method_list method instead of letting them in Script
https://github.com/godotengine/godot/blob/0d04e7ec914ff609dfbdfff3dca04ed12a21e600/core/object/script_language.h#L173-L174

I suspect this is not really intended, but it creates the impression two instances of the same script can have different property/methods.
This is something possible in dynamic language such as Python (where a class basically a dictionary that can be updated at anytime), but I don't think this is used in Godot (GDScript doesn't allow to modify the class at runtime as far as I know).

I think it's a good thing to consider a script can never change once loaded (this is the kind of assertion that is expected, so you can be sure plenty of things will break in subtile way if an extension starts to do that)

So my guess is we should correct this (or at least add comment on GDNativeExtensionInstanceInfo in gdnative_interface.h) to make sure GDExtension user won't get mislead by this

@touilleMan
touilleMan force-pushed the stringname-in-gdextension-api branch 2 times, most recently from 0f06f94 to 4b815a1 Compare October 23, 2022 13:20
@touilleMan

Copy link
Copy Markdown
Member Author

This PR is running fine with the demo project, so I guess this is ready for review & merge \o/

@touilleMan
touilleMan force-pushed the stringname-in-gdextension-api branch from a698181 to feadd9a Compare October 23, 2022 20:36
@bruvzg

bruvzg commented Oct 24, 2022

Copy link
Copy Markdown
Member

This is something possible in dynamic language such as Python (where a class basically a dictionary that can be updated at anytime), but I don't think this is used in Godot (GDScript doesn't allow to modify the class at runtime as far as I know).

I think it's a bind for the get_property_list of Object instance which have script attached, not for the Script itself.

@touilleMan
touilleMan force-pushed the stringname-in-gdextension-api branch from feadd9a to b6ba0dc Compare November 8, 2022 20:45
@touilleMan
touilleMan merged commit ce3c083 into godotengine:master Nov 8, 2022
@touilleMan
touilleMan deleted the stringname-in-gdextension-api branch November 8, 2022 22:00
@groud

groud commented Nov 28, 2022

Copy link
Copy Markdown
Member

@touilleMan I believe this PR might have caused #921 to happen. Could you confirm whether it's likely ?

@touilleMan

Copy link
Copy Markdown
Member Author

@groud that's most likely the case !

I'll try to have an eye on this before our Thursday meeting 🤠

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants