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

Add left-handed spherical and cylindrical billboards #109605

Merged
merged 5 commits into from
Jan 24, 2025
Merged

Add left-handed spherical and cylindrical billboards #109605

merged 5 commits into from
Jan 24, 2025

Conversation

PranavSenthilnathan
Copy link
Member

@PranavSenthilnathan PranavSenthilnathan commented Nov 7, 2024 •
edited by jeffhandley
Loading

Adds the left-handed CreateBillboard and CreateConstrainedBillboard variants. The code gen and perf for the new variants are very similar to the existing right-handed versions.

Closes #93046

@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@dotnet-issue-labeler dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@jeffhandley jeffhandley added this to the 10.0.0 milestone Nov 7, 2024
@build-analysis build-analysis bot mentioned this pull request Nov 7, 2024
Open
jeffhandley
jeffhandley reviewed Nov 7, 2024
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Numerics/Matrix4x4.cs
@@ -224,7 +224,7 @@ public Vector3 Translation
public static Matrix4x4 Add(Matrix4x4 value1, Matrix4x4 value2)
=> (value1.AsImpl() + value2.AsImpl()).AsM4x4();

/// <summary>Creates a spherical billboard that rotates around a specified object position.</summary>
/// <summary>Creates a right-handed spherical billboard matrix that rotates around a specified object position.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

Once this is merged, please also follow up with a PR to update Matrix4x4.xml (dotnet/dotnet-api-docs). Once seeded from the original comments, we don't automatically flow changes over to the API docs repo.

@build-analysis build-analysis bot mentioned this pull request Nov 10, 2024
Open
3 tasks
@jeffhandley jeffhandley changed the title Add left handed spherical and cyclindrical billboards Add left-handed spherical and cylindrical billboards Nov 11, 2024
@PranavSenthilnathan
Copy link
Member Author

@tannergooding or @michaelgsharp PTAL when you get a chance

tannergooding
tannergooding reviewed Nov 19, 2024
View reviewed changes
src/libraries/System.Numerics.Vectors/tests/Matrix4x4LeftHandTests.cs Outdated
expected.M44 = +1.0f;

Matrix4x4 actual = CreateLookAt(cameraPosition, cameraTarget, cameraUpVector);
Assert.True(MathHelper.Equal(expected, actual), $"{CreateLookAtMethodName} did not return the expected value.");
Copy link
Member

Choose a reason for hiding this comment

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

nit: We have existing AssertExtensions.Equal(expected, actual, allowedVariance); helper that will provide better diagnostic messages in the case of failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated this and used 1e-5 for the variance (matches MathHelper.Equals). I think we can can use tighter errors in some cases where we have numbers with abs < 1 and having 6 sigfigs, but I'll just leave it for now.

@build-analysis build-analysis bot mentioned this pull request Jan 22, 2025
Open
3 tasks
This was referenced Jan 24, 2025
Open
Open
Open
tannergooding
tannergooding approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM. Couple small nits that would be nice to cleanup, but don't feel the need to block merging on getting them resolved

@build-analysis build-analysis bot mentioned this pull request Jan 24, 2025
Open
@PranavSenthilnathan PranavSenthilnathan merged commit 6e2d394 into dotnet:main Jan 24, 2025
142 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
be539ee 
* main: (22 commits)
  Clean up Stopwatch a bit (dotnet#111834)
  JIT: Fix embedded broadcast simd size (dotnet#111638)
  Revert potential UB due to aliasing + more WB removals (dotnet#111733)
  re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
  Handle OSSL 3.4 change to SAN:othername formatting
  JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
  JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
  JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
  [Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
  Change empty subject test certificate to include a critical SAN.
  Fix reversed code offsets in GcInfo (dotnet#111792)
  Swap some libraries areas between leads (dotnet#111816)
  Add left-handed spherical and cylindrical billboards (dotnet#109605)
  JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
  Fix Zip64ExtraField handling (dotnet#111802)
  Add build support for Android+CoreCLR (dotnet#110471)
  arm64: Add bic(s) compact encoding (dotnet#111452)
  JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
  Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
  JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephentoub stephentoub stephentoub left review comments

@jeffhandley jeffhandley jeffhandley left review comments

@tannergooding tannergooding tannergooding approved these changes

@michaelgsharp michaelgsharp Awaiting requested review from michaelgsharp

Assignees

@PranavSenthilnathan PranavSenthilnathan

Labels
area-System.Numerics new-api-needs-documentation
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Billboard Lefthanded APIs for System.Numerics.Matrix4x4
4 participants
@PranavSenthilnathan @jeffhandley @stephentoub @tannergooding