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 incomplete loading while minimized under some windowing systems. #6617

Conversation

brettle
Copy link
Contributor

@brettle brettle commented Aug 8, 2024

Description

Under some window systems (notably linux wayland), QWindow::isExposed() always returns false if the window is minimized (e.g. when running tests). This was preventing the initial non-culling call to renderNow() from accomplishing its goal of ensuring that meshes/textures are loaded onto the GPU. (This resulted in the skin test failing because the aabb for the skin was not calculated using the skeleton.) This fixes that by allowing the non-culling call to proceed even if the window is not exposed. It also suppresses the resulting errors/warnings only for that call because several of the parsing tests fail if the stderr output is not empty.

Tasks
Add the list of tasks of this PR.

@brettle brettle requested a review from a team as a code owner August 8, 2024 22:05
@brettle brettle added the test suite Start the test suite label Aug 8, 2024
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

I am afraid that disabling all GL errors in the CI tests (under wayland) might open the door to some tests passing whereas they should actually fail. Why not disabling minimization in the CI tests?

src/webots/gui/WbView3D.cpp Outdated Show resolved Hide resolved
docs/reference/changelog-r2024.md Outdated Show resolved Hide resolved
src/webots/gui/WbView3D.cpp Outdated Show resolved Hide resolved
@brettle
Copy link
Contributor Author

brettle commented Aug 9, 2024

I am afraid that disabling all GL errors in the CI tests (under wayland) might open the door to some tests passing whereas they should actually fail. Why not disabling minimization in the CI tests?

It's useful to run the CI tests locally while doing other work simultaneously, so I'd really rather not disable minimization. Moreover, if the tests (other than the "with-rendering" tests) fail while minimized, that is arguably a bug.

Note that the suppression is only for the initial post-load rendering pass. Any test which does more than one timestep will do at least one rendering pass without suppression and if the test requires no error output it will still fail.

If that doesn't address you concern, would it be addressed by only doing the suppression if some env var is set and setting that env var in test_suite.py? If so, let me know if you have a preferred env var name.

@brettle
Copy link
Contributor Author

brettle commented Aug 9, 2024

I could also not set the env var in test_suite.py and instead output a message mentioning it when we detect the situation where it might be useful. Something along the lines of "WARNING: Webots will print errors/warnings because your windowing system does not support rendering to minimized windows. To suppress those, set WEBOTS_QUIET_MINIMIZED_RENDERING=true in your environment."

@brettle
Copy link
Contributor Author

brettle commented Aug 9, 2024

Thinking about this some more, I'm wondering if instead of this PR, we should just make the initial non-culling render target a FBO instead of the window's framebuffer. I think that would succeed without errors/warnings even if the window was not exposed. Thoughts?

@omichel
Copy link
Member

omichel commented Aug 9, 2024

Thinking about this some more, I'm wondering if instead of this PR, we should just make the initial non-culling render target a FBO instead of the window's framebuffer. I think that would succeed without errors/warnings even if the window was not exposed. Thoughts?

Yes, if that works, it would be ideal.

@brettle brettle requested a review from omichel August 13, 2024 15:37
@brettle
Copy link
Contributor Author

brettle commented Aug 13, 2024

I am afraid that disabling all GL errors in the CI tests (under wayland) might open the door to some tests passing whereas they should actually fail. Why not disabling minimization in the CI tests?

Latest commit no longer disables error reporting.

@brettle
Copy link
Contributor Author

brettle commented Aug 13, 2024

Thinking about this some more, I'm wondering if instead of this PR, we should just make the initial non-culling render target a FBO instead of the window's framebuffer. I think that would succeed without errors/warnings even if the window was not exposed. Thoughts?

Yes, if that works, it would be ideal.

Afaict, it was already rendering to an FBO, so I mostly just needed to ensure that it would still render if not exposed. The only things that were triggering errors were blitting of the render to the screen, swapping buffers, and clearing the default framebuffer during postprocessing. The first 2 were easy to disable for the initial render. The clearing of the default framebuffer during postprocessing doesn't appear to be needed (and seems like it could cause other problems) so this removes it.

Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

That looks much better this way to me.
Thank you.

@brettle brettle merged commit 0bcfb17 into cyberbotics:master Aug 14, 2024
22 checks passed
@brettle brettle deleted the fix-incomplete-loading-under-some-windowing-systems branch August 14, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

2 participants