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

Vulkan child window buffer copy happens at the wrong point #790

Closed
pac85 opened this issue Jun 26, 2022 · 8 comments · Fixed by #792
Closed

Vulkan child window buffer copy happens at the wrong point #790

pac85 opened this issue Jun 26, 2022 · 8 comments · Fixed by #792

Comments

@pac85
Copy link
Contributor

pac85 commented Jun 26, 2022

Normally the copy is done in vkAcquireNextImageKHR ( relevant line ) but it would make more sense to do it after a call to vkQueuePresentKHR. This causes a bug in SeriousEditor (and potentially other programs) where the ui is displayed with a 1 event delay making it unusable (as well as showing uninitialized buffers whenever they are reinitialized). This is a quick fix I made.

Although less noticeable it could also cause a 1 frame input lag in games that make use of child window rendering, my patch should also fix it.

@pac85
Copy link
Contributor Author

pac85 commented Jun 27, 2022

I mistakingly made a PR to the repo with generated source code. In any case that PR also has a commit with a bit of cleanup.

@Tk-Glitch
Copy link
Member

Tk-Glitch commented Jun 27, 2022

In case you wanted to create a new PR:
The current patch can be found here: https://github.com/Frogging-Family/wine-tkg-git/blob/master/wine-tkg-git/wine-tkg-patches/misc/childwindow/childwindow-proton.patch .
The relevant line you linked seems to be based on wine 7.3.0, which might have used that patch instead: https://github.com/Frogging-Family/wine-tkg-git/blob/master/wine-tkg-git/wine-tkg-patches/misc/childwindow/legacy/childwindow.patch (which we don't use anymore on newer trees).

The current patch is a rebase of the Proton patch found here: ValveSoftware/wine@6e57f48

While I'm all fine integrating your changes here and grateful for your contribution, sending a PR for Wine's Proton's Wine integration sounds like a good idea to me. Is that something you'd be up to?

@pac85
Copy link
Contributor Author

pac85 commented Jun 27, 2022

In case you wanted to create a new PR: The current patch can be found here: https://github.com/Frogging-Family/wine-tkg-git/blob/master/wine-tkg-git/wine-tkg-patches/misc/childwindow/childwindow-proton.patch . The relevant line you linked seems to be based on wine 7.3.0, which might have used that patch instead: https://github.com/Frogging-Family/wine-tkg-git/blob/master/wine-tkg-git/wine-tkg-patches/misc/childwindow/legacy/childwindow.patch (which we don't use anymore on newer trees).

The current patch is a rebase of the Proton patch found here: ValveSoftware/wine@6e57f48

While I'm all fine integrating your changes here and grateful for your contribution, sending a PR for Wine's Proton integration sounds like a good idea to me. Is that something you'd be up to?

I assume by wine's proton you mean Proton's wine right?

I agree that it would be a good idea, I've ported my fix to proton, here is the commit ( new one ) and I'm gonna make a PR there as soon as I can. Do you think the comment I made for this issue would be appropriate for the PR? I guess I should give a more proper explanation of the issue it fixes.

EDIT: Made the pull request so I think this issue could be closed, unless it would still be useful if I made a PR on this repo too (perhaps after the PR gets reviewed ).

@Tk-Glitch
Copy link
Member

Yeah, I typo'ed, sorry. Fixed :D

Thank you very much for the PR in Valve's repo. Yeah it's still useful for sure, until your changes are merged on our side. It's up to you, as the author, to decide how you want the changes to be added. The code looks fine to me so I would merge it as is on our end considering we're also a testing playground, and you would have feedback pretty fast in case of issues. I'll let you decide what sounds best to you!

@pac85
Copy link
Contributor Author

pac85 commented Jun 28, 2022

Thank you for being so kind. I've made a PR to this repo. I wasn't sure on how to add my commit to the patch, I catted my patch to the end. Let me know if I need to do it differently (I was thinking of making it into a separate patch so it could be disabled if needed) and I'll edit the PR. Also let me know if I have to add the change to the other versions of the patch as well.

@Tk-Glitch
Copy link
Member

Thank you for being so kind.

Thank you for offering your time and efforts to the community!

That's perfect. That will allow for better history tracking! :)
Porting it over to the other version of the patch isn't necessary imho, as legacy trees aren't used much and we're currently using pretty large revert lists to make things play well with those proton fshack and vk childwindow patchsets so the current patch actually affects quite a few versions behind as well.

However, adding your changes to the staging fshack patchset (which contains pretty much the same implementation for vk child window rendering support) would be useful for sure (you can find it here: https://github.com/Frogging-Family/wine-tkg-git/blob/master/wine-tkg-git/wine-tkg-patches/proton/valve_proton_fullscreen_hack/valve_proton_fullscreen_hack-staging.patch). Please let me know if you want me to add your changes to that patch or if you're up for a second PR (or updating the existing one, as you please).

@pac85
Copy link
Contributor Author

pac85 commented Jun 28, 2022

Give me a moment and I'll update my PR

edit: It's taking a while to compile, might take an hour or so if ccache doesn't do it's thing.

@pac85
Copy link
Contributor Author

pac85 commented Jun 29, 2022

Made another commit. Thanks again, I'm glad to give my contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants