Update to Faster math conversions - #8392
#8392Update to Faster math conversions#8392Doprez wants to merge 5 commits into
Conversation
| /// <summary> | ||
| /// Returns a <see cref="Matrix"/>. | ||
| /// </summary> | ||
| public static Matrix ToMonoGame(this System.Numerics.Matrix4x4 matrix) |
There was a problem hiding this comment.
Wondering if there needs to be copious doc in Matrix/Vector* structs etc to always mirror Numerics layout? These are not things that will ever change but mostly as a way to ensure across platforms/byte-order etc it should hold true. (Tests?)
Also, it's kind of implicit but ref <struct> doesn't cause an alloc (cool) but may need a doc comment explaining the gymnastics happening here to help the compiler use the Numerics without any allocs.. i.e. this
There was a problem hiding this comment.
are you talking about docs to inform developers using the xml comments? or are you talking about the layout attribute to ensure the struct matches the numerics attribute explicitly? I may be missing something so please let me know but both are row major matrices so I believe the memory layout should match. Tests would be a great idea though, just to be sure.
Instead of ToMonoGame(this System.Numerics.Matrix4x4 matrix) it should be ToMonoGame(this ref System.Numerics.Matrix4x4 matrix) to ensure the clarity of whats expected right? I can add a better description to the xml doc as well if it feels needed.
There was a problem hiding this comment.
are you talking about docs to inform developers using the xml comments? or are you talking about the layout attribute to ensure the struct matches the numerics attribute explicitly? I may be missing something so please let me know but both are row major matrices so I believe the memory layout should match. Tests would be a great idea though, just to be sure.
I meant both: xml comment and a layout attribute. especially people may not be aware that there is link between the two. (note: am not a maintainer, so this is mostly a drive-by :) )
Instead of
ToMonoGame(this System.Numerics.Matrix4x4 matrix)it should beToMonoGame(this ref System.Numerics.Matrix4x4 matrix)to ensure the clarity of whats expected right? I can add a better description to the xml doc as well if it feels needed.
Here, I meant that you are also avoiding extra allocs (because of ref <struct>) which is kinda nice - but to me it was not obvious at first glance and hence I had to double check dotnet docs :) simply adding that to the xml comment will be nice...
|
This will need to wait for the console runtime to be fully switched to NativeAOT and will also need to be tested on consoles, I have some doubts about |
@tomspilman is this still a blocking concern noted by @ThomasFOG ? |
|
@Doprez we would need you to update your initial PR description to include the new statement regarding our LLM policy as shown in the updated PR templates here: https://github.com/MonoGame/MonoGame/blob/develop/.github/PULL_REQUEST_TEMPLATE.md Just need to add the following and check it off at the bottom of your PR description |
|
It has been done, I will come back to this later and fix the conflicts as well when I have a bit more time. |
I was looking through Mono to see if you all were having the same problems of the existing System.Numerics library built into .NET or if you were still using your own math classes.
I noticed the conversion methods were creating new values instead of using the Unsafe class which has been very helpful for our Bepu to Stride conversions that were a bottleneck before finding this option.
This PR changes the built in conversions to use
Unsafe.Asto convert instead of creating a new value.The one thing I dont understand is why the implicit conversions for
Matrixare as fast as theUnsafe.Asbut the implicit forVector3andQuaternionare what I would expect from using the new keyword.This is the example where I forced
Matrixto use the new keyword the rest are using the existing implicit conversion:And this is the example of using the
Matriximplcit conversion:here is the gist files of the benchmarks I ran https://gist.github.com/Doprez/04d61e03915a9527a482cddbaa826544
Contributor Declaration