-
Notifications
You must be signed in to change notification settings - Fork 158
[DRAFT] AGS 4: Replace Alfont with SDL_ttf #2711
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
base: ags4
Are you sure you want to change the base?
Conversation
d98176c to
1369313
Compare
fdc2f06 to
2f57807
Compare
|
@ericoporto if you have some spare time, could you take a look at the CMake scripts in this pr?
I will read about CMake scripts and try figure this out too in the meantime. |
5812598 to
ba821f1
Compare
|
It seems like I figured out most things, using existing scripts as an example (although few things still are strange to me). Following immediate issues remain with building:
|
18aabd3 to
1eeca86
Compare
|
This looks alright, but I can look into this next week when I am back home, I have a variant of this in one of my older branches where Freetype and SDL_ttf are built as static libraries (in VS it gets added to VS dependencies built on the server), if it’s desirable. The advantage of having Freetype built alongside the engine instead is to take advantage of miniz + libpng to get colorful emojis, but perhaps this is not necessary. And also having the sources makes it much easier to build with different build systems. I can’t tell the issues with the things that aren’t building. |
|
This will take me a little while but my idea is to put SDL_ttf like we have SDL_sound as a dependency, and then put in the docker images (an attempt here ericoporto@364655b) and add to |
|
I am confused by the last two comments, cannot tell if they suggest same or opposite approaches? The first one looked like you say that having sources along with the engine is better (?), the second sais that it's better to have them separate. Also, I cannot tell if you mean to only have SDL_ttf downloaded separately, or both SDL_ttf and freetype. The problem with SDL_ttf source in the current branch is that it's missing one functionality that is important for the engine (see libsdl-org/SDL_ttf#538). Here I added it directly, but if it's loaded from elsewhere then either we have to wait until it's added to the upstream library (if that happens at all), or alternatively maintain our own library version and fetch that. |
|
I think you got it right. Overall it’s easier to have the sources in the directory directly so updating or changing something can be done more easily - in parts. But having the sources not there and have the dependencies outside is less code we are committing in our main repository. My feeling is we are never going to edit Freetype's code directly because we haven’t done so in a long time. So I think this could be done in a sort of middle ground where the current old Freetype is removed, and we get the new one as an external dependency and build SDL_ttf from code directly in the repo. The “get Freetype as an external dependency” could be done as we have for other dependencies right now or alternatively as a submodule. |
1eeca86 to
f491456
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f491456 to
5313dca
Compare
|
I think your fork of SDL_ttf should be under AGS organization to make it easy to sync changes from upstream, like fork to the org, add an AGS branch and add your changes to this branch. This is my learning from syncing changes manually from mojoAL. |
|
I tried to throw away the Freetype CMakeLists.txt because it was too complex and replaced with our own... It seems things are building. ericoporto@cd19e67 (Cirrus, GHA) , I also had to rebase because I was getting a weird error at link time in my computer. It also builds much faster for me because freetype configuration step on it's own CMakelists was taking three minutes on my PC and now it's instant. |
|
@ivan-mogilko could you rebase this and apply my commit from above? Outside maybe (haven't checked) some README or License information somewhere it should be good. |
Used this as an opportunity to move sources from Common to libsrc folder in the project root.
5313dca to
a2fe3f2
Compare
A simpler CMake file is easier to deal and configure for our possible platforms. The original CMake wasn't building on Emscripten or on Windows and I had no idea how to fix, but with this one it appears it works on all the platforms we care.
a2fe3f2 to
10c5ab5
Compare
|
Okay, I rebased. But I am unsure about the future of this PR. You have mentioned that it's better to fetch SDL_ttf from its own repository (I am not certain about Freetype). Is this what should be done instead? I wish we make a decision now, and finalize this, since the PR is stalling for several months now. My addition to SDL_ttf (which has not been approved upstream yet) is not critically necessary, but needed mostly as a fixup for "bad" fonts. I suppose that there could be a way to detect if the function is present in the loaded SDL_ttf library, and use this function or follow a fallback scenario in case it's not present. Same could be done for other custom additions, like "character spacing" feature proposed by edmundito. This way we may have a custom SDL_ttf fork, but at the same time someone can still use a SDL_ttf installed on their system from upstream. |
|
I changed my mind on fetching things vs having in the repository. Having in the repository is easier to adjust for different build systems, specifically VS and Xcode (for iOS). Plus If there's something that isn't building due to minor issues (like for Emscripten), it's easier. I think if we supported only regular desktop systems and nothing else I would trust upstream, but as soon as iOS, Android and Emscripten join in, things may break. I have been playing also with Windows Arm64, which is another thing that upstream libraries usually rarely test. Also we want SDL_ttf on the Editor too, and this way it's easier to build. About the PR I believe it should be done now and we can move on with it as is and then fix anything else that comes up - I think like minor stuff like license mentions, on the Editor and on Linux. I will take a look on this after merge. @ivan-mogilko it appears there is something wrong in the Vcxproj file? |
|
@ivan-mogilko this needs to be rebased and fixed again before merging. Other than this, it looks done, any reason for being a draft still? |
|
There are two things that are bothering me:
|
Drafting a change required to resolve #1528.
What is done:
What will not be part of this PR:
What should be added to this PR:
But that will require to change few bits in the engine, as right now text wrapping is done prior to calling Font Renderer interface.
Likely, a IAGSFontRenderer interface will have to be expanded with RenderTextWrapped method, and same done in plugin API.