-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Shader files not loading in 1.7 #9354
Comments
I did see that issue, but I decided to post a separate one because they're using 1.6, which has no problems for me, and they're also experiencing an error message, which I'm not getting even if use a non-existent path (because it just becomes an empty string, I guess). Could still be related though? ¯\_(ツ)_/¯ |
Yea, after re-reading #9338, they do definitely seem different. The warning dialog in that one's really throwing me for a loop. I mean, this one's crazy too. They're both crazy, just, each their own unique kind of crazy 😄 |
@PankajBhojwani Check this out! TerminalSettings.h:117
But look at all the other settings ...
When we create a new TerminalSettings leaf object, it has no PixelShaderPath because it is not an inheritable setting. The danger of macros with a bunch of identical names. |
Maybe we should |
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Fix for #9354 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #9354 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here
As mentioned in #9354 (comment) `GETSET_SETTING` is too visually similar to `GETSET_PROPERTY`, but with a _VERY_ different meaning. I think that merely changing the name of the macro would make it harder for us to make this mistake again.
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Fix for #9354 <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist * [x] Closes #9354 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I work here (cherry picked from commit 5aaf5b4)
🎉This issue was addressed in #9371, which has now been successfully released as Handy links: |
🎉This issue was addressed in #9371, which has now been successfully released as Handy links: |
Environment
Steps to reproduce
experimental.pixelShaderPath
on a profile (or globally) such that it points to an appropriately structured HLSL file (such asInvert.hlsl
from the samples folder)Expected behavior
The pixel shader is loaded and executed.
Actual behavior
The pixel shader is not loaded.
Comments
I have not been able to load any shader files at all in 1.7 from the official release, nor when I build it from source. Poking around in the debugger, I can see that the shader file path is definitely there when the JSON is read in:
However it seems to disappear at some point between being read and sent to
SetPixelShaderPath()
. I'm not sure if it's being deserialized correctly because I don't really understand how I'm supposed to inspect the settings object; it doesn't behave the way I'd expect it to (just C++ things, I assume).release-1.7
release-1.6
I've tried this on other machines that have never had any version of the Terminal installed and they all exhibit the same issue.
The text was updated successfully, but these errors were encountered: