Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix
Object::notification
order #78634Fix
Object::notification
order #78634Changes from all commits
c4705a5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taking the address of a temporary, which is causing crashes when the functions are called later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, right because unlike
classdb_register_extension_class
we're not copying the data into a new internal struct, we're directly referring to the struct provided by the GDExtension. We should probably not do that - it'd probably be better to copy that data into an instance of the struct on the Godot side...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying the data might be better in general. This is per instance tho, so either we duplicate the memory for each instance, or somehow create a mapping...
Another solution for this specific case is to add another class with a virtual override Godot side:
ScriptInstance
ScriptInstanceExtensionDeprecated
(add this)ScriptInstanceExtension
Then have
gdextension_script_instance_create[2]
create the correct class. (edit: might requirereinterpret_cast
on thenative_info
member to make this work...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a proposed fix that copies the data: #81205
Hm, yeah, I suppose this does make each instance take up 23 pointers more memory... This is a trickier problem than I thought at first!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that. Looks like I don't have a good intuition for these kind of problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I worry about the scalability of this approach: what happens as we make more changes to this struct? (We already have a change queued up for #81119)
I'd like to avoid the compatibility code continuing to require maintenance and growing in complexity over time. Copying is at least simple and straight forward. But copying as done in my current PR is always a performance and memory hit, even on the new code path. What if the hit was only on the compatibility path, maybe that's an OK trade-off?
Directly relates this comment on the PR: #81205 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries - several other people (including me) read through your code without noticing it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that
if not (p_info->notification_func) {script_instance_extension->deprecated_native_info.notification_func = nullptr}
is needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think this is fine. If
p_info->notification_func
isnullptr
, thenscript_instance_extension->deprecated_native_info.notification_func
will get set tonullptr
as well.