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

Expose the left-handed matrix creation APIs #88930

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

tannergooding
Copy link
Member

This resolves #80332

@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.

@ghost ghost assigned tannergooding Jul 14, 2023
@ghost
Copy link

ghost commented Jul 14, 2023

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #80332

Author: tannergooding
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

Comment on lines -871 to -873
expected.M11 = 0.979457f;
expected.M12 = -0.0928267762f;
expected.M13 = 0.179017f;
Copy link
Member Author

Choose a reason for hiding this comment

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

There's no change in these values, they've just been simplified to the shortest roundtrippable string for consistency

/// <remarks>
/// Viewport matrix
/// | width / 2 | 0 | 0 | 0 |
/// | 0 | -height / 2 | 0 | 0 |
/// | 0 | 0 | maxDepth - minDepth | 0 |
/// | 0 | 0 | minDepth - maxDepth | 0 |
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a minor bug fix to the new API. It was creating a left-handed viewport.

}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Impl CreateLookToLeftHanded(in Vector3 cameraPosition, in Vector3 cameraDirection, in Vector3 cameraUpVector)
Copy link
Member

@krwq krwq Jul 16, 2023

Choose a reason for hiding this comment

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

why can't this just pass negative cameraDirection to the one above? Presumably this will be optimized anyway and perf should be the same since they're inlined?

Copy link
Member

Choose a reason for hiding this comment

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

(similar question to other methods)

Copy link
Member Author

Choose a reason for hiding this comment

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

This one in particular could, but that adds a hidden additional cost that the JIT can't currently optimize away. For the others the algorithms are not simply negating a single input.

Copy link
Member

Choose a reason for hiding this comment

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

is there an issue against JIT to improve this? is this cost high enough to justify code duplication?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there an issue against JIT to improve this

There are many issues tracking the broader issues that impact this space. It's not really limited to just one thing.

is this cost high enough to justify code duplication?

This is a known perf oriented type and the amount of code duplication is not significant. We have (ignoring the formatting done to help improve readability, around 8-11 lines of pretty simple code here.

@tannergooding tannergooding merged commit d77f87f into dotnet:main Jul 18, 2023
@tannergooding tannergooding deleted the fix-80332 branch July 18, 2023 22:44
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide "Left Handed" overloads of matrix create functions
3 participants