-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
OCULUS_multiview implementation w/ multisampling support #24048
Conversation
It seems the PR modifies some example files which are unrelated to this change (like |
Please also remove build and *.d.ts files. |
Do you know if this is something that will get fixed in the spec somehow? Ideally this is something that the renderer would just use when available and the developer shouldn't have to learn all these details and have to choose between higher CPU usage or being a frame late... |
I agree completely that I want this to be automatic and error free. From my understanding, fixing it at a spec level isn't trivial - you've basically got to pick your poison between slowdown and shimmering (with automatically handled resolves) or failed rendering (with discard on resolve). AFAICT, both options require changes to how three handles texture uploads to behave optimally. Because this is such a big win for experiences that can use it, I'm inclined to check it in behind a flag so folks can opt-in and see if the limitations are acceptable for their app. I'll also keep looking at how we can make it easier to use, both on the platform side and three.js, and see if I can find a generalized solution for everyone that doesn't involve the upload delay. |
@rcabanier Do you have any insights on this? |
It is unlikely that this will be fixed in the OpenGL spec. Just as with ext_multisampled_render_to_texture this was done on purpose because storing/loading and binding/unbinding are very costly on mobile hardware. That being said, when we switch to the Vulkan backend, we will have more control over loading and storing so we can fix it ourselves. |
Having multisampled multiview as an option will definitely be very helpful. It's hard to have complex immersive scenes on our limited hardware and this feature greatly helps code that is CPU limited. |
@cabanier Thanks! What about this other issue?
Is it possible to refactor If we are not able to implement this without the developer having to know about it I don't think this PR will get merged. |
Are you proposing that we detect problematic calls that cause flushes and then disable the extension (along with a warning)? |
No no. I was hoping @snagy could clarify what exactly breaks when using |
Whenever you do a texture upload or bind another framebuffer or anything else in the list in the link in the first post, it drops the framebuffer and nothing else draws to that buffer for the rest of the frame. The workaround in this PR (deferring texture uploads until the frame is complete) is the best thing I can come up with to fix most experiences without large changes to three. For a lot of apps (like the ball shooter examples), that's fine. They get a huge perf boost with no visual cost. The biggest problem with this workaround is that skeletal meshes upload their bone textures with TexImage, which is one of the prohibited operations. With the workaround, that means that all bone animation lags by one frame. If this PR lands, the next thing I would do is try to move the skinnedmesh processing and upload before the multiview scene begins rendering, so they don't interrupt the render but also aren't delayed. Beyond that, other textures could potentially be uploaded the same way - before the multiview scene loop begins. Stuff like movie textures are one frame delayed with the workaround, which could be fixed with some work. We have movies in our app but the frame delay on a movie is less consequential (for us) than the skinned mesh delay. The trickiest usage to fix will be applications that switch off to render another view (such as a mirror reflection) in the middle of the scene. We would also have to move this to the start of the frame so it doesn't interrupt rendering, and I'm not sure we can easily guarantee that without changing a lot of dev expectations on how three works. It's possible to fix this on a per-app basis as a dev, though - our app includes a camera that renders its own view of the scene, and we were able to render that before the multiview frame by just architecting our code properly. I'm skeptical that we could get it to a universal, always-on implementation until the browser and drivers allow more control over when the multisample resolve happens. (AFAIK this is possible once we're using Vulkan as our base API in the browser, instead of GLES). |
For good performance on mobile GPUs three.js should be doing these optimizations anyway; it's just more obvious if this extension is turned on (which is the reason they put this limitation in). |
ad3b94b
to
eef0acd
Compare
@snagy it looks like your patch still has conflicts |
I'm happy you didn't come by when I accidentally pushed 366 pre-existing commits into this branch. The force push was just patching up that mistake. |
Hey, this optimization is really useful, would be cool to have it available. |
This currently seems to not account for the skybox. It renders as a unit box centered on the camera, ie each face 0.5 units from the camera. |
Just to be clear, by skybox do you mean the scene.background? |
Sorry, yes. |
I finally got around to testing this and didn't see this problem - I made a scene with a cube map background and it rendered properly with and without multiview. It looks like that code change a bit, (in pr #24805 ) so it might have been fixed there. |
After some more testing, we are only seeing this in the Meta Quest Pro. We have updated our fork of this PR with the latest changes and threejs version, and the issue remains. Update: We have modified L70 in WebGLBackground.js to have a size of 10000 for all parameters. This seems to be good enough for now. |
I poked at this and thought about it a bit - I validated it was happening on my Quest Pro and then went back and saw the same behavior on my Q2. It's incredibly uncomfortable, so I'm not sure if somehow I didn't notice it originally or if something else was happening. I'll test this a few more times to see if there's more going on there. Anyway, I think the behavior on the quest pro is 'correct' - it was creating a 1 meter box and then moving it to match the camera origin each frame. Unfortunately for multiview, it's moving it to center on the spot between the cameras and then the multiview cameras still apply their view offsets when the box gets rendered, causing it to 'properly' offset per eye as though you've got a 1 meter box around your head. The two solutions are either a) special case how the projection happens so the background projection isn't affected by camera position or b) just make the box really big. (b) is the simpler option so I've updated this PR to include that. |
Related issue: #20368
This is a crack at reimplementing multiview for XR devices that support the OCULUS_multiview extension. This extension lets us use multiview along with MSAA.
This is a huge speed improvement for applications that are CPU limited and draw count bound. Even the VR ballshooter example runs at a higher framerate when shooting on the quest 2 with multiview enabled than without.
Unfortunately the extension has one big problem - the multisampled render to texture extension this uses will discard the frame buffer if other texture operations are used during rendering. This is outlined in KhronosGroup/WebGL#2912 . I've added a deferred upload of texture data (such as video frames) to mitigate this problem, by deferring uploads until after rendering of the main scene has finished. This adds a frame of latency to the texture uploads, and is only active if multiview is enabled.
Some apps (mostly ones that render stuff like real time reflections to a texture) won't work with this extension without refactoring when those renders happen, to ensure that they don't interrupt the main frame.
Most experiences will get a free perf gain from this extension, but some will get broken. Because of this, I do not enable multiview by default and instead added a 'multiviewStereo' flag to the renderer properties to enable it. If this flag isn't set, everything should work as before.