Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated library to import sdl_gfx #75

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Conversation

Kyjor
Copy link
Contributor

@Kyjor Kyjor commented Jan 26, 2024

Here is a fix for the sdl_gfx functions not actually working when called. The package SDL2_gfx_jll wasn't listed as a dependency, and I updated the calls in LibSDL2 to actually use them. Eventually I would like to update to a newer version of the package, but this is what we have at the moment.

Copy link

@JamesWrigley JamesWrigley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not involved with the project at all but I took the liberty of doing an unofficial review 😛 It'd be best if the bindings were generated completely automatically by the code in gen/, that way it's easier to update them.

Comment on lines +15 to +16
using SDL2_gfx_jll
export SDL2_gfx_jll

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -6267,411 +6270,411 @@ struct FPSmanager
end

function SDL_initFramerate(manager)
ccall((:SDL_initFramerate, libsdl2), Cvoid, (Ptr{FPSmanager},), manager)
ccall((:SDL_initFramerate, libsdl2_gfx), Cvoid, (Ptr{FPSmanager},), manager)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all the libsdl2_gfx functions come from a specific header file? If so then we could set the library to be called for that header: https://github.com/JuliaInterop/Clang.jl/blob/master/gen/generator.toml#L12

@Kyjor
Copy link
Contributor Author

Kyjor commented Jan 28, 2024

No that's fine, thank you! When I get a chance either today or tomorrow, I can address these issues. This is super helpful!

src/LibSDL2.jl Outdated
end

function TTF_RenderGlyph_Solid(font, ch, fg)
ccall((:TTF_RenderGlyph_Solid, libsdl2_ttf), Ptr{SDL_Surface}, (Ptr{TTF_Font}, Uint16, SDL_Color), font, ch, fg)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JamesWrigley I updated the config, and it seems to be overwriting all of these. I'm not sure if I'm doing something wrong here, or if I should be going back and updating these. I figured I'd commit it and show instead of trying to explain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot reproduce this 🤔 If I clone the repo and regenerate the bindings those lines aren't changed. What Clang.jl version are you using? I know gen/Project.toml has compat bounds but I'd actually suggest deleting all of them and running ] up. Generator projects are typically used to generate bindings for the latest version of packages and they'll never be a dependency, so I don't see any point in having compat bounds for them.

src/LibSDL2.jl Outdated
@@ -7098,6 +7171,10 @@ const SDL_MUTEX_TIMEDOUT = 1

const SDL_MUTEX_MAXWAIT = ~(Uint32(0))

const SDL_beginthread = _beginthreadex
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws an error saying it doesn't exist. I'm not sure if I'm supposed to just delete it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not show up when I generate the bindings 😅 Maybe because we're using different package versions? I've updated everything.

@@ -219,7 +262,7 @@ mutable struct SDL_BlitMap end

struct SDL_Surface
flags::Uint32
format::Ptr{Cvoid} # format::Ptr{SDL_PixelFormat}
format::Ptr{SDL_PixelFormat}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this stay a Cvoid?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's ok, Clang.jl probably just didn't have full support for pointer fields before. You can see from the comment that it did recognize the type of the field but didn't use it for some reason.

@JamesWrigley
Copy link

I tried your branch locally and got it working, the only changes I made were:

  • Deleting the compat bounds from gen/Project.toml and updating all the dependencies
  • Blacklisting SDL_BYTEORDER and SDL_FLOATWORDORDER, those depend on some other property that isn't captured for some reason.

After that, all the tests passed.

@Kyjor
Copy link
Contributor Author

Kyjor commented Jan 31, 2024

Tried it with a linux machine instead of windows and it worked! I want to add a few smoke tests for the added libraries. Thank you! Definitely learned some useful stuff about Clang.jl that I can use for my other stuff 😄

@Kyjor
Copy link
Contributor Author

Kyjor commented Jan 31, 2024

Okay I actually have it working and added a simple test for each header file from the gfx library. I think it's ready for an official review. If you happen to know anyone who can, would you direct them here please? Thanks!

@JamesWrigley
Copy link

Noice noice 👍 Maybe @Gnimuc?

@Gnimuc
Copy link
Member

Gnimuc commented Feb 1, 2024

I think it's good to merge.

@Kyjor
Copy link
Contributor Author

Kyjor commented Feb 8, 2024

Anything else I need to do here? Is there an ETA on merging? I'd like to use this soon. Thanks!

@Gnimuc Gnimuc merged commit a711a37 into JuliaMultimedia:master Feb 8, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants