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

Add Min(float) and octahedron encode/decode to Vector3.cs #102356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a-johnston
Copy link
Contributor

Just a few methods which were missing for the cs struct. Is there a GodotSharp test suite I can update with these methods? In lieu of that I manually tested a few 100k random normalized vectors and the values matched between c# and gdscript test scripts 100% of the time.

@a-johnston a-johnston requested a review from a team as a code owner February 3, 2025 09:40
@AThousandShips AThousandShips added this to the 4.x milestone Feb 3, 2025
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.

Thanks, looks great!

It looks like we missed the Vector3.Min method in #80223 (4.3).

Regarding the octahedron methods from #54050 (4.0), I feel like they don't belong in Vector3 but that's where they ended up in Core. The implementation in this PR matches the C++ implementation.

Is there a GodotSharp test suite I can update with these methods?

No, we don't have tests for GodotSharp yet.

modules/mono/glue/GodotSharp/GodotSharp/Core/Vector3.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Vector3.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Vector3.cs Outdated Show resolved Hide resolved
@a-johnston a-johnston force-pushed the add_missing_cs_vector_methods branch from e123f85 to 14f7e6f Compare February 4, 2025 22:21
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.

LGTM, thanks!

@raulsntos raulsntos added cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release labels Feb 5, 2025
@raulsntos raulsntos modified the milestones: 4.x, 4.5 Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release cherrypick:4.4 Considered for cherry-picking into a future 4.4.x release enhancement topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants