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

gfx_glyph integration redux #362

Merged
merged 25 commits into from
May 12, 2018
Merged

gfx_glyph integration redux #362

merged 25 commits into from
May 12, 2018

Conversation

Ratysz
Copy link
Contributor

@Ratysz Ratysz commented Apr 27, 2018

This is a work in progress; I feel that having specific diffs in front of us makes it easier to discuss exact implementation of #132 (start of discussion can be found there).

(Side note: trying to adopt verbose commit messages, instead of post-factum dissection in comments.)

`TextCached::load_fonts()` is no longer needed, as fonts created using `Font::new_glyph_font()` load themselves into the `GraphicsContext::glyph_brush` directly. This is possible thanks to new `GlyphBrush::add_font_bytes()`, which should hit release soon (making direct git dependency unnecessary).

`Font::default_font()` and `GraphicsContext::glyph_brush` initialization need de-tangling.
The `Scale` is defined in the `Font` because, firstly, that's how existing API works, and I think it's very intuitive; secondly, from what I understand, having several copies of the same font at different scales is less computationally intensive than dynamically scaling the same font - need to benchmark that.
Not deadset on this, but it's a trivial enough change.
De-tangled `glyph_brush` init and `Font::default_font()` - there's still duplicate data (I think), fully getting rid of that will require changing how `::default_font()` works and is used.
@Ratysz
Copy link
Contributor Author

Ratysz commented Apr 28, 2018

I think I've reached a point where I'll have to start adding (even more) code that's trivial and/or will be removed if we absorb TextCached into Text (or outright replace latter with the former).

It's functional as it is, though I'm not completely sure the transforms in TextCached::draw_ex() all work as they should, or if there's a better way to do that entirely.

Summary:

  • Added a pub(crate) glyph_brush: GlyphBrush field to GraphicsContext; it initializes with the same data as Font::default_font() (which had to be slightly rewritten to hopefully avoid duplication).

  • Added a Font::GlyphFont variant. It can be created by loading a file with Font::new_glyph_font(), or re-created (retrieved from the brush) from a usize ID via Font::get_glyph_font_by_id(). Don't think there's any (significant) overhead for re-creation. Said font variants's scale information is stored within it, as with current ones (there is no hard reason for it, other than keeping it out of TextCached API).

  • Added TextCached drawable. API is nearly identical to Text - the getter methods for width and texture and whatnot are missing; most of them don't make sense in the context.

  • Added an (undocumented, but straightforward) example for experimenting with said TextCached.

There are a few standing questions:

  • General API. Might be worthwhile to review what methods Text actually needs before we start merging/replacing it; that depends on if we're keeping the rendered Font variants (and if we're not, there will be a lot of dead code in text.rs).

  • TextCached bounds, and everything to do with them. From what I understand, gfx_glyph offers automatic wrapping, done via Section::bounds (and Section::layout) field - this can probably be used by/as TextCached::width() and TextCached::height().

  • Performance. Two things to investigate: whether or not having separate Font::GlyphFonts for separate font scales is faster than using the same Font::GlyphFont with different scales; and, how much overhead is calling glyph_brush.draw_queued() in every TextCached::draw_ex(), over just once, somewhere, somehow.

  • The example. If we end up seamlessly integrating TextCached into Text, it will be entirely pointless.

@icefoxen
Copy link
Contributor

Sorry, took me a couple days to find time to take a good look at this.

I like it.

Only note, you should probably use DrawParam::into_matrix() to set up the transform for TextCached::draw_ex(). Hopefully that makes it all work out correctly.

On to pondering questions...

  • General API. I really want to be able to just remove the pre-rendered Text and TTFFont entirely and use gfx_glyph for everything. I want to make an 0.5.0 release that does that, pretty much; my hope is that gfx_glyph will be entirely superior. We can also deal with Can we have pixel-based sizing for fonts? #268 in the process.
  • Bounds, I haven't looked at yet. gfx_glyph definitely has a richer API for text drawing than ggez currently does, and we should take full advantage of that. Love2D isn't quite much guidance because it hides all the Cool Formatting Stuff behind print and printf functions that allow coloring and wrapping of strings; we should probably use something like the TextParam type from dettalant's PR. More advanced font rendering #92 and Support for multi-line text #108 would get solved by this. Maybe theoretically Outlined Text #211 someday... Should look at that more.
  • Performance I think we have to look at empirically. I will whip up some benchmarks when I have time.
  • The example is good for a) profiling, and b) showing off how shiny our stuff is now.

So what I'm imagining now is something that looks sort of like this...

struct TextCached;
impl Drawable for TextCached { 
    fn draw_ex(self, ctx: &mut Context, param: DrawParams) -> GameResult<()> {
        let text_params = param.into(); // Makes a TextParams with reasonable defaults where needed
        self.queue(ctx, params);
        self.draw_queued(ctx);
    }
}
impl TextCached {
    pub fn queue(&self, ctx: &mut Context, params: &TextParams) {
        ctx.glyph_brush.queue(params.into());
    }

    pub fn draw_queued(&self, ctx: &mut Context) {
        ctx.glyph_brush.draw_queued();
    }
}

I think this would let users both do the simple thing easily, and also access the full power of the API... a little like how Mesh and MeshBuilder do about the same thing but one is quick and easy while the other is more powerful.

@icefoxen
Copy link
Contributor

Hilarious benchmark results, from here: https://github.com/icefoxen/ggez/blob/gfx_glyph_integration_too/examples/glyph_bench.rs . It draws 2500 copies of a single text object, either with the current Text::draw(), with the TextCached::draw() from this PR, and with a trivial TextCached::queue() queue'ing up all the copies and TextCached::draw_queued() drawing them all at once.

  • Drawing nothing: 880 fps debug, 12,000 fps release
  • Drawing with Text::draw(): 2 fps debug mode, 81 fps release mode.
  • Drawing with TextCached::draw(): 2 fps debug mode, 265 fps release mode.
  • Drawing with TextCached::queue() and TextCached::draw_queued(): 30 fps debug mode, 884 fps release mode.

So, there's basically no reason NOT to use GlyphBrush for everything.

Ratysz added 9 commits May 1, 2018 16:35
`TextCached` is now built from `TextFragment`s via `TextCachedBuilder`, invoked via `TextCached::builder()`, with a shorthand `TextCached::new()` that creates a `TextCached` from a single `TextFragment`. Not set on builder pattern: it's trivial to have `TextCached::new()` accept either single fragment or a vector of them, forgoing the need for a builder altogether.

`TextFragment`s can be implicitly constructed from `String` or `(String, Color)` (more of these convenience implementations are trivial), and can contain color, font and/or scale information that will override that of their parent `TextCached`. This makes it possible to inline differently colored/sized/etc fragments (letters, words) into the text with dead-simple calls.

When explicitly queued, `TextCached` now accepts a `TextParam` - a struct containing offset, bounds, color, font, and scale information (the latter three can be overriden by individual fragments, as mentioned). Offset dictates where the `TextCached` will be drawn relatively to `DrawParam::dest` specified in `TextCached::draw_queued()` (in screen coordinate units) prior to transforms - this provides the "rotation origin" that can't be specified in `TextCached::draw_queued()`'s `DrawParam`s due to unknown (better: unusable) size of the `TextCached`'s rect. Bounds allow `gfx_glyph` to do it's wrapping magic.
Also added `TextCached::new_empty()`, mostly because I can.
Added more notes; convenience conversion for `TextParam`.
This equalizes queueing and drawing with direct drawing.
@icefoxen icefoxen added this to To do in 0.4.3 release May 6, 2018
Ratysz added 3 commits May 6, 2018 23:34
Cached metrics now don't require mutability everywhere; thread safety and return correctness on `RwLock` poisoning is guaranteed (hopefully I haven't missed anything).
@Ratysz
Copy link
Contributor Author

Ratysz commented May 7, 2018

This is pretty much done, now. Summary, details, and everything else should be apparent from documentation and example.

There are still a few bits to tighten (all marked with TODO) and verify (how well does pixel sizing actually work, etc - mostly things that happened while I was working on this), but it's quite serviceable now.

I'll check up on my other projects, and start working on fully supplanting current Text with this. In a new PR, of course; probably after 0.4.3. Hoping to see any and all criticism, thoughts, and insights well before that!

@icefoxen icefoxen merged commit 7d1104c into ggez:devel May 12, 2018
0.4.3 release automation moved this from To do to Done May 12, 2018
This was referenced May 19, 2018
@Ratysz Ratysz deleted the gfx_glyph_integration_too branch June 8, 2018 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.4.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants