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

Integrate gfx_glyph #132

Closed
ishitatsuyuki opened this issue Aug 28, 2017 · 18 comments
Closed

Integrate gfx_glyph #132

ishitatsuyuki opened this issue Aug 28, 2017 · 18 comments

Comments

@ishitatsuyuki
Copy link

It's a high quality text rendering library with line breaking handling, which should be better than the current codebase.

@icefoxen
Copy link
Contributor

It would be nice, yes! Glyph caching is on the to-do list anyway; I've been looking at gfx_glyph as a way of doing this but haven't really tried it out.

@icefoxen
Copy link
Contributor

icefoxen commented Sep 25, 2017

Looks okay but there's a few rough edges. The main ones:

  • GlyphBrush takes a lifetime for the font it refers to but doesn't seem to have an option for a Cow or such, so if you dynamically load a font and try to store it and a GlyphBrush together you end up with the Dreaded Self-Borrowing Struct
  • It assumes your render target is using format gfx::format::Depth but by default ggez uses gfx::format::DepthStencil
  • I need to get the stupid gfx integration working properly at all argh

@ishitatsuyuki
Copy link
Author

I have cross posted the second issue. I'm not sure about the solution of the other two, so I will leave it for further discussion.

@alexheretic
Copy link

@icefoxen regarding your first rough edge; gfx_glyph takes Into<SharedBytes<'a>> as font-data rather than a reference to a byte array, so you can use owned data (since v0.3).

Let me know if you find more annoyances with gfx_glyph.

@Ratysz
Copy link
Contributor

Ratysz commented Apr 25, 2018

So I've been tinkering with this (not everything is on my public fork), and ran into graphics::Text being incompatible with how gfx_glyph does things.

graphics::Text stores a texture and draws that in Text::draw_ex(); "updating" it is remaking it via Text::new(), with a graphics::text::Font reference (it's an enum that determines how the Texts texture is created).

gfx_glyph frontloads all fonts into a GlyphBrushBuilder, which builds into a GlyphBrush. It queues text fragments (VariedSections) that contain all drawing information, including a FontId (handle to a font stored in the GlyphBrush), and draws them to a RenderTargetView all at once with GlyphBrush::draw_queued().

So far, I see these approaches on how to marry the two:

  • Try to preserve current strategy, build a GlyphBrush instance when initializing the Font, store it inside (either a new variant, or rewrite of Font::TTFFont). Calling GlyphBrush::draw_queued() in every Text::draw_ex() defeats the purpose, so a kind of batching API is needed.

  • Put GlyphBrush in graphics::Context (as is proper), then either rewrite Font::TTFFont or create a new variant that would interface with it. Either have user explicitly call for GlyphBrush::draw_queued(), or do that in the loop (event::run()). The caveat is, GlyphBrush cannot accept fonts after it's built, and ggez can't feed it fonts until context::Context (and graphics::Context) are initialized.

Both will require some sort of font handle to be stored within Text, and make methods like Text::width() partially useless (due to their reliance on internal texture; Texts made with non-brush Font variants can still use those). Of course, it's possible to create a separate CachedText or similar and use either approach with it; I think #343 is something like that.

After naively implementing the first one (it lacks the drawing part, and screws up API by requiring 'static on font references passed to Text::new(); and, Option gore), I'm leaning towards the second. Can't quite figure out the most appropriate way of handling the circular initialization problem: Option<GlyphBrushBuilder> and Option<GlyphBrush>in graphics::Context that are lazily/explicitly "swapped" after fonts are loaded by user (or on first draw call)? A new struct that pretty much does the same? A Future?

@icefoxen
Copy link
Contributor

@Ratysz Hey, thanks for the great summary! I'll play with the code you linked.

One thing is sure, this will almost certainly need changes to the text drawing API to take full advantage of gfx_glyph. That's fine. Changing ggez's API is better than trying to shoehorn something into a shape that doesn't fit.

What I was originally thinking of is your first option, since I thought the glyph cache was only cleared upon demand. Turns out this is not true, it basically gets cleared whenever you change what text you're drawing. It actually looks like adding an option to only clear the cache explicitly would actually be really easy, if we ever need it.

Digging around into its guts a little it looks like there's a few layers to this. It looks like gfx_glyph uses rusttype::gpu_cache to cache rendered glyphs on a texture, which is all I ever really wanted from it. However it ALSO creates a vertex array with everything it needs to do the actual drawing, so that it only has to calculate positioning information and such when things change as well. I'm PRETTY sure that this vertex array is all that gets thrown away when draw_queued() is called and it notices a section isn't being re-used, the glyph texture is preserved. It appears that the only time that gets cleared and has to be re-made is when it ends up too small to contain all the glyphs on it and gfx_glyph creates a larger one. @alexheretic , am I correct in how this works?

So what it looks like is that we can do something like the first option and it will probably work okay; we basically use GlyphBrush as something that can draw text immediately (call draw_queued() inside draw_ex(), which should be kinda-fast) or can instead take a bunch of text to queue and draw it all at once (so use GlyphBrush the way it should be used). It doesn't all have to be perfect, we can give the user an easy option and a more-complex-but-better-option.

Of course, the real answer is "try things out and benchmark". That ought to be interesting!

Also, having multiple GlyphBrush'es share the same Font appears to be perfectly possible now, so that problem is long solved.

Now pondering the second option... it would essentially come down to having a separate GlyphBrush stored for each Font, I expect, either in the Font or the GraphicsContext. That seems the only real way to be able to add fonts to a GlyphBrush after it's created, which is strange since I don't see anything that would make that terribly difficult, so far. This seems like a rather odd approach rather than just making ggez's API more cooperative though. The circular initialization problem is already something that happens for the GraphicsContext a lot, but you can work your way around it by structuring things sneakily enough.

(Also, gfx_glyph uses log to print helpful messages like "your cache is too small, I'm expanding it", so we should be able to take advantage of that now. :D)

SO

Assuming this all makes sense, how to move forward:

  • Create a nice little module to draw text with gfx_glyph in a way that we like, something along the lines of Add glyph cache implement into ggez(WIP) #343 , maybe throw it into 0.4 with a big fat "experimental" warning.
  • Play with it and see how we like it.
  • Assuming it works well, rework the text drawing so that this becomes the only way of drawing text that you need to have.

@alexheretic
Copy link

I'm PRETTY sure that this vertex array is all that gets thrown away when draw_queued() is called and it notices a section isn't being re-used, the glyph texture is preserved. It appears that the only time that gets cleared and has to be re-made is when it ends up too small to contain all the glyphs on it and gfx_glyph creates a larger one. @alexheretic , am I correct in how this works?

Yeah that's about right but I never miss an opportunity to explain it with boring verbosity; there are 3 kinds of caching layout, texture and draw.

  • The layout cache makes sure we only figure out the position of each glyph in a Section once, so this is removed per-section once the section is no longer used or changes.
  • Then we pass the positioned glyphs to the gpu texture cache, updating the texture as necessary. Glyphs are never explicitly removed from the cache, but the cache will clear them itself when it needs to. As you say we also upgrade the texture to a larger size if it's too small, this is not optimal though so users can use gfx_glyph=warn logging on to see when it's happening and try to pick a larger texture size to begin with.
  • Finally the draw cache remembers the full vertex state and just reuses them if all sections are the same. This makes drawing very fast indeed but is cleared if any section has changed or been removed.

@Ratysz
Copy link
Contributor

Ratysz commented Apr 26, 2018

I tinkered some more, this time putting a centralized GlyphBrush in GraphicsContext. (It's easy enough to re-commit to the decentralized approach, and I still wanted to check it out.)

Implemented a new TextCached (name wip) that tries to mimic Texts API, but instead works with said GlyphBrush. Right now it queues and draws in the same call, but it's trivial to split or extend that. Not yet sure which additional fields (over Text), besides font_id, are needed for it to be a fully-featured replacement. Quite likely the font size.

GlyphBrush initializes with graphics::Font::default_font(), and can be replaced with TextCached::load_fonts(), which takes an array of graphics::Fonts and builds a new brush out of them. It's a bit icky: when using exclusively TextCached, graphics::Fonts in user app are created only to be destructured back into rusttype::Fonts, which are then cloned - this avoids requiring 'static in user side.

If/when alexheretic/glyph-brush#24 happens, it should be possible to omit the ungainly TextCached::load_fonts() and load fonts into the brush as they are created, either automatically for Font::TTFFont - or, better, explicitly, through a new variant. Tweaking graphics::Font::default_font() will then eliminate the last duplicates, I hope.

@icefoxen
Copy link
Contributor

Ratysz's PR has been merged, and it's pretty slick. The API looks okay, we'll leave it as it is for now and play around with it, and see if there's anything we want to change or that needs fixing. In 0.5.0 it should become the main API.

@icefoxen
Copy link
Contributor

Pulled a bunch of the TTFFont stuff out of the graphics::text module in commit 64b5cdb, leaving only GlyphFont behind. Next steps:

We need an API where we can:

  • Create nice fonts that are both a general point size and also a specific pixel size
  • Create variable and fixed width fonts nicely

@icefoxen
Copy link
Contributor

icefoxen commented Jun 14, 2018

Random pondering:

But the gap between "create text" and "draw text" is now quite fuzzy.
Hmmm, there's "create text specification" (creating the TextCached), "feed text specification into the system and get everything ready to draw" (TextCached::queue()) and then "actually draw it" (TextCached::draw_queued()).
Going by my gut, we want is the second two steps to be merged in the common case, and be able to split them apart in the uncommon case, mirroring SpriteBatch somewhat.
gfx-rs does sort of the same; the Encoder serves basically the same role as the GlyphBrush...
You queue bunches of stuff and then draw 'em all with "graphics::present()"
Yeah
Because graphics::draw(text_cached) ends up calling draw_queued() anyway.
So we need a Text, which is just the current TextCached type without the queue() method, then a TextBatch that is analogous to SpriteBatch
but I worry then that if you draw a bunch of Text's, draw other stuff, then draw more Text's, and they all overlap, then the order won't work out right 'cause they have separate Encoder's...
Have to check that.

@icefoxen
Copy link
Contributor

Figured it out. TextBatch (nee TextCached) should be renamed Text, since that's basically what it is. Then you can do graphics::draw(Text). Or you can do graphics::queue(Text) and it will draw them all at once when you do graphics::draw_queued(). And each Text is made from zero or more TextFragment's, as it currently is. ezpz.

Also, bitmap fonts should be entirely separate from this and part of their own crate, since they can just be implemented using SpriteBatch'es anyway.

icefoxen added a commit that referenced this issue Jun 25, 2018
A lot of little shuffling and refactoring of API, which
will probably go on for a while.  There's like a million
TODO's and such.  Still, very nice all in all.

Closes #132.
@icefoxen
Copy link
Contributor

DONE IN 2ae4841

🎉 🎉 🎉

@systemraen
Copy link

@icefoxen Regarding this, I'm getting the following when running the hello_world example:

error[E0277]: the trait bound `&ggez::graphics::Text: ggez::graphics::Drawable` is not satisfied  --> examples\hello_world.rs:52:9
   |
52 |         graphics::draw(ctx, &self.text, (dest_point,))?;
   |         ^^^^^^^^^^^^^^ the trait `ggez::graphics::Drawable` is not implemented for `&ggez::graphics::Text`
   |
   = help: the following implementations were found:
             <ggez::graphics::Text as ggez::graphics::Drawable>
   = note: required by `ggez::graphics::draw`

@icefoxen
Copy link
Contributor

icefoxen commented Jun 27, 2018

Yeah that's because I'm in the middle of tearing apart and redoing a bunch of drawing stuff...

...or because I did something dumb. Are you working off of 2ae4841 or a later version?

@systemraen
Copy link

Alright, I was on e056397 which had the change to draw function using the non-reference version of D

@icefoxen
Copy link
Contributor

Yeah, if I was smart I would have made that stuff part of its own branch; I'm not used to people being interested enough to track devel I guess. XD

@systemraen
Copy link

No problem, thanks for helping figure out what was happening and all the work you're doing. Excited to see the SDL dependency removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants