-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(ui): add UI text rendering #1378
Conversation
|
5296e35
to
5d858c8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1378 +/- ##
==========================================
- Coverage 54.36% 53.74% -0.62%
==========================================
Files 438 449 +11
Lines 25376 25666 +290
Branches 2346 2374 +28
==========================================
Hits 13795 13795
- Misses 11581 11871 +290 ☔ View full report in Codecov by Sentry. |
aba4392
to
843adb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a first pass - it's a very big PR! Great job on this, tyvm for working on it 🥳
595eef3
to
1a37ccb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
Most of the requests handled, just kept the ones related to the FontAtlas to solve since probably that whole class will suffer some changes. |
3e34299
to
7023a82
Compare
7023a82
to
8ae79e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang-Tidy
found issue(s) with the introduced code (1/1)
ccaf83a
to
2c44a39
Compare
6d867d7
to
55fe5bf
Compare
55fe5bf
to
e0a9f48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just some nitpicking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After all the reviews LGTM!! You could use the asset path instead of the ID, so maybe it would be easier to identify the asset. But it's nothing important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than what I have pointed out, LGTM!
Also make sure to create an issue for the issue you pointed you in the 'known issues' section! |
f4c6719
to
d59c8de
Compare
d59c8de
to
b7de01f
Compare
Description
Added initial support for text in the UI system including font loading, atlas generation and text rendering.
Missing features
UIElement
bounds which can cause some miss alignment in the intended position.Known Issues
UIText
component with the updated text or call.reset()
inUiText.va
to force the recreation of the buffer.Checklist