NotNotTech / Raylib-CsLo

autogen bindings to Raylib 4.x and convenience wrappers on top. Requires use of `unsafe`
Mozilla Public License 2.0
115 stars 10 forks source link

Codegen #13

Closed IrishBruse closed 2 years ago

IrishBruse commented 2 years ago

Again just a wip pull request for you to give feedback.

Could you force push my pr as i have cleaned up the commit log

jasonswearingen commented 2 years ago

hi I'll look at this tonight!

jasonswearingen commented 2 years ago

do you have access to Visual Studio? running the Codegen via VS fails due to different working directory at CodeGen/Program.cs line 44: using (JsonDocument document = JsonDocument.Parse(File.ReadAllText(ApiJsonFile)))

There are a few ways of fixing this, I'm okay with any way, as long as running from Visual Studio would be supported.

IrishBruse commented 2 years ago

havent tested it with visual studio will do when done

jasonswearingen commented 2 years ago

Since the raylib_api.json doesn't include extras such as Physac or RayGui We will still need to use the ClangSharp generator. So care needs to be taken to ensure that the generated Raylib bindings are still compatible with the clangsharp generated code.

IrishBruse commented 2 years ago

image

IrishBruse commented 2 years ago

just not pushed them yet

IrishBruse commented 2 years ago

i compiled them with the raylib parser they arent in the repo by default

jasonswearingen commented 2 years ago

just not pushed them yet

oh okay, did you generate those yourself with the raylib parser? neat that it's available.

Also, I took a look at some of the generated code under autogen/wrappers and it looks good.

Regarding naming it RaylibS, are you planning on try to make it "Safe" as in no pointers are needed?

IrishBruse commented 2 years ago

yup thats the plan

IrishBruse commented 2 years ago

you shouldnt need to have the unsafe keyword is my goal

IrishBruse commented 2 years ago

also all tests pass with my native bindings

jasonswearingen commented 2 years ago

I will make a new Codegen branch and give you commit/member permissions? I think that would be better for feedback/R&D than putting it into main branch pr's. What do you think?

IrishBruse commented 2 years ago

Just clone my branch if you want to test it out git clone https://github.com/IrishBruse/Raylib-CsLo.git --branch safe-codegen --recursive

jasonswearingen commented 2 years ago

Just clone my branch if you want to test it out

Yep that's what I did! Sorry I am not actually very familiar with github collab, so it took a while to figure that out!

Okay I think that the handling of native structs like Model is going to be the tricky part, if you have a plan already in place for that could you describe? (or if there's already code for it, plz point me to it)

IrishBruse commented 2 years ago

Ah no bother collab is weird. For something like the Model class if its to complex to write an auto gen for its probably way faster and easier to write our own version of it in fact thats the plan for a handful of the much more complex raylib functions ill be reusing some of the functions you added it auto gens the class as a partial so i just plan on implementing the functions that are set to be skipped in the codegenSettings.cs. Model will probably be a similar thing its native stuff can be codegened and then we can implement safe methods and the sort for it.

IrishBruse commented 2 years ago

So how did you do the examples were they hand written and should we update them to use the safe apis? Im nearly done with the safe code generator for the core raylib_api.json.

jasonswearingen commented 2 years ago

I did all the examples manually, which took a long time (probably 20 or 30 hrs). I think porting all of them to the safe api's is going to be even more work, as I was able to do a lot of regex-based search-replace to assist me in the initial port and I think that's not going to be as useful with the safe api's, as the safe api's will require more changes to the logic and workflow.

I think a better solution would be to just port a few "showcase" examples for each catagory. I can help do that once your initial codegen work pass is complete.

jasonswearingen commented 2 years ago

please ping me when this is ready to be tested! I have been stuck in nodejs land the last couple weeks so look forward to checking out your awesome codegen !!!

IrishBruse commented 2 years ago

Last night I pretty much finished up replacing the old codegen I'm just running through fixing up any of the broken examples now and adding more safe wrappers around all the functions.

IrishBruse commented 2 years ago

@jasonswearingen So that's everything replaced if you want to take a look at it before I go and add more safe functions to it most the examples work plus I updated the Readme a bit and the one in examples.

jasonswearingen commented 2 years ago

will look today and reply in a few hrs!

jasonswearingen commented 2 years ago

So I took a look, and think I know what at least some of the crash problems are: it's because of managed (C#) / unmanaged (CPP) memory conversions, and what the raylib native dll expects.

For example, FirstPersonMaze.cs

The line

Color[] mapPixels = LoadImageColors(imMap);

"works" fine, but there is native memory (Color*) that was allocated internally (the (Raylib.LoadImageColors(image) call), the LoadImageColors() coppies this Color* data into a managed Color[] object. This is a problem because that native pointer is "lost", causing a memory leak (but not crash)

The crash occurs at the line

 UnloadImageColors(mapPixels);   // Unload color array

because internally, it is passing the managed Color[] object to the Native Raylib.dll to be freed. This is a big no-no because the Color[] is managed memory, coppied data from the original native Color* created in LoadImageColors().

You don't actually have to clean up the managed Color[] object, the GC will handle that normally/automatically. What has to happen is the native Color* needs to be properly disposed to prevent a memory leak. This is actually easy to do in this scenario, we can simply change the LoadImageColors() method to be:

    public static Color[] LoadImageColors(Image image)
    {
        var nativeColorArray = Raylib.LoadImageColors(image);
        var toReturn = Helpers.PrtToArray(nativeColorArray, image.width * image.height);
        Raylib.UnloadImageColors(nativeColorArray);
        return toReturn;
    }

and comment the //UnloadImageColors() line in FirstPersonMaze.cs line 132.

That will work and remove the memory leak, however there are some concerns this brings up:

let me know what your thoughts about the above are, and we can figure out how to proceed.

IrishBruse commented 2 years ago

Automatically converting the Color to a managed object works in this situation, but is the Color ever needed? Probably not, and if so, the dev could always use the "unsafe" api.

I mean it works now best to have the function than not plus it definitely is needed of you want to do stuff to images on the cpu side.

Helpers.PrtToArray() is misleading, it should be renamed to 'CopyPtrToArray` as it's not casting a reference, but rather copying data into a new managed object

Good Idea fixed that.

This managed vs unmanaged memory is sprinkled all over Raylib 3d workflows, so there are going to be many situations where this or more complex marshalling issues need to be considered

Not sure what else would need this really other than array functions and i dont think there are many of these so we can probably manually implement them unless im missing alot of them.

jasonswearingen commented 2 years ago

fyi there is some break due to raylib.dll missing in the examples project.

a quick glance through things and it looks like you have prebuilt win-x86 binaries, but two problems with this:

output folder For nuget packages, this is automagically handled via the nuget resolver. But for some dumb reason .csproj doesn't include the same binary resolve system. For source based (project) workflows, I think the best solution would be to copy both linux and windows x64 to the output folder. Similar to what you already did with the linux raylib.so

**x86 vs x64 Now days C# runs the build arch native to the OS. as modern platforms are x64 that means x64 is the default, and mixed-arch processes are forbidden thus loading the x86 version of raylib.dll will cause an error. I see you mention that you got raygui x86 from 'manjaro'. If you can get x64 versions from him and checkin that would be best. However if you need, I think I can build x64 versions, as I've figured that out in the past. I'm not sure I know how to seperate raygui from raylib though. Is that a strict requirement? rolling them up into a single unified library is how I did that in the past.

, can I fix this and commit back somehow? Not sure how you would prefer.

IrishBruse commented 2 years ago

thoses binaries are all from you i did yesturday just rebuild the raylib dll with all the extras ill commit that now

IrishBruse commented 2 years ago

@jasonswearingen Manjaro is a linux distro I have it on my laptop and tried it out ill need to recompile the lib again tho to add the extras

jasonswearingen commented 2 years ago

sounds great, I will check this out again in a few hours, family stuff keeping me busy at the moment.

IrishBruse commented 2 years ago

Been busy with college and updating one of my libs recently saw you doing some updates 👍🏻

jasonswearingen commented 2 years ago

Been busy with college and updating one of my libs recently saw you doing some updates 👍🏻

yep, please see https://github.com/NotNotTech/Raylib-CsLo/issues/12#issuecomment-1089250868 there is a design decision that needs to be made.

IrishBruse commented 2 years ago

Raymath doesn't really need to be ported as most the stuff in it is apart of the vector classes and math if you are using csharp it makes more sense to use the built in methods apart of System.Numerics.Vector2 etc and the easings is converted to cs i convert the .h file in the generator class for it. The reason for the matrix thing is because of using system.numerics.matrix but id imagine the best solution would be transposing them in the safe codegen would be easy to detect a matrix being passed and just transpose it if you show me an example of what need to be done i could add it to the codegen. The pointer stuff is probably a bug will look into that think c# is just fine with casting it for what ever reason.