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

Fix bunnymark test screenshot and replace rand with nanorand #2746

Merged
merged 13 commits into from
Jun 10, 2022
Merged

Fix bunnymark test screenshot and replace rand with nanorand #2746

merged 13 commits into from
Jun 10, 2022

Conversation

stevenhuyn
Copy link
Contributor

@stevenhuyn stevenhuyn commented Jun 7, 2022

Connections
Closes Bunnymark Screenshot is Empty Screen #1557

Description
Bunnymark screenshot right now is an empty screen because that example requires you to press spacebar.

Also replacing rand dependency with smaller nanorand dependency

unsafe seems to be required because that seems to be how it's meant to be used to generate a DeviceID. https://docs.rs/winit/latest/winit/event/struct.DeviceId.html#impl

KeyboardInput: https://docs.rs/winit/latest/winit/event/enum.WindowEvent.html#variant.KeyboardInput

Adds a check if it is the bunnymark test and then gives the example a spacebar event and steps a couple of frames.

Testing
It is the change

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the PR and the thorough description!

wgpu/examples/framework.rs Outdated Show resolved Hide resolved
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) June 7, 2022 06:13
@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 7, 2022

Failing because of:

error: use of deprecated field `winit::event::KeyboardInput::modifiers`: Deprecated in favor of WindowEvent::ModifiersChanged
   --> wgpu\examples\conservative-raster\..\framework.rs:585:25
    |
585 |                         modifiers: winit::event::ModifiersState::empty(),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

I'm not sure what you're meant to use instead of it, the type rqeuires ModifiersState, not ModifiersChanged.

I believe it's still not implemented
rust-windowing/winit#1151

auto-merge was automatically disabled June 7, 2022 06:38

Head branch was pushed to by a user without write access

@cwfitzgerald
Copy link
Member

I would just silence the warning for that struct init. We don't have a choice it seems.

@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 7, 2022

Also just checked, the color of the bunny changes per run, checking it out now. So screenshot failures too. Oops!

Uses a random function to determine color

edit: made it deterministic

@stevenhuyn stevenhuyn marked this pull request as draft June 7, 2022 07:07
@stevenhuyn stevenhuyn marked this pull request as ready for review June 7, 2022 07:09
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Good stuff

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) June 7, 2022 17:03
@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 7, 2022

Wait I think StdRng underlying implementation is chosen depending on the platform. Based on what I saw in the coode.

https://rust-random.github.io/rand/rand/rngs/struct.StdRng.html

image

auto-merge was automatically disabled June 7, 2022 19:11

Head branch was pushed to by a user without write access

@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 7, 2022

I have followed their suggestion and am now using ChaChaRng for reproducibility. But we have now added a dependency.

@stevenhuyn stevenhuyn marked this pull request as draft June 7, 2022 19:23
@stevenhuyn stevenhuyn marked this pull request as ready for review June 7, 2022 19:51
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Looks good outside of the dependency, let's nix the dep on rand at all, and just use nanorand's WyRand

@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 7, 2022

Looks good outside of the dependency, let's nix the dep on rand at all, and just use nanorand's WyRand

Hmm looks like a lot of the other examples use rng to an extent, (Not sure why we are seeing a fail for bunnymark only though)

Edit: done although there was a Rand wasm feature in the cargo.toml that I'm not sure what is for.

@stevenhuyn stevenhuyn changed the title Fix bunnymark test to actually show bunnies in screenshot Fix bunnymark test screenshot and replace rand with nanorand Jun 7, 2022
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

One touch, but looks good after that, thanks for sticking with this

wgpu/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Connor Fitzgerald <[email protected]>
@cwfitzgerald
Copy link
Member

Hmm, I'll take a look on my windows machine in a bit.

@stevenhuyn
Copy link
Contributor Author

Hmm, I'll take a look on my windows machine in a bit.

I'm actually using x86_64 Windows and it passes on my machine so no idea.

@cwfitzgerald
Copy link
Member

Try running with $env:WGPU_ADAPTER_NAME='basic' to run on the same software adapter that CI is running

@stevenhuyn
Copy link
Contributor Author

image
Nice

@cwfitzgerald
Copy link
Member

lmao, mark the test as failing on that device .specific_failure(None, None, Some("Basic"), false) added to the test parameters and lets get this out the door.

@stevenhuyn
Copy link
Contributor Author

stevenhuyn commented Jun 8, 2022

Okay now we start seeing water panicing on basic mode only:

Doesn't even generate a screenshot.

I can also definitely run the example.

Is it even worth it?

running 1 test
test water ... FAILED

failures:

---- water stdout ----
117 outliers over max difference 166
thread 'water' panicked at 'UNEXPECTED TEST PASS: BACKEND | ADAPTER', wgpu\examples\water\..\..\tests\common\mod.rs:305:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@cwfitzgerald
Copy link
Member

That's hilarious, you don't get a screenshot because it expects to fail but it passes (CI fails when tests unexpectedly pass so the tests stay updated).

Thanks for this PR and sorry for the hassle, I'll sort this PR out in the morning, need to double check some stuff.

@stevenhuyn
Copy link
Contributor Author

nws, thank you for your work!

@cwfitzgerald
Copy link
Member

@stevenhuyn what OS do you have? Windows 11?

@stevenhuyn
Copy link
Contributor Author

ye win 11

@cwfitzgerald
Copy link
Member

Yeah, that'd do it, so the water example is fixed on windows 11's warp, but not on windows 10's warp. We currently have no way of representing that in the code right now....

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) June 10, 2022 21:32
@cwfitzgerald cwfitzgerald merged commit df1472d into gfx-rs:master Jun 10, 2022
@stevenhuyn
Copy link
Contributor Author

Woo!

@cwfitzgerald
Copy link
Member

Filed #2760 as a follow up.

@stevenhuyn stevenhuyn deleted the bunnymark-screenshot branch June 10, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bunnymark Screenshot is Empty Screen
2 participants