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

Managed texture loading #3297

Merged
merged 64 commits into from
Sep 6, 2023
Merged

Managed texture loading #3297

merged 64 commits into from
Sep 6, 2023

Conversation

jprochazk
Copy link
Collaborator

@jprochazk jprochazk commented Sep 4, 2023

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
Copy link
Owner

emilk commented Sep 6, 2023

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.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Exciting!

@@ -1998,6 +2016,8 @@ impl Context {

/// 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
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.

crates/egui/src/load.rs Show resolved Hide resolved
crates/egui_extras/Cargo.toml Outdated Show resolved Hide resolved
crates/egui_extras/src/loaders.rs Show resolved Hide resolved
@jprochazk
Copy link
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
34 checks passed
@jprochazk jprochazk deleted the improved-texture-loading branch September 6, 2023 08:51
crates/egui/src/load.rs Show resolved Hide resolved
crates/egui/src/context.rs Show resolved Hide resolved
/// 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
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

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

type Entry = Poll<Result<Arc<[u8]>, String>>;
Copy link
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 🤷

@@ -95,6 +95,11 @@ impl TextureHandle {
crate::Vec2::new(w as f32, h as f32)
}

/// `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
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
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