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

Proposal containing the results of the SilkX experiment #1727

Merged
merged 10 commits into from
Feb 12, 2024
Merged

Conversation

Perksey
Copy link
Member

@Perksey Perksey commented Oct 12, 2023

This shall remain in draft until the SilkX experiment has concluded!

Copy link
Contributor

@Khitiara Khitiara left a comment

Choose a reason for hiding this comment

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

overall LGTM

@uwx
Copy link

uwx commented Oct 16, 2023

Notes from Discord chat:

For this requirement, how do we implicitly handle this without a known length? For a C-string we can search for the null-terminator, but not all strings will necessarily follow that pattern, and not every byte ptr will be a c-string leading to a crash when VS inspects it. This also feels like something that should be in an extension method since really it should only be defined for byte, sbyte, short, ushort, int, uint as other generic types will not convert nicely.

what is the motivation for the implicit operator? having to do new string(x) makes more sense to me
like, in C++ there is no implicit cast from char* to std::string
because the process of that conversion is not determinable
you have a pointer to memory, and i think it would be unwise to make assumptions about what that pointer means

ok lets make it an explicit operator for now

Also, need a thorough discussion about left-to-right order of PtrPtrPtr chains, and what Mut and Ptr should be named for clarity

* Update SilkX Proposals

- Updated Pointer Proposal to match current implementation
- Add Generic Math Proposal based on Current work

* Added missing function proposals and fixing some language

* Implementing Review Feedback

* Major rewrite of Generic Math Proposal

-Slimmed it down though

* Remove Vector 5 note

* Addressing Review Comments

* More Review Comments

* Vector Transform Adjustments

-and fix CreateFromPointNormal nomenclature

* Remove  the Endians that escaped the first time

* Update for Rect/Box changes

* Added LH/RH Matrix Functions

* Box/Rect changes

Making this a set of master types with obfuscated implmentation. Go For Perf
@Perksey
Copy link
Member Author

Perksey commented Nov 19, 2023

Maths

  • We agree with the addition to add scalar operations over vectors (i.e. Vector4 * T does XT, YT, ...)
  • Add an analyser for encouraging the most correct and most efficient type instead of using sub-optimal types.
  • Ensure we've documented that.
  • Vector * Matrix?
    • assuming row major, not allowing matrix * vector (only vector * matrix)
    • does it make sense to just do it the way game engines do it and the way 2.0 does it?
    • possibly technically invalid mathematically, even if common programmatically.
    • easy to hit corner case that not all users may understand.
  • Ensuring we have no upcasting and downcasting operators that could have unexpected behaviour.
  • System.Numerics is strictly right hand, which is the same as XNA and somewhat default for OpenGL users. This differs with DirectX that is typically left hand.
    • Happy with having explicit LH and RH functions.
  • Missing matrix APIs that were added in .NET 8 to System.Numerics
    • CreateLookTo (additional to CreateLookAt)
    • CreateViewport
    • CreateBillboardLH
    • CreateConstrainedBillboardLH
    • CreateBillboardRH
    • CreateConstrainedBillboardRH
  • Investigate newer less-hated alternatives to quaternions for future (see Freya Holmer talks)
  • Possibly have a general Transform struct in the future?
  • Rectangles and boxes are obscured with regards to internal layout, bindings to have interchange types that implicitly cast to the mathematical constructs that are represented by the Box/Rectangle types.
  • Investigate in the future if we can either add an Origin property for compatibility or an analyser for users to know that Min is the new Origin.
  • Approved notwithstanding the missing APIs.

@Perksey
Copy link
Member Author

Perksey commented Nov 19, 2023

SilkTouchX

  • We discussed a particular problematic case where RegisterClassEx returns an atom which is later reinterpreted to be a pointer - a debugger will explode when inspecting this pointer as it is not necessarily
  • Where ReadOnlySpan represents a string and we don't just want to pass the ref as-is (i.e. we want to add the null terminator like we do for string).
  • Generally we think that providing a tool that works 90% of the time is fine, the unsafe overloads are always there, but we'd worry about users making incorrect assumptions and we can probably do implicit behaviour for that final 10%.
  • Require that users manually encoding strings add that null terminator and document this. Our implicit ones do the right thing.
  • Don't allow ref types to throw an error when being handed something that isn't a valid pointer.
  • Approved provided that we:
    • make unsafe available
    • special case ROSpan as above
  • Future discussions need to be had on Vulkan implementation intricacies (getProcAddr) and also the addition of "complex" overloads.

@Perksey
Copy link
Member Author

Perksey commented Nov 21, 2023

@curin @uwx @Khitiara If any of you want to put up a PR for the CreateLookTo (additional to CreateLookAt), CreateViewport, CreateBillboardLH, CreateConstrainedBillboardLH, CreateBillboardRH, CreateConstrainedBillboardRH it'd be much appreciated sop we can get this in.

I'll update SilkTouchX now.

@Perksey Perksey marked this pull request as ready for review December 21, 2023 19:23
@Perksey Perksey requested a review from a team as a code owner December 21, 2023 19:23
@Perksey
Copy link
Member Author

Perksey commented Dec 21, 2023

@dotnet/silk-dotnet @uwx @Khitiara This is ready for merge I think.

Copy link
Contributor

@Khitiara Khitiara left a comment

Choose a reason for hiding this comment

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

LGTM

@Perksey
Copy link
Member Author

Perksey commented Jan 9, 2024

Is it possible to get an approval here? The revisions to the proposals are as discussed in the WG meeting.

@Perksey
Copy link
Member Author

Perksey commented Feb 12, 2024

NOOOOOO NOT THE NATIVE BUILD JOBS

@Perksey Perksey enabled auto-merge (squash) February 12, 2024 18:36
@Perksey Perksey disabled auto-merge February 12, 2024 18:44
@Perksey
Copy link
Member Author

Perksey commented Feb 12, 2024

Build is not triggering because there have been no source changes, merging manually.

@Perksey Perksey merged commit df25011 into main Feb 12, 2024
15 of 27 checks passed
@Perksey Perksey deleted the proposal/silkx branch February 12, 2024 18:45
- CreateConstrainedBillboardLH
- CreateBillboardRH
- CreateConstrainedBillboardRH
- Investigate newer less-hated alternatives to quaternions for future (see Freya Holmer talks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Perksey don't suppose you have a link? Quaternions are, of course, ontologically evil, so I would be interested to hear about viable alternatives. 👀

Copy link
Member Author

@Perksey Perksey Apr 5, 2024

Choose a reason for hiding this comment

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

I think it was @HurricanKai or @ThomasMiz that raised this point (and alluded to things like rotors as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants