Skip to content

Conversation

@dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented May 23, 2023

This is draft because I don't necessarily know that this is what we want to do yet.

See the discussion on issue #1119

When this is paired with a PR godotengine/godot#77410 to Godot that implements "possible solution nr 2", then both virtual and non-virtual methods are called with a consistent encoding (ie. Object **) and seem to work in my limited testing :-)

NOTE: This should fail tests until godotengine/godot#77410 is merged (if it is merged)

@dsnopek dsnopek added bug This has been identified as a bug regression labels May 23, 2023
@dsnopek dsnopek requested a review from a team as a code owner May 23, 2023 20:28
@dsnopek dsnopek marked this pull request as draft May 23, 2023 20:28
@saki7
Copy link
Contributor

saki7 commented May 23, 2023

We need tests for this issue (#1045 (comment))

  • Callee is non-virtual / virtual
  • Parameter is Object* / Ref<T> / Scalar
  • Parameter qualifier is Value / const Value&
  • Return type is void / Object* / Ref<T> / Scalar

Total: 2 * 3 * 2 * 4 = 48 cases

I think we should merge your PR #1101 asap.

@dsnopek dsnopek force-pushed the revert-pr-1044-1045 branch 4 times, most recently from 33605e1 to 87a9e4c Compare May 25, 2023 23:00
@dsnopek
Copy link
Collaborator Author

dsnopek commented May 26, 2023

I've updated the tests so that they fail due to the regressions from #1044 and #1045. (So, this failure is a success!)

Also, the tests seemed to have stopped failing on the bugs that #1044 and #1045 originally set out to fix! It looks like we were no longer getting a ptrcall in the situation we needed to, so I also swapped out the $Example for a var example: Example = $Example variable which got that correctly tested again as well.

The tests do pass when paired with the Godot PR godotengine/godot#77410

@dsnopek dsnopek force-pushed the revert-pr-1044-1045 branch from 87a9e4c to 7336564 Compare May 26, 2023 02:45
@dsnopek dsnopek changed the title [DRAFT] Revert the changes from PR #1044 and #1045 [DRAFT] Revert the changes from PR #1044 and #1045 and standardize on Object ** encoding in ptrcall May 26, 2023
@Daylily-Zeleen
Copy link
Contributor

The crash happen at overried EditorInspectorPlugin::_can_handles(Object * object), is it relate about this?

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 29, 2023

The crash happen at overried EditorInspectorPlugin::_can_handles(Object * object), is it relate about this?

Yes. If you use this PR together with Godot PR godotengine/godot#77410 then that crash won't happen. In a personal project I have a working EditorPlugin from GDExtension using these :-)

…standardize on `Object **` encoding in ptrcall
@dsnopek dsnopek force-pushed the revert-pr-1044-1045 branch from 7336564 to ad72601 Compare June 7, 2023 13:31
@dsnopek dsnopek changed the title [DRAFT] Revert the changes from PR #1044 and #1045 and standardize on Object ** encoding in ptrcall Revert the changes from PR #1044 and #1045 and standardize on Object ** encoding in ptrcall Jun 7, 2023
@dsnopek dsnopek marked this pull request as ready for review June 7, 2023 13:31
@dsnopek
Copy link
Collaborator Author

dsnopek commented Jun 7, 2023

Now that Godot PR godotengine/godot#77410 has been merged, I've rebased this and taken it out of draft.

The CI should pass now that the Godot changes are in and a new artifact has been created. If not, I'd like to get that fixed before merging this one.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

LGTM, TIWAGOS ;)

@akien-mga akien-mga merged commit b5a3aeb into godotengine:master Jun 7, 2023
@akien-mga
Copy link
Member

Thanks!

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

Labels

bug This has been identified as a bug regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants