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

sdl2: Add SDL2_gfx submodule to nxdk/lib/sdl/SDL2_gfx #432

Closed
wants to merge 1 commit into from

Conversation

Teufelchen1
Copy link
Contributor

Hi πŸ™‹β€β™€οΈ

This PR aims to add the SDL2_gfx library to nxdk. There is no up to date git repo of it, so I created my own instead as advised by Thrimbor.
I never added a submodule before, so I would like to get some feedback. Additionally, I'm not sure if the mirror should stay in my GitHub profile, but could be moved to XboxDev instead. But thats up for debate.
Thanks for your patience.

@GXTX
Copy link
Contributor

GXTX commented Mar 12, 2021

If it's a part of NXDK and not just something users can add simply I think it'd be better suited under the XboxDev org instead of your user account.

@JayFoxRox
Copy link
Member

JayFoxRox commented Apr 10, 2021

I'm convinced this PR got stuck because of what was also discussed in #418 (comment): How to handle ports.

If it's a part of NXDK and not just something users can add simply I think it'd be better suited under the XboxDev org instead of your user account.

Agreed.

However, I also think that niche-libs (such as this one) should be decentralized with only a central index (the wiki currently acts as one) or package-manager.

The current system is not sustainable.
Most ports mirror the build system of each lib. Therefore library updates become a nightmare, and some libs are simply impossible to port due to shortcomings in the nxdk makefile system (stuff like file-dependent flags being hard to add, cross-compilation where host code runs inbetween, ..).

If nxdk was a more standard-ish GNU toolchain (flags built into nxdk-cc, nxdk-c++, nxdk-ld, nxdk-ar etc., instead of the makefile), then it could be used by various build-systems (such as GNU Make, CMake, etc.). This would solve many issues, as I experienced with my nxdk-cmake attempt (where I was able to port a handful of libs without touching their code-tree at all / without mirroring their build-system + most Xbox specific changes are cleanly upstreamable).

nxdk could then focus on implementing low-level drivers, platform-abstraction (SDL2) and missing POSIX or winapi functions for compatibility.
(Edit: SDL2 is also totally upstreamable so we could also remove it from nxdk in the future; but that's another debate, too)

That way, any user could just host their own nxdk package (if even necessary) without manually integrating it into the centralized nxdk makefile. The samples could move to an nxdk-docs repository which houses samples for the core functionality + samples for user-ports (to aid with exposure).


I'm getting off-topic here (as I usually do), but I feel this needs to be discussed to avoid dangling port PRs without feedback.
I'm sure these dangling PRs got discussed on Discord or elsewhere, but the lack of public feedback is far from ideal.
The lack of activity is probably discouraging people who are looking into upstreaming or creating their own ports.

@@ -31,3 +31,6 @@
[submodule "lib/sdl/SDL_image"]
path = lib/sdl/SDL2_image
url = https://github.com/SDL-mirror/SDL_image.git
[submodule "lib/sdl/SDL2_gfx"]
path = lib/sdl/SDL2_gfx
Copy link
Member

@JayFoxRox JayFoxRox Apr 10, 2021

Choose a reason for hiding this comment

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

I'm not sure if this belongs into lib/sdl.

This library is not an official SDL project - the other projects in that folder are either official or done by SDL organization members and promoted by SDL.
It's really just a random drawing library that happens to use SDL internally.

@@ -31,3 +31,6 @@
[submodule "lib/sdl/SDL_image"]
path = lib/sdl/SDL2_image
url = https://github.com/SDL-mirror/SDL_image.git
[submodule "lib/sdl/SDL2_gfx"]
path = lib/sdl/SDL2_gfx
url = https://github.com/Teufelchen1/libSDL_gfx.git
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if necessary here, but instead of reuploading the files without history, you can use "Import Repository" for https://svn.code.sf.net/p/sdl2gfx/code/trunk on GitHub to end up with the full SVN history.

Unfortunately this still won't allow to keep the repository synchronized with the SVN counterpart.
There are other solutions and tools to do that though (if necessary).

However, frustratingly, the author did not use tags and there's no commit from the official release date of the source package either. So it looks like they released a version that's not checked or marked in their source-control.
So it's questionable if there's any benefit from the history.

@@ -74,3 +74,30 @@ CLEANRULES += clean-libsdlimage
.PHONY: clean-libsdlimage
clean-libsdlimage:
$(VE)rm -f $(LIBSDLIMAGE_OBJS) $(NXDK_DIR)/lib/libSDL2_image.lib

SDL_GFX_DIR = $(NXDK_DIR)/lib/sdl/SDL2_gfx
Copy link
Member

@JayFoxRox JayFoxRox Apr 10, 2021

Choose a reason for hiding this comment

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

SDL2_gfx and SDL_gfx are different projects which do the same thing, but have different dependencies and requirements:

SDL_gfx supports the original SDL1 library using software surfaces, and SDL2_gfx supports SDL2 using rendering functions.

It's realistic to imagine that someone will use some SDL 1.x implementation with nxdk.
In that case they might want to use SDL_gfx instead of SDL2_gfx.
Even if this is unlikely to happen in practice, we should still encourage a clean workspace and avoid such potential problems.

Hence we should ensure that we use distinct variable and filenames.

@Teufelchen1
Copy link
Contributor Author

I'm convinced this PR got stuck because of what was also discussed in #418 (comment): How to handle ports.

Yes.

The current system is not sustainable.

πŸ“‰

[...]
That way, any user could just host their own nxdk package (if even necessary) without manually integrating it into the centralized nxdk makefile. The samples could move to an nxdk-docs repository which houses samples for the core functionality + samples for user-ports (to aid with exposure).

I'm getting off-topic here (as I usually do), but I feel this needs to be discussed to avoid dangling port PRs without feedback.

(Yes you do, we missed that)

I'm sure these dangling PRs got discussed on Discord or elsewhere, but the lack of public feedback is far from ideal.

Yes it got discussed on Discord.

 Thrimbor (March 14. 2021, 01:27 am):
Ok, I thought about how to handle ported libs, the build system and distribution a bit, and here is my proposal:
- Create a new repository named "nxdk-ports". This will serve as a collection of all ported libraries that are not considered essential. zstd and SDL_gfx would get put there instead of the nxdk repo, and libjpeg, libpng, zlib, SDL_ttf and SDL2_image would move there, too. SDL2 would stay in nxdk for the moment, as it's currently the best method to get controller input, but when we rewrite Xinput, we can move out SDL, too.
- Provide multiple different flavors of the nxdk Docker image: nxdk:lto (prebuilt with LTO enabled), nxdk:release (prebuilt with -O2), nxdk:debug (prebuilt with -g) and nxdk:src. nxdk:src will be the equivalent of the current Docker image, containing only the build requirements and the code, while the others will contain prebuilt versions of nxdk without the code.
- Provide a new Docker image nxdk-full or similar, that will contain nxdk and the ports, with all the same flavors as the other image.
- New nxdk-full images are built by GH Actions on the nxdk-ports repo. We can use a webhook to also trigger an image build whenever the nxdk repo is updated.
- Refactor the buildsystem, have a separate makefile that just provides the necessary variables etc. to get started, so that libraries can have full control over how they're built, without having their CLFAGS cluttered

All in all, with nxdk-full, this would provide an easy way for people to build stuff locally or on CI without having to keep all the submodules around or build the libraries themselves over and over, while we still have the option to completely change the way it's built internally - we could even switch to a package-based system, and users of the Docker image wouldn't need to care.

@JayFoxRox
Copy link
Member

My latest thoughts on matter of ports is in #418 (comment)

I think we should close both of these PRs; but I have a very narrow vision for nxdk, so I'd like to have at least one other maintainer agree with me (by closing those PRs, unmerged).

@thrimbor
Copy link
Member

The PR author has agreed to closing this on Discord. The library will eventually be provided as a package once we have a package manager.

@thrimbor thrimbor closed this Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants