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

Viewport out-of-range when first constructing certain UIs rendered with egui-wgpu callbacks #3759

Closed
msparkles opened this issue Jan 1, 2024 · 13 comments · Fixed by #3778
Closed
Labels
bug Something is broken egui-wgpu

Comments

@msparkles
Copy link
Contributor

Describe the bug

Viewport out-of-range, causing a validation error, when first constructing certain UIs rendered with egui-wgpu callbacks.
This only happens on the first frame, the fix would simply be skipping the callback and set viewport command when the viewport rect is outside of the display.

To Reproduce
Steps to reproduce the behavior:

  1. Make a Bottom panel with a scroll area inside
  2. Add egui-wgpu Callbacks to it

Expected behavior
The item to be skipped if the viewport is outside of display.

Additional context
https://github.com/automancy/automancy/blob/main/src/bin/automancy/gui/tile_selection.rs

(Remove the following from the code to actually get the panic:

        if !ui.ctx().screen_rect().contains_rect(rect) {
            continue;
        }

)

@msparkles msparkles added the bug Something is broken label Jan 1, 2024
@msparkles
Copy link
Contributor Author

This needs an egui fix because the scissor could be smaller than the viewport.

@emilk emilk added the egui-wgpu label Jan 2, 2024
@emilk
Copy link
Owner

emilk commented Jan 2, 2024

Is it the PaintCallback::rect that is wrong, or the clip rect, or both? What are their values compared to the viewport rect?

Why does it only happen on the first frame?

the fix would simply be skipping the callback and set viewport command when the viewport rect is outside of the display.

what about when it is partially outside the frame?

@msparkles
Copy link
Contributor Author

Actually, we just thought about it for a bit- the problem should be that the scissor isn't being correctly set..

@msparkles
Copy link
Contributor Author

msparkles commented Jan 2, 2024

Why does it only happen on the first frame?

Regarding this, we can only assume it's some placement negotiation oddities that's happening, and that's beyond our knowledge to really understand..

Here's a screenshot of that first frame

image

@msparkles
Copy link
Contributor Author

Gonna work on this right now and implement a fix. \o/

@msparkles
Copy link
Contributor Author

Okay, to recap everything:

  • Apparently there's a discussion? on this-- We came from Vulkan, we used to render outside of the render attachment and clip stuff with scissor, so that we can render half an object-- Viewport rect must lie entirely within the current attachment size? gpuweb/gpuweb#373 apparently this is an.... issue in wgpu?
  • Apparently there's still a minor bug in egui where it would try to set a viewport of width and/or height of 0, just plug this before the set viewport command- this is unrelated to this issue
if viewport_px.width_px <= 0 || viewport_px.height_px <= 0 {
    continue;
}
  • Part of this is an user error on our part- with that piece of code patched onto egui, the issue now is this:

image

Aka. The Thing Is Not Supposed to be Squished- but this is apparently wgpu's issue now?

@msparkles
Copy link
Contributor Author

CC: @cwfitzgerald You're the wgpu person right? :P

tldr: apparently WebGPU? says that, you should be able to set viewport to be outside of attachment, but wgpu doesn't allow that

@emilk
Copy link
Owner

emilk commented Jan 2, 2024

I think @Wumpf has thought about this before when we implemented a similar thing in Rerun

@Wumpf
Copy link
Collaborator

Wumpf commented Jan 2, 2024

Today, WebGPU specification requires to fail validation on the device timeline if the viewport is outside of the render targets, see https://www.w3.org/TR/webgpu/#dom-gpurenderpassencoder-setviewport so wgpu behaves correctly here.

With that am I correct that the only thing wrong on the egui side is that it should discard zero area viewports?
Not sure actually whether it should still invoke egui render callbacks 🤔, I guess it should.

@msparkles
Copy link
Contributor Author

Today, WebGPU specification requires to fail validation on the device timeline if the viewport is outside of the render targets

That is, rather annoying.. is there any other way to achieve this rendering then? Partially out of range and then clipped into the screen.

With that am I correct that the only thing wrong on the egui side is that it should discard zero area viewports?

Yeah.

Imo it should skip the whole callback, because if not updated it'll just use the older viewport, which is gonna override other stuff.

@cwfitzgerald
Copy link
Contributor

That is, rather annoying.. is there any other way to achieve this rendering then? Partially out of range and then clipped into the screen.

Intersection of your desired rect and the screen viewport rect.

@emilk
Copy link
Owner

emilk commented Jan 3, 2024

Shapes with empty clip rects are already pruned by the epaint tessellator before the results are handed to the rendering backend:

if !new_clip_rect.is_positive() {
return; // skip empty clip rectangles
}

@msparkles
Copy link
Contributor Author

In here..

if callback.rect.is_positive() {

The rect is checked during callback...

...but that rect isn't clamped to screen size yet.

msparkles added a commit to msparkles/egui that referenced this issue Jan 7, 2024
msparkles added a commit to msparkles/egui that referenced this issue Jan 7, 2024
emilk pushed a commit that referenced this issue Jan 8, 2024
<!--
Please read the "Making a PR" section of
[`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md)
before opening a Pull Request!

* Keep your PR:s small and focused.
* If applicable, add a screenshot or gif.
* If it is a non-trivial addition, consider adding a demo for it to
`egui_demo_lib`, or a new example.
* Do NOT open PR:s from your `master` branch, as that makes it hard for
maintainers to add commits to your PR.
* Remember to run `cargo fmt` and `cargo cranky`.
* Open the PR as a draft until you have self-reviewed it and run
`./scripts/check.sh`.
* When you have addressed a PR comment, mark it as resolved.

Please be patient! I will review your PR, but my time is limited!
-->

Closes #3759
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken egui-wgpu
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants