-
-
Notifications
You must be signed in to change notification settings - Fork 23.4k
Presort text draw calls. #108369
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: master
Are you sure you want to change the base?
Presort text draw calls. #108369
Conversation
7c707e0 to
579ce25
Compare
2ea9abf to
5cbc9d3
Compare
e6f38fd to
3245a94
Compare
Compatibility break actually fixes an incorrect earlier definition.
3245a94 to
ad37465
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.
Given how fairly important batching text calls seems to be, the class reference is a bit lacking in recommendations on how to make use of the API introduced in this PR.
| <param index="5" name="color" type="Color" default="Color(1, 1, 1, 1)" /> | ||
| <param index="6" name="oversampling" type="float" default="0.0" /> | ||
| <description> | ||
| Adds draw calls for dropcap glyph outlines to the draw list [param list]. |
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 can be seen as huge nitpick but I had to get it off of my chest someday.
All of these very brief descriptions in the TextServer may seem good at a glance, but they're quite unwelcoming. Hard to parse, difficult to localize, devoid of information. Yes, as an experienced developer with decent English knowledge, it's reasonably easy to understand what each parameter does, but that's not every one. Neither are the parameter names taken for granted in most of the class reference.
In this PR the descriptions are mostly okay, in my opinion, considering it's better to keep them consistent with the surrounding TextServer-related documentation (which is arguably more important), as well as to avoid redundancy. Regardless, it will be worth doing a huge pass someday.
Again, it's easy to understand these as a professional, but exposing methods and assigning them very terse and cryptid descriptions is one sure way to make the reader feel uneasy, like they're interacting with something they shouldn't be doing.
| Draws all draw calls in the draw list to the canvas item [param ci]. If [param free] is [code]true[/code], draw list is automatically deleted. | ||
| </description> | ||
| </method> | ||
| <method name="draw_list_reserve"> |
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.
Are we sure we want to expose what is essentially a reserve method to the public API? I would be very wary since this is a very prickly topic of discussion.
(Note that clayjohn got originally confused by the purpose of the linked PR).
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.
With the way TextServer is implemented (to be implementable as GDExtension), I do not think it is possible to have non-exposed methods. And unlike Dictionary, TextServer is a low level API most users won't ever interact with, so it should not be an issue.
ad37465 to
bc5bc87
Compare
bc5bc87 to
11bcba5
Compare





Pre-sort text draw calls to improve batching, should significantly reduce draw call number for languages with a lot of glyphs and multiline text with outlines/shadow.
Before:
After:
Fixes #107109
Fixes #104537