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

Implement WindowEvent::Minimized and WindowEvent::Restored for Windows #3200

Closed
wants to merge 1 commit into from

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Oct 30, 2023

  • Tested on all platforms changed
    • I only tested this change on Windows since the implementation affects only Windows
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

This PR partially implements #1578 by adding new window events WindowEvent::Minimized and WindowEvent::Restored.

@rhysd rhysd requested a review from msiglreith as a code owner October 30, 2023 04:23
@daxpedda
Copy link
Member

daxpedda commented Oct 30, 2023

See #2929. I'm not sure how many attempts this is now.
I guess #2334 is the original issue.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 31, 2023

I haven't noticed the PR though I tried searching PRs with some words. I have two questions about the PR:

  • How does it cover the Restored event in this PR?
  • Is the PR active? There has been no comment for 4 months.

@daxpedda
Copy link
Member

daxpedda commented Oct 31, 2023

  • How does it cover the Restored event in this PR?

The WindowState tells you if the Window is minimized or not. You will receive a WindowState every time anything changes.

  • Is the PR active? There has been no comment for 4 months.

Apparently not. I was just leaving it here for reference. E.g. the way you implemented it is not how we used to intend to implement it. I would prefer to go down the route of something more comprehensive like WindowState instead of creating more and more events. But I also don't want to hold development because we don't have the manpower to finish up more comprehensive PRs like that.


Let's just wait and see what other maintainers have to say about this.

Imo we should just proceed here, if/when this merges, I will also make a PR to add Web support. We can always re-discuss merging these events into something like WindowState later, instead of having stale PRs.

@kchibisov
Copy link
Member

I think adding minimized events is not really a good idea. We already have Occluded if it matters and it should be used to manipulate rendering instead. Minimized is just misleading most of the time and it doesn't mean that you shouldn't draw anymore.

@daxpedda
Copy link
Member

I think adding minimized events is not really a good idea. We already have Occluded if it matters and it should be used to manipulate rendering instead.

We had a similar discussion before here: #2980.

I still don't understand why this remains a bad idea. I completely agree that Occluded should be used to figure out rendering, but these two things are not the same. If a window is minimized or not is completely separate information that might still be useful for users to know.
E.g. some programs might stop running when they are minimized, but will continue running if they are just in the background.

Minimized is just misleading most of the time and it doesn't mean that you shouldn't draw anymore.

Would you mind elaborating on that?

@kchibisov
Copy link
Member

Would you mind elaborating on that?

Minimize doesn't mean that you don't have to render, because compositor could show a preview. Like the preview you have when however over application in windows bar.

@daxpedda
Copy link
Member

daxpedda commented Oct 31, 2023

Would you mind elaborating on that?

Minimize doesn't mean that you don't have to render, because compositor could show a preview. Like the preview you have when however over application in windows bar.

Yeah, that's a more complicated problem though and honestly makes information like that even more important.
If we don't intend to send Occluded when a window is minimized (which I find would need some discussion), it is still good to know as a user that the window is minimized, e.g. pause the game, render at lower FPS.
If we do want to send Occluded, users might still want to know if the window is minimized for the reasons pointed out above.

@kchibisov
Copy link
Member

Yeah, that's a more complicated problem though and honestly makes information like that even more important.
If we don't intend to send Occluded when a window is minimized (which I find would need some discussion), it is still good to know as a user that the window is minimized, e.g. pause the game, render at lower FPS.
If we do want to send Occluded, users might still want to know if the window is minimized for the reasons pointed out above.

The thing is that compositor can send Occluded for minimize as well, but then it'll send Occluded(false) when you try to preview.

@daxpedda
Copy link
Member

The thing is that compositor can send Occluded for minimize as well, but then it'll send Occluded(false) when you try to preview.

Ah alright, nice!

@kchibisov
Copy link
Member

The thing is that only macOS/Wayland has this event delivered reliably iirc. On Web it should be as well. And that's the only event you can trust to pause rendering.

@daxpedda
Copy link
Member

The thing is that only macOS/Wayland has this event delivered reliably iirc. On Web it should be as well. And that's the only event you can trust to pause rendering.

Sure, I mean we can add this kind of information in the documentation. I'm surprised this doesn't work reliably on Windows (though I haven't used Windows in a very long time now).

Pausing rendering should remain in Occluded anyway, even if we send a Minimized event, we should document that this doesn't mean the user should pause rendering, we should send an Occluded event for that separately.

@kchibisov
Copy link
Member

even if we send a Minimized event, we should document that this doesn't mean the user should pause rendering

yeah, but we have this sort of information on is_minimized already, you just don't have a notification when you're getting minimized, though you can still sort of check during event loop handling whether you're minimized or not.

I just don't want to add more events...

@rhysd
Copy link
Contributor Author

rhysd commented Oct 31, 2023

@daxpedda @kchibisov Thank you for the further comments. I understood the situation and the Occluded event sounds sufficient for my use case. I'll wait on the PR.

@rhysd rhysd closed this Oct 31, 2023
@kchibisov
Copy link
Member

Occluded works on web.

@kchibisov
Copy link
Member

Ah, no, you were adding for Windows. Windows doesn't have a notion of Occluded...

@rhysd
Copy link
Contributor Author

rhysd commented Oct 31, 2023

Ah, I see. Then I think I need to wait for #2929.

@rhysd
Copy link
Contributor Author

rhysd commented Oct 31, 2023

I may not get the discussion well. Should I modify this branch as follows?

  • send Occluded(true) on wparam == SC_MINIMIZE
  • send Occluded(false) on wparam == SC_RESTORE

@kchibisov
Copy link
Member

No, you shouldn't. Minimize != Occluded.

You need something like that https://learn.microsoft.com/en-us/windows/win32/api/dxgi1_2/nf-dxgi1_2-idxgifactory2-registerocclusionstatusevent or https://learn.microsoft.com/en-us/windows/win32/direct3ddxgi/waiting-when-occluded

@rhysd
Copy link
Contributor Author

rhysd commented Oct 31, 2023

Thanks. I didn't know this D3D API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants