GDExtension: Add mem_alloc2 (and friends) so padding can be requested#108725
Conversation
bruvzg
left a comment
bruvzg
left a comment
There was a problem hiding this comment.
Seems reasonable to expose it, and always handle padding on the engine side.
Ivorforce
left a comment
Ivorforce
left a comment
There was a problem hiding this comment.
I totally missed that godot-cpp was doing its own pre-padding.
Fully agreed this is the right way to fix it. Additionally, this should fix double pre-padding (once by godot-cpp and once by godot), which may also have caused issues, in addition to wasting RAM.
I'd go as far to say as we should consider squeezing this into 4.5. As a new interface, it should be impossible for this to introduce new bugs, and it would be nice to already use it with godot-cpp in the 4.5 release (rather than in half a year from now).
The risk is more that an API is stabilized that then has to be indefinitely supported, whereas adding it after would give the whole 4.6 cycle for review, testing and feedback. At least this was the reasoning for the But yes, lately there are only 2 Godot releases per year, which isn't great for GDExtension which is tied to minor releases. It comes down to a lot of these make-or-break scenarios 😕 |
If everyone was on board with this (including the production folks), I think it could be OK for 4.5. It mirrors what Godot has already had internally for a long time, so there isn't too much we could get wrong about this API. Let's chat about it at next week's GDExtension team meeting
Yeah, I agree. I only starting digging into this problem on Monday, so this would be a very quick turn around for making a core change to the GDExtension interface. Especially given that the problem has existed since Godot 4.0 was released, so it isn't a 4.5 release blocker or anything However, like I said above, as far as APIs go, this one does seem pretty safe given that mirrors a fairly stable internal API (even Godot 3.0 has the |
aaronfranke
left a comment
aaronfranke
left a comment
There was a problem hiding this comment.
Looks good to me. As discussed in the GDExtension meeting, we should also, as a follow-up PR, warn whenever there is a mismatch between release and debug, and/or warn whenever the .gdextension file has a configuration that could lead to a mismatch.
|
Discussed at the GDExtension meeting, and we're OK with waiting for Godot 4.6 on this one. The issue has existed since Godot 4.0 and doesn't come up as often as other support issues. And this will allow us time to really test that debug and release builds can mixed and there are no other issues |
|
Thanks! |
6 participants
This is part of my plan to address godotengine/godot-cpp#1820 in godot-cpp
If we are able to request padding, then we should be able to make debug builds of GDExtensions (specifically ones using godot-cpp) not cause heap corruption when used with release builds of Godot
Given that we're towards the end of the development cycle for Godot 4.5, I'm targeting this for Godot 4.6