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

GDExtension binary compatibility broken for _get_configuration_warnings() #88379

Closed
dsnopek opened this issue Feb 15, 2024 · 3 comments · Fixed by #88455
Closed

GDExtension binary compatibility broken for _get_configuration_warnings() #88379

dsnopek opened this issue Feb 15, 2024 · 3 comments · Fixed by #88455

Comments

@dsnopek
Copy link
Contributor

dsnopek commented Feb 15, 2024

Tested versions

System information

Ubuntu 22.04

Issue description

PR #68420 changed the return value for _get_configuration_warnings() from PackedStringArray to Array.

This breaks binary compatibility for GDExtension, which means that any GDExtension that implemented _get_configuration_warnings() would likely segfault if used with Godot master presently.

Suggestions from @akien-mga on RocketChat:

I think it might be best to revert the signature back and find a different way to handle property warnings. Could be a new virtual method, either for both node and property (deprecating the old one) or one only for property warnings.

Steps to reproduce

  • Install a GDExtension which implements _get_configuration_warnings() (for example, Terrain3D)
  • In the editor, add a Node that implements _get_configuration_warnings() a the scene (for example Terrain3D)
  • Crash!

Minimal reproduction project (MRP)

Try the sample in the Terrain3D extension

@Mickeon
Copy link
Contributor

Mickeon commented Feb 16, 2024

Personal opinion is to bring back the original method as the one being exposed, whose result merges with a new "_get_configuration_warnings... whatever name is chosen" used exclusively with arrays of dictionaries.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 16, 2024

Okay, I'm convinced. I hope whoever is at the helm to change this takes this these opinionated changes in mind:

  • Bring back _get_configuration_warnings the way it was with PackedStringArray;
  • Replace currently breaking method with an internal _get_configuration_warnings_dict. Do not expose it yet until further changes;
  • Delegate configuration warnings to Object itself in some way;
  • Allow associating configuration warnings to properties in a more dynamic way.
    • I'm imagining, right off the bat, associating a property to a Callable. Something akin to update_configuration_warnings can then be called when a property changes, passing the property name, and... I think you get the gist.

@akien-mga
Copy link
Member

I'd opt for reverting #68420 for now to fix the compat breakage, and then have @RedMser redo the PR with the suggested changes to avoid breaking the compatibility. Note that you can use git cherry-pick on the original commit to get it re-committed after the revert, then modify it.

@Mickeon Mickeon changed the title GDExtension binary comptability broken for _get_configuration_warnings() GDExtension binary compatibility broken for _get_configuration_warnings() Feb 17, 2024
Joex3 pushed a commit to Joex3/godot_voxel that referenced this issue Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Immediate Blocker
Development

Successfully merging a pull request may close this issue.

3 participants