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

Enforce presence of all fragment shader inputs in Veldrid #5687

Merged
merged 22 commits into from
Jun 13, 2023

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Mar 19, 2023

This works around D3D11 not working on shaders that don't make use of all members in the fragment input / vertex output. It seems D3D11 naturally expects all vertex output members are used in the fragment shader, and its optimisation of culling out unused fragment input members seems to affect the vertex output / fragment input structure.

One solution was to design localised vertex structures that match the needs of the fragment shader, but that turned out to be not pretty as time went and more shaders required this workaround.

This solution, on the other hand, prevents D3D11 from culling out unused fragment input by adding each of them via regex and passing them in the output structure.

Tested to work with Parallels, with no major differences performance-wise.

Copy link
Contributor

@smoogipoo smoogipoo left a comment

Choose a reason for hiding this comment

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

Seems okay, just a few nits.

@smoogipoo
Copy link
Contributor

Must revert ppy/osu-resources#249

Can you explain why it "must" be reverted? As far as I understand this change should work regardless?

@frenzibyte
Copy link
Member Author

frenzibyte commented Mar 19, 2023

Since that PR requires osu!-side changes, it's better to revert it and close the osu!-side PR rather than the other way around.

Now that I look at it though, I thought the changes required osu! to use a different vertex type, but it seems the only changes required are just using the new vertex shader, so it's not that big of a deal.

@peppy
Copy link
Sponsor Member

peppy commented Jun 6, 2023

What's the status of this PR? Might be worth revisiting and closing if deemed unnecessary at this point, or bringing up-to-date.

@frenzibyte
Copy link
Member Author

frenzibyte commented Jun 7, 2023

It's still valid for merge and would avoid all the vertex shaders that were created specifically to work around the limitation resolved by this PR (e.g. sh_Blur.vs and sh_LogoAnimation.vs)

@peppy
Copy link
Sponsor Member

peppy commented Jun 7, 2023

👍

Needs some conflict resolution at very least then

@frenzibyte
Copy link
Member Author

Have updated this now, but do note that I haven't tested it on DirectX because compiling and running on a VM is kind of a pain for me. Testing and making sure blurred framebuffers renders correctly on DirectX would be appreciated.

@frenzibyte frenzibyte requested a review from peppy June 7, 2023 05:00
@smoogipoo smoogipoo merged commit 97d7157 into ppy:master Jun 13, 2023
17 checks passed
@frenzibyte frenzibyte deleted the output-fragment-input branch June 13, 2023 12:59
@bdach bdach mentioned this pull request Aug 21, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants