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

Textrender rework #352

Merged
merged 15 commits into from
Nov 21, 2022
Merged

Textrender rework #352

merged 15 commits into from
Nov 21, 2022

Conversation

jembishop
Copy link
Contributor

@jembishop jembishop commented Nov 19, 2022

This reworks the text rendering system so the tiles in some rendered text can be persisted across frames. This is important for performance in situations where you want to incrementally write to a text box for example in a Pokemon/Phoenix Wright style dialogue system. Previously the only way to do this was to render all the chars in the text every frame, which is currently prohibitively slow (though this could be potentially sped up).

In addition with this implementation it is easy to individually colour each character, so I added the ability to do that to.

I expanded the tests to test keeping the renderer instance (which contains the tiles) over multiple frames, and also added in some coloured text.

If you are happy with this implementation then I will change the examples which use text, and also add some more documentation. Thanks for taking a look.

Copy link
Contributor

@gwilymk gwilymk left a comment

Choose a reason for hiding this comment

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

Thanks so much! This is quite a nice improvement to the API and I really like the ability to change colours of fonts as you're rendering. Also, the example didn't really get any more complicated (if anything simpler) so I definitely see this as a win :D.

One very minor comment on the API, but otherwise looks great!

tiles: HashMap<(i32, i32), DynamicTile<'a>>,
}

impl<'a> Write for TextRenderer<'a> {
pub struct TextWriter<'a, 'b: 'a, 'c> {
Copy link
Contributor

Choose a reason for hiding this comment

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

My only comment about this API would be the use of 3 lifetime parameters. Playing around locally, I could reduce this to 2 by getting rid of 'b entirely, and the example still compiled.

I'd prefer minimising lifetime parameters because it can make the API more complicated then in needs to be :). We can always put them in if needed.

Copy link
Contributor Author

@jembishop jembishop Nov 20, 2022

Choose a reason for hiding this comment

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

Thanks, would you mind being more specific with regards to this change? If I try to remove the 'b lifetime param and replace it on the vram_manager/bg with 'a or 'c it gives me borrow checking/lifetime errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've done it here :) (and also fixed the remaining example)

jembishop/agb@textrender-rework...gwilymk:gba:textrender-rework-gwilym

@jembishop jembishop marked this pull request as ready for review November 20, 2022 20:11
@jembishop
Copy link
Contributor Author

jembishop commented Nov 21, 2022

Additional question. I think I am finding that practically the palette index you must pass for the foreground and background colours has to be <16, probably due to the way that bit shifting in the rendering works? If so best to document this

@gwilymk
Copy link
Contributor

gwilymk commented Nov 21, 2022

Additional question. I think I am finding that practically the palette index you must pass for the foreground and background colours has to be <16, probably due to the way that bit shifting in the rendering works? If so best to document this

Yeah, that's because GBA colour palettes have 16 colours in them :). But there would be no harm in adding an assert that the index you pass is within the valid range. I don't think the renderer will work at all with 256 colour palettes at the moment for the gba because it'll need to generate different tiles (I'm not sure dynamic tiles even work with 256 colours yet...)

@jembishop
Copy link
Contributor Author

Thanks for your feedback and changes. I added in the asserts for the colour indices, and I am happy to have this merged.

@gwilymk
Copy link
Contributor

gwilymk commented Nov 21, 2022

amazing, thanks so much! I've added a simple changelog entry and will probably release 0.13 this weekend to include it :D

@gwilymk gwilymk merged commit 99ce2f7 into agbrs:master Nov 21, 2022
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.

None yet

2 participants