Add indexer to MatrixXxX es (preferably ref returning)

[BreyerW]

I would like to be able to query any value of matrix using indexer

Like:

Matrix4x4 m=....

var firstRowColumn=m[0,0]; //equivalent to m.M11

This is mainly convenience, but become big one if one want to get matrix property "randomly" e.g. in for loop. In this case one must write pretty big if else block or switch. ref might be useful to be able to send matrixes to graphic libraries efficiently and conveniently.

Next use case is migrating between vector libraries that support indexers (like OpenTk) Note: ref returning indexer shouldnt be problem there because omitting ref when calling means automatic dereference so in theory one could just change using and recomplie without problems (assuming no issues with world coordinates etc.)

Since ref and out alternative methods got postponed, i think patch with them is perfect to include ref indexers as well unless you want to backport them to ealier versions of c# than 7

EDIT: If you decide to implement ref indexer without changing internal representation, i think it could be implemented using Unsafe.Add. Something along these lines:

public ref float this[int row,int col]{
    get{

if(row<0 || row>3 || col<0 || col>3) throw new IndexOutOfRangeException();

             var index=0;
             if(row is 1)
                      index=4;
             else if(row is 2)
                      index=8;
             else if(row is 3)
                      index=12;

             index+=col;
             return ref Unsafe.Add<float>(ref this.M11,index );
     }
}

EDIT 2: With spans available, you could implement indexers for retrieving only row or only column (return 4-elements span which is constructed using DangerousCreate(this, ref this.MdesiredRow0, 4), Column is more tricky and probably would be implemented as tuple or float[4] since column arent layed out in continguous blocks, unless you add 'jumping' every X elements support to spans. Or just return span over whole matrix and force users to deal with 'jumping')

[BreyerW]

Ok, did some small testing and it turns out that i forgot that sample above wont work because one cant return by ref this if it is inside struct (and Matrix is). So i searched and coded potential replacements and found at least one:

[StructLayout(LayoutKind.Sequential)]
    public struct testStruct
    {
        public int field1;
        public int field2;
        public int field3;
        public ref int this[int i] => ref Span<int>.DangerousCreate(this, ref field1, 3)[i];
    }

I made extremely simple struct like above and populated it with random numbers:

var test=new testStruct() { field1 = 7, field3 = 67 };

And console show expected values:

Console.WriteLine(test[0]); //print 7
Console.WriteLine(test[1]); //print 0 (not initialized so zeromemory!)
Console.WriteLine(test[2]); //print 67

BTW sample above will work if you remove ref from before Unsafe and from indexer ofc but you lose ability to send matrixes like this SendToGPU(ref m[0,0],16) //send matrix4x4 to gpu without copying

[mellinoe]

Hi @BreyerW , thanks for the suggestion.

I think it is a reasonable addition. Most other matrix implementations do provide an indexer for convenience. I looked at OpenTK, SharpDX, and UnityEngine's Matrix types. GLSL and HLSL also allow matrix values to be indexed in a similar way.

I'm not sure that we can provide a by-ref indexer, though. I don't fully understand the safety implications around returning a reference to a structure's field. Obviously, it's not permitted by regular C#. @jkotas, could you shed some light on those concerns?

For what it's worth, I don't think the indexer being by-value is necessarily that bad. You probably don't want to be dynamically indexing into a matrix in a critical path, anyways. So it being slightly slower than using the fields directly, as a trade-off for actually being safe, is okay IMO.

[BreyerW]

You have valid concerns of course. However if there is no performance loss (there might be a problem since DangerousCreate receive object which probably imply boxing - although worth note that this is only one possible implementation - noone said there isnt more) and no additional complexity (example in 2nd post seems simple enough and there shouldnt be problem with safety since span is snapshotted and immediately discarded after return) then i would still implement ref indexer since it enable indexer to work with ref/out receiving methods.

Anyway, ref return is small bonus. If there wont be ref then it's no 'biggie' since i can still do ref m.M11 and be done with optimizations.

[mellinoe]

there might be a problem since DangerousCreate receive object which probably imply boxing

Yes, that would be a problem -- you're boxing on every call to the indexer. You could equivalently (?) use this pattern to avoid that:

int* ptr = ((int*)Unsafe.AsPointer(ref field1) + i);
return ref Unsafe.AsRef<int>(ptr);

Again, I don't think this is a generally safe pattern to use for a public API. Without the "ref return" part, I think we could move forward with the proposal.

@tannergooding Any thoughts on this one?

[jkotas]

int* ptr = ((int*)Unsafe.AsPointer(ref field1) + i);
return ref Unsafe.AsRef(ptr);

This pattern has GC hole. It will cause your app to crash in the GC intermittently. Do not ever do this.

[mellinoe]

@jkotas Thanks for the confirmation.

[jkotas]

I don't fully understand the safety implications around returning a reference to a structure's field. Obviously, it's not permitted by regular C#. @jkotas, could you shed some light on those concerns?

This is described in detail in dotnet/roslyn#5233 - look for "this in structs" paragraph.

[BreyerW]

Can i ask if this feature have chance to appear with ref/out alternative methods? If not could this feature request be changed to 'up-for-grabs'? I have to admit that lacking indexer is as much showstopper as lack of ref/out methods and if there is no chance to be shipped with ref/out, then i would like to help to make it possible ;)

Since there is consensus that indexer shouldnt be ref returning i belive best solution performance-wise will be combining col and row to index and then making big switch (which should be compiled to jump tables). Though maintanability-wise it might be somewhat 'nightmare'-ish compared to my 1st example (for 4x4 it would be 16 cases). If extra performance hit from using Unsafe class is acceptable then i could simplify it to something similar in first post, just without ref.

[jkotas]

If extra performance hit from using Unsafe class is acceptable

The Unsafe class should help the performance compared to the switch statement, not hurt it.

[mellinoe]

Can i ask if this feature have chance to appear with ref/out alternative methods?

Could you clarify what you mean? Are you referring to this proposal? If so, that is already planned to be added, hopefully in the next release past 2.0. I have a branch which implements it, but it needs coordination with JIT changes in order to preserve SIMD functionality.

Since there is consensus that indexer shouldnt be ref returning i belive best solution performance-wise will be combining col and row to index and then making big switch (which should be compiled to jump tables).

Right, I think that's the (only?) way to go. I don't really think there's any trickery that we can do with unsafe code or the Unsafe class to make it any faster, given we are talking about a struct indexer.

[BreyerW]

@mellinoe exactly that proposal. I wanted to know if u feel fairly capable of adding indexer before next release in net core env.

As for Unsafe return Unsafe.Add<float>(ref this.M11,index ); should work even for structs (note no ref return) as long as struct is decorated with sequential or explicit layout. At least VS doesn't complain about that code. What are safety implications im not sure (though c# is very restrictive about ref safety so i would be suprised if there is serious problem awaiting to ambush from behind ;)) nor im not sure anymore about performance since according to @jkotas Unsafe actually should be faster (i was slighty suprised since jump tables are already O(1) operation+there is not much more than directly returning field). I guess there is need for proper testing for both scenarios.

[jkotas]

i was slighty suprised since jump tables are already O(1)

Right, jump tables are significantly slower than a simple indexing that you get by using Unsafe.

[mellinoe]

I think we could quickly write up some perf tests to determine which version is faster. We should have the flexibility to choose either, with a small caveat. Right now, System.Numerics.Vectors doesn't rely on System.Runtime.CompilerServices.Unsafe, and we'd need to do some build configuration changes to allow it. But we could consider adding that dependency if we found that it was significantly better.

[tannergooding]

Marking this as blocked given it needs the language rules around ref returns for structs expanded.

[BreyerW]

@tannergooding ref isnt strictly necessary unless you would really like perfect solution. Without ref return on indexer this proposal can be implemented with existing API (although it should get expanded to get/set not just get if possible)