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

Add Harness::new_eframe and TestRenderer trait #5539

Merged
merged 14 commits into from
Jan 2, 2025
Merged

Conversation

lucasmerlin
Copy link
Collaborator

@lucasmerlin lucasmerlin commented Dec 29, 2024

This allows creating egui_kittest::Harnesses from eframe::Apps.
I also introduce a new TestRenderer trait and a WgpuTestRenderer. I really didn't like how tightly egui_kittest was coupled to wgpu. This should make it possible to create e.g. a GlowTestRenderer in oder to support custom glow rendering with PaintCallbacks.
HarnessBuilder now has an option to initialize a WgpuTestRenderer, either from a default configuration (this should be updated based on #5506) or a WgpuSetup or existing RenderState.

I also introduce a LazyTestRenderer which basically has the old behavior. In the uninitialized state it remembers the TextureDeltas and a closure to create the actual test renderer. When rendering a snapshot, it will actually initialize the wgpu renderer and apply the texture deltas. This means tests without any snapshots will save the overhead of having to create the wgpu resources and tests with snapshots will continue to work fine without having to configure wgpu in the beginning.

I also renamed Harness::wgpu_snapshot etc... to Harness::snapshot.

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5539-lucaskittest-eframe-app
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin lucasmerlin force-pushed the lucas/kittest-eframe-app branch 3 times, most recently from 8f5f0ec to 8eee137 Compare December 29, 2024 22:05
@lucasmerlin lucasmerlin added the feature New feature or request label Dec 29, 2024
@lucasmerlin
Copy link
Collaborator Author

Tests should pass once #5542 is merged and this is rebased

@lucasmerlin lucasmerlin changed the title Add Harness::new_eframe Add Harness::new_eframe and TestRenderer trait Dec 29, 2024
@lucasmerlin lucasmerlin marked this pull request as ready for review December 29, 2024 22:43
@lucasmerlin lucasmerlin requested a review from Wumpf as a code owner December 29, 2024 22:43
crates/egui-wgpu/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It just works! ✨

Comment on lines +8 to 22
/// In order to access the [`eframe::App`] trait from the generic `State`, we store a function pointer
/// here that will return the dyn trait from the struct. In the builder we have the correct where
/// clause to be able to create this.
/// Later we can use it anywhere to get the [`eframe::App`] from the `State`.
#[cfg(feature = "eframe")]
type AppKindEframe<'a, State> = (fn(&mut State) -> &mut dyn eframe::App, eframe::Frame);

pub(crate) enum AppKind<'a, State> {
Context(AppKindContext<'a>),
Ui(AppKindUi<'a>),
ContextState(AppKindContextState<'a, State>),
UiState(AppKindUiState<'a, State>),
#[cfg(feature = "eframe")]
Eframe(AppKindEframe<'a, State>),
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to be able to store the eframe::App in the State, so it would be accessible via the already existing Harness::state_mut fns, but I needed some way to access the State as a eframe::App when updating.

I think my solution with the function pointer is really cool, but maybe there is a cleaner way to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So essentially State is type constrained iff we're in the Eframe variant, but otherwise not 🤔
Lgtm, can't think of a better way either right now

@lucasmerlin lucasmerlin force-pushed the lucas/kittest-eframe-app branch from 8eee137 to 1ef8ad1 Compare December 30, 2024 11:42
@lucasmerlin lucasmerlin force-pushed the lucas/kittest-eframe-app branch from 1ef8ad1 to a01927e Compare December 30, 2024 11:56
Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

awesome! Let's put in this pr before #5506 which then becomes more limited in scope :)

crates/eframe/src/epi.rs Show resolved Hide resolved
crates/egui-wgpu/src/lib.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/lib.rs Outdated Show resolved Hide resolved
use egui_kittest::kittest::Queryable;

#[test]
fn test_demo_app() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran this locally with cargo test -p egui_demo_app at first without --all-features and then got issues because the top bar has different number of tabs. A bit unfortunate :/
With --all-features it works for the most part, but I need to bump various threshholds.

Edit: Of course this affects lotsa tests. I'll put this in after I landed my "prefer software rasterizer for testing" change as this may (!) stablize it a bit!

Comment on lines +8 to 22
/// In order to access the [`eframe::App`] trait from the generic `State`, we store a function pointer
/// here that will return the dyn trait from the struct. In the builder we have the correct where
/// clause to be able to create this.
/// Later we can use it anywhere to get the [`eframe::App`] from the `State`.
#[cfg(feature = "eframe")]
type AppKindEframe<'a, State> = (fn(&mut State) -> &mut dyn eframe::App, eframe::Frame);

pub(crate) enum AppKind<'a, State> {
Context(AppKindContext<'a>),
Ui(AppKindUi<'a>),
ContextState(AppKindContextState<'a, State>),
UiState(AppKindUiState<'a, State>),
#[cfg(feature = "eframe")]
Eframe(AppKindEframe<'a, State>),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So essentially State is type constrained iff we're in the Eframe variant, but otherwise not 🤔
Lgtm, can't think of a better way either right now

fn render(&mut self, ctx: &Context, output: &FullOutput) -> Result<RgbaImage, String>;
}

/// A lazy renderer that initializes the renderer on the first render call.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be even better if we wouldn't need to initialize the renderer for each TestHarness individually 🤔
(But that might get hairy quickly and a user can always bring their own renderer for that :))

crates/egui_kittest/src/renderer.rs Show resolved Hide resolved
crates/egui_kittest/src/renderer.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf merged commit 46b58e5 into master Jan 2, 2025
46 checks passed
@Wumpf Wumpf deleted the lucas/kittest-eframe-app branch January 2, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui_kittest feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add eframe_kittest to test a whole eframe::App
2 participants