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

Added GPUTextEngine #412

Merged
merged 29 commits into from
Dec 7, 2024
Merged

Added GPUTextEngine #412

merged 29 commits into from
Dec 7, 2024

Conversation

captain0xff
Copy link
Contributor

@captain0xff captain0xff commented Oct 17, 2024

This PR extends the new text rendering api to support the gpu api. It's still not ready for merging but the reason I am creating the PR is so that the API can be reviewed.

Things that I still need to do:-

  • The API needs to be documented (I will do this after the public API is finalized)
  • The math_3d.h header should be removed in favor of custom math
  • Shaders should be added for all the backends along with a proper workflow to load and build them
  • The textgputext example should be improved to show the full capabilities of the api

I will need some help with the shaders because I don't have much idea about the workflow needed for the Metal and Direct3D backends.

@captain0xff
Copy link
Contributor Author

Looks like stuff broke after pulling the latest changes. I am working on fixing them.

@captain0xff
Copy link
Contributor Author

Fixed.
The SDL_gpu_textengine.c is mostly similar to the SDL_renderer_textengine.c. I have just adjusted most stuff for the gpu api.
I have added TTF_GetGPUTextDrawData to return the user the needed geometry data to draw the text and removed the Draw function. The UpdateGPUTexture method is stolen from the renderer gpu backend and can/should be improved. Apart from this I have updated the CreateDrawSequence to cache the positions along with the texcoords and indices.

examples/testgputext.c Outdated Show resolved Hide resolved
@slouken
Copy link
Collaborator

slouken commented Oct 17, 2024

In general, this looks good so far!

examples/SDL_math3d.h Outdated Show resolved Hide resolved
@captain0xff
Copy link
Contributor Author

I have removed the hardcoded shader stuff and took the script from src/render/gpu/shader to build my shaders. I have only tested the workflow for spirv and vulkan since I only have access to a linux machine atm. I will appreciate if someone can help generate the shaders for other platforms or even provide some input about them. The renderer gpu backend doesn't seem to have any hlsl or msl shader present so I assume that the script generates them from spirv provided you have spirv-cross in your path.
Next, I will clean up and improve the example. It may take some time depending on how quickly I can grasp the math behind perspective and lookAt functions and implement them. I can copy them from somewhere but I like to understand the math behind stuff I do. Also, that can cause licensing issues.
After, that hopefully that the PR will be ready for a final review once I address the CI.

@captain0xff
Copy link
Contributor Author

I have improved the example and also formatted it

@captain0xff
Copy link
Contributor Author

I have fixed the CI.
I am marking the PR ready. It still needs the shaders for the other platforms but I don't have access to a mac and windows machine.

@captain0xff captain0xff marked this pull request as ready for review October 25, 2024 15:06
examples/testgputext.c Outdated Show resolved Hide resolved
@thatcosmonaut
Copy link

Now that we have a CLI for shadercross we should be building shaders with that instead, no need for another shader frontend dependency.

@captain0xff
Copy link
Contributor Author

I don't think you need to commit all files in the shaders directory. See https://github.com/libsdl-org/SDL/tree/a10578acbdd86924574c9d5863b698b90a4ae212/src/render/gpu/shaders, there are no .spv files there.

Yeah, pushed them by mistake. I will remove those files.

examples/testgputext.c Outdated Show resolved Hide resolved
@captain0xff
Copy link
Contributor Author

Note, the example seems to leak memory. But it's probably due to #425 and so not caused by me.

@madebr
Copy link
Contributor

madebr commented Oct 30, 2024

Note, the example seems to leak memory. But it's probably due to #425 and so not caused by me.

You can verify by linking to SDL3_test, and calling SDLTest_TrackAllocations(void); at the very beginning, and calling SDLTest_LogAllocations(void) at the very end.

@captain0xff
Copy link
Contributor Author

Well it's show allocations and those are increasing.

@captain0xff captain0xff changed the title Added GPUTextEngine (WIP) Added GPUTextEngine Oct 30, 2024
@captain0xff captain0xff requested a review from slouken October 30, 2024 19:32
@captain0xff
Copy link
Contributor Author

Just to make sure, this PR isn't waiting for something from my end right?

@@ -0,0 +1,37 @@
License
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't include any fonts in the SDL_ttf project. There are plenty out there already for people to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from discord.

I actually decided to ship the font because if the font isn't monospaced the example will look quite weird and also different fonts will change the size of the text drastically due to the 3D rotation, it will be very hard to mitigate that only by changing the point size.

Copy link
Collaborator

@slouken slouken left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Copy link
Contributor

@MirkoCovizzi MirkoCovizzi left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! I'm implementing fonts in my project and this is a very good learning material!

Just noticed a minor typo

examples/testgputext.c Outdated Show resolved Hide resolved
examples/testgputext.c Outdated Show resolved Hide resolved
@captain0xff
Copy link
Contributor Author

Done. Thanks for the review.

@captain0xff
Copy link
Contributor Author

libsdl-org/SDL#11398 was closed as not planned. So, this PR is again ready I hope?

@slouken
Copy link
Collaborator

slouken commented Nov 15, 2024

@captain0xff, can you rebase and resolve conflicts?

@slouken
Copy link
Collaborator

slouken commented Nov 15, 2024

Actually, never mind, I can squash this as a single commit.

@captain0xff
Copy link
Contributor Author

Yes, that will be better. Most of the commit messages are not very sensible.

@slouken slouken merged commit 82f028b into libsdl-org:main Dec 7, 2024
5 checks passed
@slouken
Copy link
Collaborator

slouken commented Dec 7, 2024

Merged, thanks!

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.

5 participants