Skip to content

Add test for mipmap filters #19095

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented May 6, 2025

Objective

Add a test for mipmap filters, and as a bonus test GltfLoaderSettings::override_sampler. Neither of these are covered by current tests or examples.

Solution

gltf_override_sampler.mp4

The test supports CI screenshots, although I haven't added it to the CI config.

Problems

It's currently broken.

  • The test works by loading the same glTF multiple times, each time changing GltfLoaderSettings::override_sampler.
  • That falls afoul of Gltf assets do not reload correctly #18267.
  • I've confirmed that the test does work if the assets are duplicated. This can be reproduced in the PR branch by syncing to faefb8dd.

Combining two tests into one is debatable.

  • Testing GltfLoaderSettings::override_sampler is convenient and provides a test case for Gltf assets do not reload correctly #18267.
  • But makes it harder to isolate bugs.
  • And gltf tests should aim to be headless.
  • Maybe this is ok for now but gets refactored later.

CI screenshot boilerplate.

  • Screenshot supports feels a bit boilerplate-y. It's fairly similar to testbed/helpers.rs.
  • My guess is that this whole test will eventually get replaced by a BSN scene.
  • Or maybe the CI testing could have a helper that queues up N screenshots and sends an event for each one.

Testing

cargo run --example gltf_override_sampler
cargo run --example test_mipmap_filter --features bevy_ci_testing

Tested on Win10/Nvidia, across Desktop/Vulkan, Chrome/WebGL, Chrome/WebGPU.

@greeble-dev greeble-dev added A-Rendering Drawing game state to the screen C-Testing A change that impacts how we test Bevy or how users test their apps S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-glTF Related to the glTF 3D scene/model format labels May 6, 2025
@greeble-dev greeble-dev added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label May 6, 2025
@alice-i-cecile
Copy link
Member

The test is not currently suitable for screenshot comparison.

We should try and fix this.

Should this test be framed as a glTF test or a renderer test?

Renderer test if we're going to be doing rendering and screenshots. Like you say, gltf loading should be tested purely in headless code.

@greeble-dev greeble-dev added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 12, 2025
@greeble-dev greeble-dev changed the title Add test for GltfLoaderSettings::override_sampler and mipmap rendering Add test for mipmap filters May 13, 2025
@greeble-dev
Copy link
Contributor Author

Test has been redone to focus on rendering - glTF testing is now just a bonus. Also added CI support.

In the longer-term I'm guessing this test should be redone as BSN scenes, and the glTF part would be split off and made headless. But I'm hoping the current choices are acceptable for the short-term.

@greeble-dev greeble-dev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-glTF Related to the glTF 3D scene/model format A-Rendering Drawing game state to the screen C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants