the Fiddler's picture

OpenTK.Math.Half

Project:The Open Toolkit library
Component:Code
Category:feature request
Priority:normal
Assigned:Inertia
Status:closed
Description

This type should provide an interface similar to IntPtr, with a methods, conversion operators and constructors that can pack/unpack floats and doubles.

I'm opening this task so we can keep track of progress.


Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
the Fiddler's picture

#21

I was thinking more about the internal implementation, for example stuff like pointer casts and unsafe code (can be used to interop with Mono.SIMD).

Do these results still hold if you move the tests around (e.g. put the constructor test second). This is abnormal, the first and last tests are practically the same code:

Half h = new Half(135f);
Half h = (Half)135f;
 
public static explicit operator Half(float f)
{
     return new Half(f);
}
Inertia's picture

#22

Release - no optimization

Timing tests
Constructor: 1,3047712 seconds
FromSingle: 1,3008029 seconds
Conversion: 1,3021977 seconds

Release - optimized

Timing tests
Constructor: 1,1260197 seconds
FromSingle: 0,9906694 seconds
Conversion: 0,9889157 seconds

both executed a couple of times outside the VS IDE, results don't vary much. The FromSingle() is in no case slower than the constructor, I'd really appreciate if we just have both and let the user make his pick, instead of enforcing the use of the constructor by leaving no alternative.

Currently merging the additions you made, will post it here asap.

@objarni: I will start censoring your posts if you keep forcing off-topic remarks into discussions. Besides the timing results you posted you didn't talk about anything related to Half, which is the topic of this discussion. Create a new topic that matches the problem.

Inertia's picture

#23

Changelog:

  • CLS Compliant! yey.
  • removed the union in favor of unsafe. Works fine.
  • Added a Half4 struct that accepts OpenTK.Math.Vector4 as input and output. Does not have throwOnError constructors yet. Will add Vector4d aswell.
  • Changed cast-to and -from int operators to double. (Reason below.)
  • Added most of the inline docu.
  • Removed constructors of the style public Half(Half h). No point in that, it's a struct.
  • Added more regions to improve overview.
  • Added some temporary file that holds stuff that is not going to be needed once integrated into OpenTK.Math.

Imho we should abandon all notion of using any integers with Half and just work with Half <-> Float <-> Double. This limits the options significantly and does not hide 'the' problem. People can still cast to float/double and convert that to half, if they must. The only reason I think of atm what sense integers would make here is something like this:

ushort U = 12345; // TexCoord [0..1] range.
Half u = U / (float) ushort.MaxValue; // you have to cast to float anyways to perform the division.
AttachmentSize
OpenTK Half v5.rar6.2 KB
the Fiddler's picture

#24

Thanks! I just checked and updating GL to use the new type is as single as changing the typemap in the generator (patch attached). Our work here is almost done!

[FromSingle]
Inertia, can you think of any scenario where this method could conceivably make even a remote difference? (I honestly cannot think of an.) If this part is actually important, we should simply bring back the LUT.

Or, we could spend our energy integrating Mono.SIMD, which would actually make a (world of) difference.

[Cast to/from double instead of int]
Makes sense.

AttachmentSize
csharp.tm.patch486 bytes
Inertia's picture

#25

"Our work here is almost done!"
Yours maybe, I still have Half2,3,4 to do. :P

[save/load]
Is there any interface you want me to derive from? I couldn't find any that implements byte[] GetBytes.

[FromSingle]
None really. I see this more like an overload to get the same job done. I don't really care which is faster (the conversion process will most likely only be used at initialization time in programs when you build VBOs or Textures), it's simply that you have the option to avoid the 'new'. Yes, I'm aware 'new' is extremely cheap in .Net, but when you try hard enough you will probably find scenarios in which one case works better than the other, so it's good to have 2 options rather than 1. It's like 10 lines extra code for the method, and there's no difference in the result.

[Cast to/from double instead of int]
I wasn't sure of it either yesterday, today it made sense ;) So it's ok when I remove all integer constructors? (they're commented out atm)

"Or, we could spend our energy integrating Mono.SIMD, which would actually make a (world of) difference."
Not sure if this works, but can't we simply cast the Simd.Vector4 to our Math.Vector4? The fields align, but it would be fugly.

objarni's picture

#26

@objarni: I will start censoring your posts if you keep forcing off-topic remarks into discussions. Besides the timing results you posted you didn't talk about anything related to Half, which is the topic of this discussion. Create a new topic that matches the problem.

I'm sorry if I offended you - but I don't think my remarks are off-topic: they have to do with a specific design decision regarding the Half type, a decision that has to do with elegance vs performance.

So please don't censor me.

Inertia's picture

#27

No offense taken, but this discussion is again leading off-topic :P Please no more posts that do not include at least 1x "Half", 1x "cast", 1x "math" and 1x "overload". Thanks!

objarni's picture

#28

You got it - and I'm taking your idea and making the "elegance vs performance" a new topic - to try and avoid hassles between the "elegance camp" and the "performance camp". Cya there! :)

the Fiddler's picture

#29

Yours maybe, I still have Half2,3,4 to do. :P
Look at it from the bright side, we won't have to implement overrides for every single function with 'h' in its name!

[Serialization]
The usual implementation is add the [Serializable] attribute and (if necessary) implement the ISerializable interface. This is how Vector234, Matrix4 etc work.

I think the ISerializable interface is necessary in this case, as the internalBits field is not public.

[FromSingle]
Yeah, but to use FromSingle you have to call new first anyway.

Half h = new Half();
h.FromSingle(3.5f);
// vs
Half h = new Half(3.5f);

When working with structs, "new" simply calls your constructor (or initializes all fields to default(T) if you don't have one). Assuming correct inlining, the above are exactly the same speed-wise.

[Cast to/from double instead of int]
Feel free to remove these.

[Mono.SIMD]
This should do the trick. According to the devs the cast should be plenty fast. Problem is, we *will* see a slight speed hit on .Net and Mono < 2.2, but I think it is worth it. We'll have to benchmark and see.

Mtah.Vector4 v = new Vector4();
unsafe
{
    Simd.Vector4f v_fast =  *(Simd.Vector4f*)&v;
}
Inertia's picture

#30

[Serialization]
ISerializable was the only thing that made sense to me too. Will take some time though as I have to read about the whole concept to write tests.

[FromSingle]
Agreed. My point is: people like choices. The 'new' is not necessary. Let's simply keep both.

[Cast to/from double instead of int]
Done.

[Mono.SIMD]
Let's discuss this in another topic, it's too much for this one.

[Texture]
We haven't touched any scenario yet where the user wants to use Half Textures. RGBA16f has pretty wide hardware support, so this should certainly be considered.

Half4[] Tex = new Half4[TexWidth*TexHeight];
pin( Tex )
GL.GetTexImage( TextureTarget.Texture2D, 0, PixelFormat.Rgba, PixelType.Half, Tex );

Making the field internal limits this to adding an overload to all functions of this style. I'm really starting to see benefits changing it public again. Most of all, we will have a good laugh when someone uses Half and modifies the field and ponders why his code doesn't work. :D

I believe Half is for advanced users anyways, they should be wise enough to figure out that the warning that the field is not intuitive to edit is there for a reason.