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

Adds support for sdl_image and sdl_ttf (also other libs) #179

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

WizzardMaker
Copy link

I needed the new versions of SDL3_image and SDL3_ttf, and I liked the implementation of this library for SDL3.
I've refactored the bindings script to be able to use different external libraries.

For now only image and ttf are added, as they are the only ones I need, but the code change allows for easy addition of the other SDL libs.

@smoogipoo
Copy link
Contributor

I think we should separate the csprojs, so we'd have the current SDL3-CS one and add SDL3-CS.TTF, etc. Just to reduce the size of native dependencies for consuming projects (for example osu! will likely never use TTF).

@WizzardMaker
Copy link
Author

I think we should separate the csprojs, so we'd have the current SDL3-CS one and add SDL3-CS.TTF, etc. Just to reduce the size of native dependencies for consuming projects (for example osu! will likely never use TTF).

Technically, dotnet will only lazy load native dependencies when those functions are first called. So without calling any SDL3.TTF_* no ttf dll would be required.
But I do see the benefits of separations of concerns here. It would also make it easier for developers when _ttf or _image related functions are in their own space.
But I think separating them into different csproj would be a bit overkill - shouldn't different classes (e.g. SDL3_ttf) be sufficient?

@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 3, 2024

I meant the inclusion of the native libs, because we're also going to have to build + package them here in addition to the bindings. The native SDL2_ttf.dll (yes I know this isn't SDL3, just using it as a reference point) for instance is a 1.8MB dependency, which will be included in the build output regardless of whether or not it's used.

@WizzardMaker
Copy link
Author

WizzardMaker commented Dec 3, 2024

Ah, yes of course. I haven't thought about the nuget package implications

I can split it up into their respective csproj with PackageReferences to the SDL3-CS package.
How should the .Android variant be handled?

@WizzardMaker WizzardMaker marked this pull request as draft December 3, 2024 11:28
@smoogipoo
Copy link
Contributor

smoogipoo commented Dec 3, 2024

For Android, I'm tempted to multi target and require the workflow for build. Including for (and removing) the existing package (@Susko3 would you agree with that?).

@Susko3
Copy link
Member

Susko3 commented Dec 3, 2024

I agree with multi targetting Android. The android workload probably won't be required if running the dotnet build command for a specific RID (--runtime), so that's not really an issue.

Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

I've not checked the python code and the sourcegen output. Both checks are needed before merge.

.gitmodules Outdated Show resolved Hide resolved
SDL3-CS/SDL3_image/ClangSharp/SDL_image.g.cs Outdated Show resolved Hide resolved
SDL3-CS/SDL3_ttf/ClangSharp/SDL_ttf.g.cs Outdated Show resolved Hide resolved
@WizzardMaker
Copy link
Author

I've now split up the projects into their respective libraries.
The native build script was also updated to the best of my abilities without running the pipeline or access to macOS/linux

There is currently a chicken or egg problem, where the sister projects depend on the main project nuget package, but before they can work they need a change in the main project (see af476b4)

cp SDL_image/install_output/bin/tiff.dll ../native/$NAME/tiff.dll
elif [[ $RUNNER_OS == 'Linux' ]]; then
cp SDL_image/install_output/lib/libSDL3_image.so ../native/$NAME/libSDL3_image.so
# TODO: find out if webp, etc. are also needed on linux here
Copy link
Contributor

Choose a reason for hiding this comment

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

With these TODOs, is it just that you haven't tested if these are output at all, or only if they're necessary at runtime? If they're output they I think we should include them.

Copy link
Author

Choose a reason for hiding this comment

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

Output at all. I know that the Linux build statically compiles a lot more dependencies than a windows build by default

I agree that they should be included, if they are built

@smoogipoo
Copy link
Contributor

Have applied a few fixes here. Of importance (to me, for testing), I replaced the PrivateAssets pkgref to SDL3-CS with a project reference.

Haven't tested things yet but have checked the SG output.

@WizzardMaker
Copy link
Author

I have used a PackageReference to SDL3_CS for dependency management when included in another project via nuget. Otherwise with just a project reference the SDL3-CS binary would either need to be manually added as dependency in nuget or be included in the bundle as well. Bundling shouldn't be done, as that would lead to multiple copies when more than one extra library is included

For that Package reference to work we either need to first publish a version with the InternalsVisibleTo change, or add the -t:package output of SDL3-CS as nuget store source to the other projects

@smoogipoo
Copy link
Contributor

I'd really like to avoid local nuget package sources. We've had to do that in other projects and it gets really finicky if you don't version it appropriately (i.e. timestamp or commit depth).

In theory it should be guided by the generated .nuspec, which looks like this:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>ppy.SDL3_ttf-CS</id>
    <version>1.0.0</version>
    <authors>ppy Pty Ltd</authors>
    <license type="expression">MIT</license>
    <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
    <projectUrl>https://github.com/ppy/SDL3-CS</projectUrl>
    <description>Package Description</description>
    <releaseNotes>Automated release.</releaseNotes>
    <copyright>Copyright (c) 2024 ppy Pty Ltd</copyright>
    <repository type="git" url="https://github.com/ppy/SDL3-CS" commit="b3dec6b80cec964abdb7143fdbc89af1c6ee8b5d" />
    <dependencies>
      <group targetFramework="net8.0">
        <dependency id="ppy.SDL3-CS" version="1.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>
  </metadata>
</package>

I'll need to read more on the documentation, as I find it hard to believe PackageReference is the way this should be done.

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