Window Icons

[Cre8or]

When merged this pull request will:

• add the window primitives Set_Icon and Clear_Icon (using glfwSetWindowIcon), allowing windows to be decorated with a custom icon on supporting platforms
• add a generic procedure to convert user-defined image types to the internal Glfw byte array type

Used-defined images are expected to be two-dimensional arrays in format (1 .. Height, 1 .. Width) of 32-bit pixels (such as an array, or record, of 4 bytes). Note that the implementation does not enforce that the pixels are in format RGBA (as per the Glfw documentation). I considered adding a Compile_Time_Error pragma to prevent instantiations of the conversion procedure if the pixel type is not 32 bits, but I am not sure if that would cause portability issues.

As always, looking for thoughts and feedback on this - if anything needs changing, let me know!

[flyx]

It is not ideal to add an interface to GLFW functionality that downgrades that functionality (by supporting only a single image). Even if we consider that full functionality might be added later, OpenGLAda will be stuck with supporting a second deviating interface for backwards compatibility.

As you say yourself, this interface is not very save. Here are a bunch of improvements I'd suggest:

• The data type should be named Image, not Glfw_Icon. The prefix is not needed since we're in the GLFW package, and the original name is GLFWimage, this should be kept to avoid any confusion.
• You can pass the images as array (Integer range <>) of Image_Data to the backend with that type having C convention. This will translate to what C expects.

Concerning the generic interface, I suggest doing this instead:

-- aliased needed for proper C interop
type Pixel_Data is array (Size range <>) of aliased Interfaces.C.unsigned_char;
type Pixel_Data_Ref is access all Byte_Array;

type Image is limited interface;

-- return Positive instead of Interfaces.C.int (what we need eventually) to communicate intent
function Image_Width (Object: Image) return Positive is abstract;
function Image_Height (Object: Image) return Positive is abstract;
function Image_Data (Object : Image) return Pixel_Data_Ref is abstract;

By making Image an interface, we leave its structure to the caller and just communicate what kind of data we require. This does not require internal allocation like your solution does (caller might need to allocate instead if their data representation differs).

The type passed to C would be:

type Image_Data is record
  Width, Height: Interfaces.C.int;
  Pixels: Pixel_Data_Ref;
end record;
pragma Convention(C, Image_Data);

And you'd create an array of Image_Data from an array of access constant Image'Class in the wrapper.

GL.Images could theoretically provide an implementation of Image complete with loading and deallocation. However it currently doesn't depend on GLFW and requiring that would be unfortunate. I would postpone that opportunity until after the general API interface is wrapped.

[Cre8or]

Hmm... I'll admit I hadn't considered using interfaces, and looking at it now it seems like an elegant solution. My only concern is how strict the interface definition should be - for example, whether or not the width/height should be allowed to be 0. What if the caller's image type is not initialised, and they've opted for their primitives to return 0 instead of, say, raising an exception? (Is this a valid concern or am I overthinking it?)

On the topic of C interoperability: my understanding is that the aliased keyword tells the compiler that every element should have a unique address, and apparently this is the norm in C/C++. By extension, I assume that the type array (Integer range <>) of Image_Data, which we'll use to pass multiple icon images to the API, should also use the aliased keyword on Image_Data then, correct?

And yes, best to keep the Glfw/GL inter-dependencies to a minimum (ideally none). If absolutely necessary, we can always just create a new package Glfw.Images for what we're doing here. I agree with all of the other points though, and will adjust the PR accordingly.

[flyx]

My only concern is how strict the interface definition should be - for example, whether or not the width/height should be allowed to be 0.

Well width/height of 0 does not define a valid image. Ideally, our API should formally specify what a correct input looks like. Therefore, excluding 0 is a valid decision. We could even do

function Image_Data (Object : Image) return Pixel_Data_Ref is abstract
  with Post'Class => (Image_Data'Result.all'Length = Object.Width * Object.Height * 4);

(Not entirely sure whether this is correct syntax, I haven't done much Ada 2012). I was hesitant to use newer Ada features previously so OpenGLAda can support non-GNAT compilers, but in reality I have never seen anyone using it with something else than GNAT. Also when I started writing this, GNAT defaulted to Ada 95 and 2005 was a switch that needed to be given. As 2012 is the default now, I see little need to keep 2012 features out.

What if the caller's image type is not initialised, and they've opted for their primitives to return 0 instead

They can't as that is not valid for a function that returns Positive. Mind that these functions must explicitly be overridden so the caller has to either return a valid value or raise an exception. The Positive restriction would specifically be there to disallow the caller returning invalid values.

By extension, I assume that the type array (Integer range <>) of Image_Data, which we'll use to pass multiple icon images to the API, should also use the aliased keyword on Image_Data then, correct?

Yes, good catch.

If absolutely necessary, we can always just create a new package Glfw.Images for what we're doing here.

Yeah, I would put it in GL.Images.Glfw since it can re-use some code from GL.Images.

[Cre8or]

And you'd create an array of Image_Data from an array of access constant Image'Class in the wrapper.

I noticed an issue with this approach.

Glfw expects the caller to pass an array of icons (even if that array only contains one icon). However, since we opted to declare Image as an interface, we can't declare an array type for it. We also can't declare an array of Image'Class as that would be an unconstrained element type. So how would we let our callers do this in Ada? Even if they define the array type on their side, Set_Window won't be compatible with it. Calling the primitive once per icon candidate is also not an option, as the API will simply stick with the most recent one. Perhaps if we defined an array of Image_Data? This would, however, make the Image interface obsolete, and expose the raw API format, which is far from ideal. Hmm...

Yeah, I would put it in GL.Images.Glfw since it can re-use some code from GL.Images.

Hold on, I'm not sure I follow. I thought we didn't want to add a GL-Glfw dependency to the binding?

[flyx]

Glfw expects the caller to pass an array of icons

See the Image_Data type above. I was a bit careless and gave it the same name as the Image_Data function. This would be a private type not exposed to the API where you paste the return values of the Image function into.

Thinking about it, this is actually over-engineered. Instead of implementing three functions, it would be far simpler for the caller to provide a

type Image is record
   Width, Height : Positive;
   Data : Pixel_Data_Ref;
end record;

There would then be the private type Image_Data shown above and the wrapper implementation should copy the given Images into an array of Image_Data while converting the Width and Height values.

This would still be an interface to the image data (we don't assume Image owns the data) and the caller can assign the three field values just like they can calculate them in three overridden functions. It would also be quite simple to provide an owner type in GL.Images from which such an Image can be produced by the caller, without the need of dependencies between the two packages.

About your question about GL.Images: GL.Images.Data and GL.Images can be in different projects, thus the project opengl-images would not need to depend on opengl-glfw.

[Cre8or]

I reworked the wrapper according to your feedback. The caller now has to fill out one (or more) object(s) of type Image, which can be passed to the Set_Icon primitive as either a single object, or an array. The version with just one icon is shorthand for applications that only wish to supply one icon candidate, and is designed for ease of use (internally it gets wrapped into an array). This is not a necessary addition though, and if you prefer I can remove it again.

Using this new implementation, I have reworked my application to convert the data, which was relatively straight forward. The only problem I encountered was the access conversion to the Pixel_Array_Ref type, as I had to make use of the Unrestricted_Access attribute (which as I understand is inherently unsafe?). But certainly easier than having to use an interface.

Thoughts?

[flyx]

The one-item Set_Icon is fine. Can you make Images of the imported function be Image_Data_Array since that's possible and safer than declaring it to be an address?

The only problem I encountered was the access conversion to the Pixel_Array_Ref type, as I had to make use of the Unrestricted_Access attribute (which as I understand is inherently unsafe?)

Ah yes, that's unfortunate. Ada does not want to give us the possibility to store the pointer to exceed the lifetime of the thing it points to. I am not entirely sure how to make the compiler happy, but try one of these:

• Make Image limited. This prevents copying it inside of Set_Icon and may convince the compiler we don't copy the pointer.
• Make the Data field have an anonymous access type (Data: access all Byte_Array). That may cause the compiler to produce a runtime check instead of outright saying it's illegal.
• Make Data be a discriminant, like in those fancy reference types in Ada 2012's Ada.Containers packages. It works there although I don't know enough details to be sure what's the relevant part of the language spec for that.

Sorry to poke in the dark here, I'd test it myself but still can't compile Ada on my M1 :/

[Cre8or]

Can you make Images of the imported function be Image_Data_Array since that's possible and safer than declaring it to be an address?

Well, this would require the API to know about Image_Data, and, by extension. Pixel_Data_Ref (and Pixel_Data). Moving these types over to Glfw.API means the caller no longer sees Pixel_Data, so they can't convert their images to the required type. Alternatively, leaving them inside Glfw.Windows, we might be tempted to with that unit into Glfw.API, but that would lead to a circular dependency, and all sorts of troubles. I concede that using 'Address is not ideal, but I can't think of a better way at the moment - well, short of introducing a separate pacakge specifically to declare all image-relevant types. Would that be an acceptable solution?

Ada does not want to give us the possibility to store the pointer to exceed the lifetime of the thing it points to

True. It's a good safety feature, but I suspect that I can knowingly ignore it in my case, as I know that:

• my images array is copied by Glfw, so it doesn't need to exist beyond the call to Set_Icon (thus I am free to discard it afterwards),
• I only need a different access to the underlying data for the purpose of calling Glfw.Set_Icon, without tampering with the data itself,
• the access type conversion is done by a unit-private function, making it harder to inadvertedly (re)use it elsewhere.

This way, my own icon conversion boils down to just:

Glfw_Icons( Index ) := ( -- Glfw_Icons is an Image_Array
    Width  => Positive( Icon.Get_Width ),
    Height => Positive( Icon.Get_Height ),
    Pixels => To_Glfw_Pixels( Icon.Get_Data_RGBA.all ) -- Converts the icon's 2D array of RGBA pixels to a Pixel_Array_Ref.
                                                       -- This is the "unsafe" part, as it maps the resulting access onto
                               -- the icon's internal data using 'Unrestricted_Access.
);

[flyx]

Well, this would require the API to know about Image_Data, and, by extension. Pixel_Data_Ref (and Pixel_Data). Moving these types over to Glfw.API means the caller no longer sees Pixel_Data, so they can't convert their images to the required type. Alternatively, leaving them inside Glfw.Windows, we might be tempted to with that unit into Glfw.API, but that would lead to a circular dependency, and all sorts of troubles.

That's no circular dependency; package specification and implementation are separate entities. This would give us the graph:

         +----------- Glfw.API <-----------+
         v                                 |
Glfw.Windows (spec) <--------- Glfw.Windows (impl)

GL.API already does that extensively.

Speaking of GL, I think it would be best to put the array type inside GL.Types (as UByte_Array), it already defines the array types for most other C types so it would make sense to just put it there. Concerning the _Ref type, I am not entirely sure whether it's valid to use anonymous access in both Image and Image_Data but I'd try it in the hope that this also fixes the 'Access problem. Also an anonymous type does not impose the possibly necessary conversion to the explicit _Ref type on the caller if they happen to use a different type for their pixel array.

I am aware that technically it's safe for the caller to use 'Unrestricted_Access, but it'd be better for the API to not impose this consideration on the caller, but making it possible to use 'Access.

[Cre8or]

Ah, you're right - I forgot that unit bodies are compiled after their specs. No circular dependencies here, as you stated.

Concerning the _Ref type, I am not entirely sure whether it's valid to use anonymous access in both Image and Image_Data but I'd try it in the hope that this also fixes the 'Access problem.

From a quick test: yes, it is valid to use in both types, but that does not solve the 'Access issue described above. The compiler rightfully warns us that a "non-local pointer cannot point to local object". From what I can see, using an anonymous access type for the pixel data does not give us any benefits here. But perhaps I'm missing something?

Also an anonymous type does not impose the possibly necessary conversion to the explicit _Ref type on the caller if they happen to use a different type for their pixel array.

I'm starting to think that I am missing something obvious, because I can't get this to work without a conversion of the access type (using 'Unrestricted_Access). How would I go about doing this in a safe(r) way?

[flyx]

From what I can see, using an anonymous access type for the pixel data does not give us any benefits here. But perhaps I'm missing something?

If we don't allocate the pixel data ourselves, we simply do not need the named access type. From my experience, named access types are typically used when data is owned, and that is not the case here so there's no need for it and it might give a reader the wrong idea.

If it does not solve the 'Access problem, bummer.

I'm starting to think that I am missing something obvious, because I can't get this to work without a conversion of the access type (using 'Unrestricted_Access).

Nah I too am very unsure about what exactly would be required, and am just throwing ideas that might or might not work. I will do some proper research into whether this is actually possible, and report back.

[flyx]

I'm afraid I have decided to stop maintaining OpenGLAda. This is my only active Ada project and I am not using it for anything, hence keeping up with the language and ecosystem is taking up too much of my time and I lost motivation.

I am very sorry for this. You can keep using your own fork, there won't be any upstream development happening anymore.

[Cre8or]

Sad to hear it, but all the same glad that you took the time to notify me. I understand your reasoning, the same has happened to me before - best to focus on what's important instead.

Regardless, thank you so much for making OpenGLAda! I would definitely not be as invested in the language if it weren't for the ability to make "practical" personal projects (using this project, among others). I know I would not have learned as much from just professional work alone. And from our previous interactions, it seems I still have a lot more to learn. 😛

I want to return the favour by publishing some of the Ada libraries that I wrote (and use), but I first need to proof-read them and make sure they're of high enough quality for public scrutiny. While I suspect they may not be useful to you, I hope that the Ada community will be able to benefit from them, just like I did from yours.

[rogermc2]

Similarly, many thanks Felix for all the effort that you have put into developing OpenGL.

[flyx]

You're welcome, both of you, and thanks for your contributions!