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.

Sign up for GitHub

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

Jump to bottom

C#: Use get_instance_binding instead of set #84947

Merged
merged 1 commit into from
Apr 4, 2024
Merged

C#: Use get_instance_binding instead of set #84947

merged 1 commit into from
Apr 4, 2024

Conversation

raulsntos
Copy link
Member

As discussed in RC, the C# wrapper types shouldn't be setting the instance_binding but instead getting it and using the create callback to add extra user data (like the CSharpScriptBinding in our case).

This should fix instantiating WebRtcPeerConnection but the linked issue seems to mention other problems so I'm not sure that it completely fixes it.

@raulsntos raulsntos added this to the 4.x milestone Nov 15, 2023
@raulsntos raulsntos requested a review from a team as a code owner November 15, 2023 20:14
dsnopek
dsnopek reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

The code changes look good to me. As discussed on RocketChat, I think using get_instance_binding() is correct for C#, and this should fix at least part of the WebRTCPeerConnection issue. However, I haven't done any testing.

This was referenced Nov 16, 2023
Closed
Closed
Closed
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Nov 17, 2023
@Faless
Copy link
Collaborator

Faless commented Nov 17, 2023

This has been confirmed to fix godotengine/webrtc-native#116 (see godotengine/webrtc-native#116 (comment))

dsnopek
dsnopek approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Like I said previously, I haven't tested this code, but it looks good to me! Using get_instance_binding() is definitely correct for this context

@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Apr 4, 2024
@akien-mga akien-mga merged commit c196d12 into godotengine:master Apr 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/instance_bindings branch April 4, 2024 18:17
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed needs testing cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsnopek dsnopek dsnopek approved these changes

Assignees
No one assigned
Labels
bug topic:dotnet
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

WebRtcPeerConnection is failing half the time on C#
4 participants
@raulsntos @Faless @akien-mga @dsnopek