Fixing small issues on my fork, I wanted to share it with you

Scorbutics commented 2 months ago

Prerequisite Checklist

[X] I searched for existing issues to prevent duplicates
[X] I searched for existing discussions on the forum to prevent duplicates
[X] I am here to report an issue and not to just ask a question or look for help (use the forum or Discord instead)

Describe your issue here

First of all thank you for creating this fork ! I don't have any other way to contact you by creating an issue, as I don't find any email address attached to your commits.

Thanks to you, I managed to fully run an SFML project that was targeted to desktop only on an Android v8+ version. Anyway, I did it with little bug fixes, on my fork *. So I wanted to share my fork here: https://github.com/Scorbutics/SFML-opengl-es2

If you are interested, you can directly check my fork latest commits if you want, or I can also make a pull request here, it's up to you to decide.

That would be a pleasure to work with you on a stable version of SFML that targets mobile platforms.

In particular, there is this bug when the RenderTarget switches from a default no-texture shader to a default with-texture shader fixed here: https://github.com/Scorbutics/SFML-opengl-es2/commit/0e8f768c858d7fec68fa60ef72feb7982ab10b9b

Your Environment

OS / distro / window manager: Android
SFML version: 2.6.1

Steps to reproduce

Running on Android v12.

Expected behavior

Running on Android v12 without any flaw.

Actual behavior

I still get bad artifacts and textures in some cases.

Zombieschannel commented 2 months ago

Wow! I see you've made many changes, though I still don't fully understand what each commit fixes (haven't had time to look into each one in detail). One small nitpick: SFML 2 should still use C++03 and while I don't have any issues with using C++17, some users might, and I think std::optional which you are using in one of your commits is a C++17 feature. The goal of this branch for me was to keep as many things as possible related to structure (like function parameters and some #defines) the same as what SFML already uses as well as this branch being a drop and replacement for existing SFML 2.6.1 project code.

This bug you've noticed with default shaders might be fixed with my most recent pull request https://github.com/Zombieschannel/SFML/pull/10 a few days ago.

Also, you mentioned being able to run SFML on Android 8+, is it just something you have not tested on older devices yet? From my testing, it should work on as low as Android 5.

Anyway, I think we could merge our works, step by step, with multiple pull requests, since you've made many changes and I don't yet see what each change does.

Scorbutics commented 2 months ago

Thanks for your answer! I know my changes aren't always very clear, that's why I've bundled the minimum changes possibles to get something working on my setup in the following pull request: https://github.com/Zombieschannel/SFML/pull/12 As you suggested, I've removed the C++17 features.

Also, I can confirm your recent pull request #10 did fix my issue, so I've rebased and discarded my own previous fix :)

About the Android 8+ only part, it's only because we have some C++17 code and at the time I've wrote my port, I used the clang compiler bundled with the NDK, which was at minimum a r26 version, and it only can target devices as old as Android 8 (which is perfectly fine for me, as it's been now 7 years old). Also, the recent clang bundled with NDK made cross-compiling easier, and as I'm porting Ruby and all of its dependencies to Android, I didn't want to bother with technical difficulties.

Scorbutics commented 2 months ago

Closing this one, as the discussion is now pursued in https://github.com/Zombieschannel/SFML/pull/12

Zombieschannel / SFML