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

Allow resizing Strings from GDExtension #79156

Merged
merged 1 commit into from Jul 27, 2023

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jul 7, 2023

This adds a new function to gdextension_interface.h to allow us to implement String::resize() in godot-cpp per issue godotengine/godot-cpp#1141

This is added to gdextension_inteface.h rather than ClassDB, because we don't want scripting to have access to it. String::resize() only makes sense if you can access String::ptrw() and directly modify the data at the pointer, which would be impossible from GDScript.

This originally came up when trying to compile the text_server_adv module in Godot as a GDExtension. See: #77532

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Is it necessary to expose it directly, adding bind_string_method(resize, sarray("size"), varray()); to the variant_call.cpp to expose existing method should work. The other string functions are in the interface due to pointer arguments, but in this case it's only int size.

@bruvzg
Copy link
Member

bruvzg commented Jul 7, 2023

because we don't want scripting to have access to it.

Why? It's exposed for the Arrays, what's the difference, String have writable [] operator exposed, which is the same as ptrw() ?

@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 7, 2023

Why? It's exposed for the Arrays, what's the difference, String have writable [] operator exposed, which is the same as ptrw() ?

Ah, that's a good point!

When I get a chance, I'll make a new PR that exposes it normally...

@dsnopek dsnopek closed this Jul 7, 2023
@dsnopek dsnopek mentioned this pull request Jul 7, 2023
@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 7, 2023

I've open PR #79177 which exposes String.resize() the normal way.

Given that String.resize() takes the desired array size of the string (ie. including the null terminator), I'm now less sure that we really want this exposed to GDScript, at least without some additional changes to make it more user-friendly. (Some possible options listed over on the other PR.)

@AThousandShips
Copy link
Member

As discussed in #79177 this seems to be the better way to handle this, assuming it doesn't suffer from the same mutability issues

@dsnopek dsnopek reopened this Jul 8, 2023
@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 27, 2023

Discussed at the GDExtension meeting, and we think it makes sense to add this, however, it would be good to add some additional warnings in the documentation about this being a potentially dangerous operation.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 27, 2023
@YuriSizov
Copy link
Contributor

it would be good to add some additional warnings in the documentation about this being a potentially dangerous operation.

Do you mean the online documentation? Or is there something you still want to add to the class references (and therefore need to update this PR)?

@dsnopek
Copy link
Contributor Author

dsnopek commented Jul 27, 2023

Do you mean the online documentation?

We were talking about the docs in the comments in gdextension_interface.h. I've already done a push since the meeting that adds this:

 * Warning: This is an error-prone operation - only use it if there's no other
 * efficient way to accomplish your goal.

@YuriSizov YuriSizov changed the title Allow resizing String's from GDExtension Allow resizing Strings from GDExtension Jul 27, 2023
@YuriSizov YuriSizov merged commit 6bfda7f into godotengine:master Jul 27, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

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.

None yet

4 participants