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

Fixed Skybox rendering for OpenVR #196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Slamy
Copy link

@Slamy Slamy commented Apr 29, 2021

Rectangle2D supports a second set of normals.
Fixed world space corner calculation for head to eye matrices.

Also tested without instancing. Still works without OpenVR projects.
One thing: There is a TODO in the shader code which must still be fixed. But currently I have the feeling that you do know more about this.

Rectangle2D supports a second set of normals.
Fixed world space corner calculation for head to eye matrices.
@Slamy Slamy force-pushed the feature/vrSkyPr branch from 8c5024a to 6ac9fab Compare April 29, 2021 22:07
@darksylinc
Copy link
Member

darksylinc commented Apr 29, 2021

At a first glance it looks ok!

I still have to look in depth and test it myself though (and also some cosmetic changes like using spaces instead of tabs)

One thing: There is a TODO in the shader code which must still be fixed

I have yet to understand what it is doing, was the 3.5f found by trial and error or a mathematically correct value?

Update: Oh I see, IIRC you're using that line to "hide" the second instance?

@Slamy
Copy link
Author

Slamy commented Apr 29, 2021

The 3.5f was a number which needed to be high enough to hide the second instance. It doesn't have to be that value.
But it feels dirty. Even "in int gl_InstanceID;" shouldn't be required. I was just very unsure how I could prohibit the shader of being called a second time. I'm still very new to shader languages and this was a jump in cold water.
The RadialDensityMask does this as well. There has to be a way. But I really don't know. Or is the RadialDensityMask drawn 2 times on each eye? I hope not :-D

Edit: If there are comments lacking, please point those places out. I'll add them.
In general Ogre lacks comments at some internal places... But this is something every developer hears frequently, I guess.

@darksylinc
Copy link
Member

In general Ogre lacks comments at some internal places... But this is something every developer hears frequently, I guess.

Write them down and tell me so we add comments to those places. Indeed that is a problem and I often document internal code when someone mentions it

I was just very unsure how I could prohibit the shader of being called a second time. I'm still very new to shader languages and this was a jump in cold water.

That wouldn't be your fault, nor a shader language feature either. We in Ogre force instances to be x2 for stereo rendering (aka instanced viewport). That way we can send everything to both eyes with a single CPU pass (vertices still get processed twice though, but with better cache friendliness)
I'll have to look where that setting was and how to avoid it (or make best use of it); since I don't do VR very often.

@darksylinc
Copy link
Member

OK so I took a deeper look.

Overall very good! I can't believe how I missed all these simple little things.
Now I see why it wasn't working and what is going on.

I need to make a couple small fixes and merge it manually. If I don't do it by next week ping me again, I may forget.

Regarding the "TODO" hack... I've been thinking. Fixing it "properly" would involve a more complex fix, because either

  1. the sky needs to be rendered in its own pass so that instanced stereo is turned off (which is under user control, and I think it'd be user hostile to ask the user to deal with the compositor, if you're using SceneManager::setSky is because 'sky as a postprocess' wasn't for you) or
  2. Each Renderable can turn instanced stereo off, which would add a performance overhead to the whole engine because, even if you're not doing instanced stereo / VR at all, RenderQueue::renderGL3 would have to ask every Renderable if they want to disable instanced stereo

Given that this looks like an edge case where... 8 extra vertices are being rendered (that's literally nothing to a GPU), I think it's the best solution to just send them offscreen like you're doing so they don't burn pixel shader time.

@Hilarius86
Copy link
Contributor

If I don't do it by next week ping me again, I may forget.

;-)

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

Successfully merging this pull request may close these issues.

3 participants