luca-piccioni / OpenGL.Net

Modern OpenGL bindings for C#.
MIT License
570 stars 109 forks source link

Facilitate porting code based on other libraries #62

Open luca-piccioni opened 7 years ago

luca-piccioni commented 7 years ago

Introduction

This issue is created to collect various ideas about how to facilitate the porting of other code written for OpenTK, Mono and other libraries. It could be a common case to have an existing source code, and for running with OpenGL.Net it necessarily need to be modified.

This does not mean that OpenGL.Net can/should/will adopt all code conventions and API of the other libraries. This is only a point that may be sensible to the users of this library, and this is the place for discuss about it. In short, this is a chance that may apply (why not, even radical) changes.

Everyone is allowed to edit this issue by adding anything not listed below. If any user wants to discuss about the particular points, can write a reply and update it as necessary; i'll do the same.

[1] Change Gl to GL

This is most invasive, stupid but still easy to adapt change. OpenTK and Mono uses the uppercase version, instead other libraries have the camelcase convention like OpenGL.Net.

This can be work-rounded by putting an using GL = OpenGL.Gl; at the beginning of the source files. This seems the most fair trade-off.

[2] Provide methods with explicit "counter" parameters

OpenGL.Net automatically consider a "counter" parameter implicitly defined by the length of another array argument. This obviously deviates from the "raw" OpenGL API, but provide more succinct signatures and less error prone implementation. It should be safe to add "raw" method signatures.

[3] Provide methods with unsafe parameters

OpenGL.Net source code generator support the creation of methods overloads with unsafe parameters. I usually minimized the generation of this methods since the IntPtr overloads were sufficient to develop my application. Currently the exceptions are the method matching the regex ^gl(Program)?Uniform(Matrix)?(1|2|3|4|2x3|2x4|3x2|3x4|4x2|4x3)(d|f|i|ui)v$, but sincerely I could remove even them.

[4] Provide methods with ref parameters

OpenGL.Net treat pointer arguments with IntPtr and object types (with few exception on String). However, the pointer argument can be marshaled with the ref modifier, telling to the default P/Invoke marshal code to pin a pointer to the argument (which need to be a generic type, constrained to be struct).

luca-piccioni commented 7 years ago

Here is my thoughts about the issue.

Change Gl to GL

This has always disturbed me. I don't remember why I've chosen the camel case convention for the project classes (it happened too many years ago). Normally acronyms are spelled with upper case tokens, and I think this should be the case.

To adapt current existing code should be sufficient a Find & Replace Gl./GL. in all files.

Provide methods with explicit "counter" parameters

I'm not a fan about this modification. While it allows user to specify only part of the array, users can introduce errors due a counter parameter greater than length of the array. For example:

uint[] buffers = new uint[4];

Gl.GenBuffers(buffers);           // It works, and user cannot be wrong
Gl.DeleteBuffers(buffers);        // It works, and user cannot be wrong

Gl.GenBuffers(1, buffers);        // It works, and user can store only 1 buffer
Gl.DeleteBuffers(1, buffers);     // It works, and user can specify only 1 buffer

Gl.GenBuffers(5, buffers);        // Memory corrupted and resource leak
Gl.DeleteBuffers(5, buffers);     // Memory corrupted and probably un-managed exception

Provide methods with unsafe parameters

I doubt that this really required for many users. If the GL command takes a pointer, it is typed as IntPtr, which can be constructed from an unsafe parameter. Additionally, a bunch of methods will be added, increasing the size of the binary image (however, this should be measured, since it could be negligible).

Provide methods with ref parameters

....

21c-HK commented 7 years ago

[1] Yes. Acronym should be upper case only. [2] No, because of the reasons/examples @luca-piccioni stated/provided. [3] No, because of the reasons @luca-piccioni stated. [4] Could you provide concrete pros/cons?

luca-piccioni commented 7 years ago

Provide methods with ref parameters

I found the emblematic use case. P/Invoke default marshaller can pass arguments by value, for free for basic system types, including IntPtr. The IntPtr arguments are normally used to pass array of blittable types (int, short, float), and the delegate is used for the object arguments (which are pinned during call).

As noted by in #60, the object arguments can cause boxing of value types, causing an extra cost of the API. Indeed the following instruction is not efficient: v would be boxed to object (Vertex4f is a blittable structure).

Vertex4f v = new Vertex4f(0.0f);
Gl.BufferData(BufferTarget.ArrayBuffer, 16, v, BufferUsage.StaticDraw)

Instead, modifying the signature with a ref, it would more efficient:

Vertex4f v = new Vertex4f(0.0f);
Gl.BufferData(BufferTarget.ArrayBuffer, 16, ref v, BufferUsage.StaticDraw);

Without the ref override, it is possible to workaround with

void SetUniform(Vertex4f v)
{
    unsafe {
        Gl.Uniform4(1, 1, (float*)&v);
    }
}

The signature of the functions must have a template parameter (i.e. Gl.Uniform4<T>(int, int, ref T)).

mellinoe commented 6 years ago

As someone starting to rewrite the OpenGL backend in my 3D graphics library, I guess I'll give some feedback here.

[1] Change Gl to GL

I have a couple of suggestions here:

  1. If the choice is between Gl and GL, then I slightly prefer GL. Gl has the unfortunate property of looking like G-capital-i, rather than G-capital-L (if that made sense 😄).

  2. I've begun to prefer a native binding approach where the native functions are exposed with the exact same name as they are truly defined with. Then, a "using static" can be added, and the functions can be called in a natural manner. This is a more radical departure from what you have, so I'll leave the suggestion at that.

[2] Provide methods with explicit "counter" parameters

This is necessary, IMO. This ties into some other feedback I have, but suffice to say, I think it's 100% needed.

[3] Provide methods with unsafe parameters

This doesn't do much for me, personally. If there are IntPtr overloads, then it is enough. Unfortunately, it doesn't really look like there are IntPtr overloads for everything that there needs to be. For example, glGenBuffers only has two wrapped overloads:

GenBuffers(uint[]) GenBuffer()

Unless I'm missing something, there's no way to call glGenBuffers without allocating an array. That makes the API very cumbersome -- I have to manage extra temporary arrays that might be useless otherwise. At the very least, the GenBuffer overload shouldn't allocate a temporary 1-element array. It should just create a local and pass its address to the native function.

This could be avoided if all of the functions had at least one raw overload exposed.

[4] Provide methods with ref parameters

I was the one who #60 , so obviously I would love if these overloads were added. out overloads for "generate" functions would also be nice, e.g.:

glGenBuffers(uint count, out uint buffers)

That's my feedback for the 4 immediate topics. I'll probably type something up a bit more general as well.

mellinoe commented 6 years ago

General feedback:

I actually have just a very small set of requirements, so I might not be a typical user of the library. At the moment, I am planning to just build a very minimal wrapper library myself, for use in my graphics library internally. I'm just going to wrap the stuff that I use, and not make an attempt to wrap everything. I think MonoGame is doing something similar ATM.

Stuff that I'm looking for in bindings:

Apologies if this is the wrong channel for feedback. As an interested party, this seemed like an okay place to comment.

luca-piccioni commented 6 years ago

@mellinoe

If you have any other critic/doubt/question about, you are free to open new issues as you wish: this makes issue tracking easier.

mellinoe commented 6 years ago

The two changes above look great. As for "close to native", I guess all that means for me is that I'm not interested in the higher-level object-oriented stuff (OpenGL.Net.Objects). But that's not to say it wouldn't be useful for someone else -- just trying to clarify what I'd like from the library.

If you have any other critic/doubt/question about, you are free to open new issues as you wish: this makes issue tracking easier.

Will do. Thanks for the discussion!