Skip to content

Managed texture loading - #3297

Merged
jprochazk merged 64 commits into
masterfrom
improved-texture-loading
Sep 6, 2023
Merged

jprochazk merged 64 commits into
masterfrom
improved-texture-loading

Conversation

@jprochazk

@jprochazk jprochazk commented Sep 4, 2023

Copy link
Copy Markdown
Collaborator

Part of #3291.

  • Add types from the proposal
  • Add loader implementations from proposal to egui_extras
  • Update usage in examples
  • Documentation

@jprochazk jprochazk added feature New feature or request egui egui_extras labels Sep 4, 2023
@jprochazk jprochazk changed the title Improved texture loading Managed texture loading Sep 4, 2023
@jprochazk
jprochazk requested a review from emilk September 5, 2023 21:44
@emilk

emilk commented Sep 6, 2023

Copy link
Copy Markdown
Owner

One thing that isn't done here in any way is constructing an Image2 from non-static bytes, e.g. egui::ColorImage::example(). Not exactly sure how that should work. Should loading from bytes just allow non-'static lifetimes?

I think so - the IncludeBytesLoader still copies the bytes to an Arc<[u8]> anyways, so why not.

A potential optimization is to replace Arc<[u8]> with enum Bytes { Static(&'static [u8]), Shared(Arc<[u8]>) } to avoid both the copies and the extra memory use.

@emilk emilk left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Exciting!


/// Try loading the texture from the given uri using any available texture loaders.
///
/// Loaders are expected to cache results, so that this call is immediate-mode safe.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

We should consider how we can alert the user of methods that aren't immediate safe, most notably the old Context::load_texture. Adding a ⚠️ Not immediate-mode safe warning to them might be a start, but perhaps we could also add it to their name somehow. load_texture_sync or something.

Comment thread crates/egui/src/load.rs
Comment thread crates/egui_extras/Cargo.toml Outdated
Comment thread crates/egui_extras/src/loaders.rs
@jprochazk

Copy link
Copy Markdown
Collaborator Author

I'm not totally sure about the API I came up with for Bytes, but we can always improve it later.

@jprochazk
jprochazk merged commit ec671e7 into master Sep 6, 2023
@jprochazk
jprochazk deleted the improved-texture-loading branch September 6, 2023 08:51
Comment thread crates/egui/src/load.rs
Comment thread crates/egui/src/context.rs
/// Load the image from some raw bytes.
///
/// See [`ImageSource::Bytes`].
pub fn from_bytes(name: &'static str, bytes: impl Into<Arc<[u8]>>) -> Self {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Again, a single impl Into<Bytes> produces a smaller API surface area

};
use std::{sync::Arc, task::Poll};

type Entry = Poll<Result<Arc<[u8]>, String>>;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can also use Bytes here 🤷


/// `width x height x bytes_per_pixel`
pub fn byte_size(&self) -> usize {
self.tex_mngr.read().meta(self.id).unwrap().bytes_used()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's avoid unwrap please 😬

We can fall back to zero instead (if we are not in the texture manager, we are not loaded)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The size method also unwraps, so I just copied that. I'll change both to not unwrap.

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

Labels

egui_extras egui feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants