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.

Sign up for GitHub

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

Jump to bottom

User controlled window visibility #9355

Merged
merged 1 commit into from Aug 20, 2023
Merged

User controlled window visibility #9355

merged 1 commit into from Aug 20, 2023

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Aug 4, 2023 •
edited

Objective

  • When spawning a window, it will be white until the GPU is ready to draw the app. To avoid this, we can make the window invisible and then make it visible once the gpu is ready. Unfortunately, the visible flag is not available to users.

Solution

  • Let users change the visible flag

Notes

This is only user controlled. It would be nice if it was done automatically by bevy instead but I want to keep this PR simple.

@IceSentry IceSentry added A-Windowing Platform-agnostic interface layer to run your app in C-Enhancement A new feature C-Examples An addition or correction to our examples labels Aug 4, 2023
@hymm
Copy link
Contributor

hymm commented Aug 10, 2023

I still see a short white flash (1 frame?). Definitely shorter than without the code. I'm guessing it's probably because render hasn't run once yet. Might be better to use a run criteria to run only the second time the system is called?

@IceSentry
Copy link
Contributor Author

Yeah, it might be better to do it after Startup. Let me try that.

@IceSentry
Copy link
Contributor Author

I've tried putting it in PostStartup, an Update system that waits for 5 seconds and spawning a camera but for all of them I still get a 1 frame flash. I'm not entirely sure if that's something that can be fixed user side or even in bevy. Personally, I still prefer this over the current status

hymm
hymm approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Agree that what's shown in the example is better than the status quo.

@hymm
Copy link
Contributor

hymm commented Aug 11, 2023

looking at this issue looks like it might not be fixable vulkano-rs/vulkano#2263.

Looking on some of the applications on my windows machine I notice a white flash on some when starting up (ldtk, firefox), but others seem to avoid it somehow (all the built in ones at least). So there must be some way to avoid it, but probably not worth worrying about for now.

@mockersf
Copy link
Member

I don't think this is worth a dedicated example. Could you merge it with example window_settings?

@IceSentry
Copy link
Contributor Author

I did it that way because I saw multiple people asking for the feature, but also I didn't realize the window_settings example was a thing so I'll merge them.

@IceSentry
Copy link
Contributor Author

Could you merge it with example window_settings?

Done.

@IceSentry IceSentry mentioned this pull request Aug 14, 2023
Open
@Selene-Amanita Selene-Amanita self-requested a review August 14, 2023 06:54
Selene-Amanita
Selene-Amanita reviewed Aug 18, 2023
View reviewed changes
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/winit_windows.rs Outdated Show resolved Hide resolved
@Selene-Amanita Selene-Amanita added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 18, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@IceSentry
Copy link
Contributor Author

@Selene-Amanita why do you consider this breaking? The default behaviour is still the same and it's only adding a new property

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@Selene-Amanita
Copy link
Member

@Selene-Amanita why do you consider this breaking? The default behaviour is still the same and it's only adding a new property

Well it's technically a change in API and also technically someone that doesn't use the struct update syntax would need to add the field, but yeah realistically it's a very minor change and probably doesn't need a Migration guide or anything.I forgot that the bot asked for it I didn't want to be passive aggressive ^^'

@Selene-Amanita Selene-Amanita removed the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 18, 2023
@IceSentry
Copy link
Contributor Author

Oh no, don't worry I didn't interpret it that way, I just didn't understand what was breaking. I guess adding a new public field could indeed break a build so I'll add a migration guide. Better safe than sorry.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@mockersf
Copy link
Member

could you revert your changes on the root Cargo.toml file? I think it's better to not put unrelated formatting in a PR

@IceSentry
Copy link
Contributor Author

Done. Sorry, I thought I had already removed those changes😅. I've disabled toml formatting for bevy's repo so hopefully this doesn't happen again.

@mockersf mockersf added this pull request to the merge queue Aug 20, 2023
Merged via the queue into bevyengine:main with commit 49bbbe0 Aug 20, 2023
20 checks passed
@ameknite ameknite mentioned this pull request Aug 23, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Aug 23, 2023
@ameknite
fix bevy imports. windows_settings.rs example (#9547)
427ba30 
# Objective

In #9355 was added an import using bevy_internal.
This change the import to use `bevy::window` instead of a
`bevy_internal` to run the example outside of the bevy repo.
@IceSentry IceSentry deleted the window-visible branch August 25, 2023 01:51
@cart cart mentioned this pull request Oct 13, 2023
Merged
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hymm hymm hymm approved these changes

@mockersf mockersf mockersf approved these changes

@Selene-Amanita Selene-Amanita Selene-Amanita approved these changes

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Enhancement A new feature C-Examples An addition or correction to our examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@IceSentry @hymm @mockersf @Selene-Amanita