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

Shaderc: Make non-semantic debug optional #11861

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

kamfretoz
Copy link
Contributor

Description of Changes

This PR drops the hard requirement and the dependencies on the patched shaderc (so you can build PCSX2 without the patch if you choose to do so), whilst still reserving the patch for those who needs/wants it (non-semantic debug changes).

Rationale behind Changes

This should go along nicely with the package mode.

Suggested Testing Steps

See if the CI passes and nothing else broke.

Copy link
Contributor

@weirdbeardgame weirdbeardgame left a comment

Choose a reason for hiding this comment

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

Was able to build a non patched pcsx2 on my end.

@kamfretoz
Copy link
Contributor Author

kamfretoz commented Oct 2, 2024

Quickly added a close menu hint to the savestate selector UI to let the user know they can do so.
Decided to put it into separate PR.

Don't you hate it when you come up with more ideas just minutes AFTER the PR has been merged?
F0bes
F0bes previously requested changes Oct 3, 2024
Copy link
Member

@F0bes F0bes left a comment

Choose a reason for hiding this comment

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

So, you've removed like 90% of the patch. The idea was to support the existing patch and an unpatched shaderc. Ideally you'd only change one line in the patch, the addition of #define SHADERC_PCSX2_CUSTOM 1.

You've removed shaderc_compilation_status_to_string and bunch of the shaderc_compile_into_xyz functions. If we're going this route a patched shaderc should be dropped outright. But that's not what I wanted to do.

@kamfretoz kamfretoz marked this pull request as draft October 4, 2024 07:04
@TellowKrinkle
Copy link
Member

TellowKrinkle commented Nov 23, 2024

So, you've removed like 90% of the patch. The idea was to support the existing patch and an unpatched shaderc. Ideally you'd only change one line in the patch, the addition of #define SHADERC_PCSX2_CUSTOM 1.

What's the point of the part of the patch they removed? From my perspective, all it seems to do is mess with the signatures of a bunch of methods to return a status code separately from the rest of the results, but that status code can be fetched from the shaderc_compilation_result_t anyways, so I don't see why it's important. It does however make it much more painful to support both shaderc at the same time, since now every method is different.

I guess you can now get an error code even if shaderc fails to even allocate a result struct, but that feels fairly unlikely and we'll still print a (fairly unique) "null result object" for that.

@F0bes
Copy link
Member

F0bes commented Nov 23, 2024

I trust your judgement on the issue. I was not expecting the original patch to be completely modified. In all honesty I can't confidently review the shaderc changes beyond if it compiles or not, hence my original expectation of it being relatively unchanged.

@TellowKrinkle
Copy link
Member

I re-added the result error code stringification, PCSX2-side this time, so it'll work regardless of what shaderc you use

@F0bes F0bes dismissed their stale review November 23, 2024 04:24

resolved

@kamfretoz kamfretoz marked this pull request as ready for review November 23, 2024 04:45
@F0bes F0bes removed the Hold Merge label Nov 27, 2024
@F0bes F0bes merged commit a71ea99 into PCSX2:master Nov 27, 2024
11 checks passed
@kamfretoz kamfretoz deleted the shaderc-neutering branch November 27, 2024 21:04
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.

5 participants