-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Create xr full screen effects doc page #10727
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
Conversation
Personally i feel we shouldn't be talking about post processing here. Post processing effects suggest that you modify the rendered output and that is where the big no-no's come in. Vignettes' etc are not post processes. They are screen space effects that use a quad to render their output (actually a single triangle would be better but we don't have an easy primitive for that). So I think we need to come up with better naming. In the same line, I don't think that referencing the original post processing document is a good idea. Again, we just set people on the wrong footing, they will read that, figure out they can do that in XR as well, and then when they are too far along find out that doing so is what is causing their performance to never get up to spec. By then it's too late. |
@BastiaanOlij understood, and now that I understand the term a bit better I fully agree! @dsnopek had mentioned something similar to me elsewhere. I'll rework this soon to swap that term out for something like "full screen effects" and also remove the reference to the other doc. |
@devloglogan it does feel like nitpicking but I feel we can prevent ourselves a bit of hardship if we get that bit right :) |
5eacb0c
to
8847300
Compare
Alright, I've opted for "full screen effects" as the term instead and removed the reference to the other doc entirely (which adds a touch more on the basic full screen quad setup). |
8847300
to
68e234e
Compare
@dsnopek Thanks for the review! I believe I've addressed all of your suggestions, please take another look at the doc when you get a chance. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is looking good to me now :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammer and spelling should be good after these two changes. @BastiaanOlij did you want to look this over again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice writeup Logan! Indeed just need the small grammar fixes and its ready to merge IMHO.
68e234e
to
58a5b50
Compare
Thanks! |
Adds a doc page informing on how users can take asymmetric FOV into account in full screen quad
post-processingshaders.@BastiaanOlij mentioned that attempting to do this while reading from the screen texture is a big no-no so I also included a section at the bottom attempting to warn users about that.