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

Revise implementation of C# Aabb.GetSupport to match the implementation in core #88919

Merged
merged 1 commit into from
May 21, 2024

Conversation

nongvantinh
Copy link
Contributor

fixes: #88834

@nongvantinh nongvantinh requested a review from a team as a code owner February 27, 2024 16:20
@raulsntos raulsntos added this to the 4.3 milestone Feb 27, 2024
@paulloz paulloz requested a review from a team May 1, 2024 16:39
@paulloz
Copy link
Member

paulloz commented May 1, 2024

We'd be interested in opinions from core contributors here, since we're not entirely sure about the implications.
The C++ implementation changed in #44468, and was not ported to C# before the current PR.

@nongvantinh
Copy link
Contributor Author

I think he might have forgotten to update the C# module because he's too busy. Besides, what benefits could this diversity bring?

@paulloz
Copy link
Member

paulloz commented May 2, 2024

I think he might have forgotten to update the C# module because he's too busy. Besides, what benefits could this diversity bring?

This isn't really the question. We (referring to @raulsntos and I) are simply trying to establish which is less harmful between:

  • Keeping the current behaviour.
  • Introducing the change, and potentially breaking people's code currently relying on this behaviour.

In essence: pushing aside the difference with GDScript, does the returned value even make sense? Hence, the review from another team, more aware of the implications.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

What we got from the rendering team on RC is that the current GetSupport implementation returns the opposite of the support, and this PR fixes that. We also think that it's unlikely there are many users consuming this API, so we think breaking compat here is acceptable. Let's merge this for the 4.3 beta, and see if we get user reports.

@akien-mga akien-mga merged commit 0cf42d6 into godotengine:master May 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@nongvantinh nongvantinh deleted the fix-88834 branch May 21, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDScript AABB::get_support and C# Aabb.GetSupport point to opposite corners
4 participants