search

ImGuiNET / ImGui.NET

An ImGui wrapper for .NET.
MIT License
1.85k stars 300 forks source link

Thoughts on safe equivalents? #124

Closed giawa closed 5 years ago

giawa commented 5 years ago

I noticed issue #23 and wanted to open an issue before I spend much time on this. I prefer to use safe methods where possible. Immediately I ran into:

public void GetTexDataAsRGBA32(out byte* out_pixels

which is part of ImFontAtlas.gen.cs and accepts a pointer to a byte array. This can be made safe by using an array of bytes instead (and using Marshal.Copy). I was thinking of going through all of the ImFontAtlas.gen.cs file and adding safe versions of these methods. Is this worthwhile? I have a local version tested with GetTexDataAsRGBA32(out byte[] working just fine, so it works in theory.

If I were to make this pull request, I imagine I would make a new file ImFontAtlas.Manual.cs in the ImGui.NET project? Let me know what you think, and thanks for your consideration.

giawa commented 5 years ago

I've put an example of how it might be done here: https://github.com/giawa/ImGui.NET/commit/74b8698c46437ef062809e04d8863c0ea61c3fd9

giawa commented 5 years ago

Another possibility would be simply having the method use IntPtr instead of byte (would need to make this an override for backwards compatibility I imagine). This actually makes the method RecreateFontDeviceTexture in the sample code safe as well, since the conversion from a byte to IntPtr is no longer required. IntPtr would work on my end as well, and avoid the Marshal completely.

giawa commented 5 years ago

My understanding is that right now ImgGui.NET uses .net standard 2.0. I don't think Span was introduced until 2.1 (I could be wrong here). Also, if you do need to get an IntPtr from a Span it looks like you need to allocate and copy anyways. I think I actually prefer the IntPtr approach because you can then do whatever you would like with it, while avoiding unsafe code.

mellinoe commented 5 years ago

Another possibility would be simply having the method use IntPtr instead of byte (would need to make this an override for backwards compatibility I imagine). This actually makes the method RecreateFontDeviceTexture in the sample code safe as well, since the conversion from a byte to IntPtr is no longer required. IntPtr would work on my end as well, and avoid the Marshal completely.

Personally, this is the version that I'd find most useful, but it's also really trivial. For something like this, I'm also not 100% sure it's worth maintaining manual versions of methods, because they are called so infrequently (usually just in one place in the entire program). I think the manual helpers are really only worth the cost when you need to call them a lot.

That said, I would like it if the code generator were able to generate "variant" overloads with different marshaling behavior. E.g. a byte* parameter would have another overload with that parameter declared as an IntPtr. ref/out could also be supported, etc. In other words, I wish that part of this could be done by the code generator itself so that it wasn't a long-term maintenance cost.

giawa commented 5 years ago

That said, I would like it if the code generator were able to generate "variant" overloads with different marshaling behavior. E.g. a byte* parameter would have another overload with that parameter declared as an IntPtr. ref/out could also be supported, etc. In other words, I wish that part of this could be done by the code generator itself so that it wasn't a long-term maintenance cost.

Okay, I've taken a stab at something like that here: https://github.com/giawa/ImGui.NET/tree/feature/type-variants

This feature branch adds the concept of a 'variant' per type, which can be optionally defined in the definitions.json file. For example:

        {
          "name": "out_pixels",
          "type": "unsigned char**",
          "variants": [ "IntPtr*" ]
        },

will create an OverloadDefinition for both an unsigned char** and IntPtr*. The OverloadDefinitions are expanded at FunctionDefinition creation. You can assign multiple variants per type, as well as have variants for multiple types. The OverloadDefinition array should be expanded to include one of each possible arrangement of types (which can grow quickly if you have lots of variants, but I don't imagine this will be common).

The first commit adds the variant feature, and the second commit modifies the definitions.json file to include the IntPtr* example that will give the behaviour discussed above (returning an IntPtr instead of a byte* for the ImFontAtlas methods). Running it from the sample code, it appears to work well. If you'd like, I can make a third commit that modifies the sample project to use a safe call to GetTexDataAsRGBA32. I can also break each of these commits up into separate pull requests if you would rather (or backtrack and go about this a different way if this doesn't align with your goals).

Let me know what you think, and if this is moving in the direction you were hoping.

mellinoe commented 5 years ago

@giawa That is fantastic, thanks for taking the time to put that together. It's very close to what I was imagining. A few notes from looking it over:

giawa commented 5 years ago

@mellinoe Thanks for the feedback!

We won't want to modify the definitions.json file, because that's an artifact that we pull over directly from the cimgui repo.

Oops, I was wondering where that file came from! Okay, I've made a new feature branch here: https://github.com/giawa/ImGui.NET/tree/feature/type-variants-sep-json

This new version uses a variants.json file to apply to a method based on method name, parameter name and parameter type. It doesn't emit any errors if it can't find a match, so that might be something worth doing. Let me know if this looks better.

I'm not sure that generating the extra variants in ImGuiNative is very useful.

I agree, but it gets complicated. I think I would have to know how to convert between types. There might be some marshaling or some .NET feature I am unaware of that can help with this process. If you can think of something that would make this simpler and can point me down the right path then I can give it a go. Right now this new feature branch still creates a new pinvoke import to match the variant.

It would be good to use one of the new variants (like GetTexDataAsRGBA32 as you mentioned) to validate that it's working properly.

The new feature branch includes a third commit that modifies the SampleProgram to use one of the newly generated methods. There are 3 commits here, any or all of them can be included in a PR if you like this addition:

1) https://github.com/giawa/ImGui.NET/commit/cd5c0269b0df5d6453ca02487103606ae7db1776 Adds the actual variants feature, but does not include a variants.json (will still compile+run since it checks for the file first) 2) https://github.com/giawa/ImGui.NET/commit/5afed427add26ffa681f54dcbe4540e7b0bc826f Adds a variants.json file with variants for the ImFontAtlas_GetTexDataAsAlpha8 and ImFontAtlas_GetTexDataAsRGBA32 methods 3) https://github.com/giawa/ImGui.NET/commit/8be7644c3e50db87e1a7a6be9602eb12d438e969 Modifies the SampleProgram to use the newly generated variant method, and removes the unsafe keyword for that method.

giawa commented 5 years ago

I modified the code a bit to keep track of which variants were applied. Then an error message can be displayed for any variants whose method/name/original type did not match the definition from definitions.json. Let me know what you think of something like this:

https://github.com/giawa/ImGui.NET/commit/d62cb5f27822e55ae12badbc68a8f26bbaa59ddd

giawa commented 5 years ago

Checking in to see if there's any other work I can do on this.

mellinoe commented 5 years ago

@giawa No, that work looks great. I went ahead and applied your changes to the master branch. I also merged in the 1.71 update, so there should be a new version going out with everything shortly.

giawa commented 5 years ago

Awesome, I replaced my custom version with the latest 1.71 update in my personal projects and it works well! Thanks