Do not assume font handle is present in assets. #490
Conversation
|
Should I also propose a CHANGELOG.md modification ? I wonder if this is the correct way to do, adding lines in changelog in the PR themselves, as this will surely creates conflicts when two PR are merged that both add lines changelog. Needing a rebase and merge fix for the second applied one. |
Moxinilian
added
A-Assets
If you would like, I think it's okay, but that's a good point about merge conflicts. I wouldn't mind adding this to the changelog after the fix merges. I currently have a branch with some changelog updates https://github.com/memoryruins/bevy/blob/changes/CHANGELOG.md. It's a few days out of date, but I'll update it and open a PR with it soon. edit: heh, yea, updating branch and it does seem very prone to conflicts. Since PRs are squashed into master these days, it's fairly easy to follow the chronology and update the changelog after the fact, and it's less to distract from PRs. |
|
This also fixes #112. Changelog policy is still TBD as we find out what works. For now, lets make it a separate change. |
|
Merge conflicts have been resolved. |
| &glyph_texture, | ||
| ) { | ||
| panic!("could not add character to newly created FontAtlas"); | ||
| if let Some(font) = fonts.get(&self.font) { |
There was a problem hiding this comment.
As an alternative, you could make an early return here in case the font doesn't exist yet. This way the rest of the function isn't indented.
There was a problem hiding this comment.
I think that would require this:
if fonts.get(&self.font).is_some() { return; }
let font = self.fonts.get(&self.font).unwrap();Which is both a double hash, slightly less safe, and less idiomatic. I definitely love flattening out indentation, but im not sure thats the right call here.
There was a problem hiding this comment.
A simple way is:
let font = if let Some(font) = fonts.get(&self.font) { font } else { return };| container_size: node.size, | ||
| }; | ||
| drawable_text.draw(&mut draw, &mut draw_context).unwrap(); | ||
| if let Some(font) = fonts.get(&text.font) { |
There was a problem hiding this comment.
Similarly here, a continue would have the same effect without indenting the whole block
|
I'm merging this now as I think its important for the 0.2.0 release happening today. @TheNeikos I'll address your code style comments in a follow up pr. |
This is a tentative fix for #489
Code changed to not do anything if the font handle is not yet present in the assets resource, removing the panicking 'unwraps'.
Not sure if this is the correct, as there could a design assumption broken before arriving to this pieces of code, and I am not yet fluent enough in bevy to know that.
This fixes both testcases ( late loading of font, or referencing inexistent fonts).