Skip to content

Fixed issue when setting ViewPort multiple times per frame. #1183

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

Merged

Conversation

SergioRZMasson
Copy link
Contributor

@SergioRZMasson SergioRZMasson commented Dec 21, 2022

Overview

This PR fixes #1182, it moves viewport update from been done immediately to be done using the command queue. The way this is currently been done on master, if multiple viewport updates are done in a single frame (like when you can more than one camera been rendered using different viewports) the viewport updates actually end up overriding each other and get lost by the time the command queue starts been processed.

Solution

In order to fix this, I'm proposing moving the setViewPort operation to be performed in the command queue. This way, this operation is going to happen in sync with the other rendering commands like "FrameBuffer::Clear" (which also generates a bgfx::View). I also had to modify BabylonNative::FrameBuffer since in some situations the FrameBuffer::SetViewPort gets called before FrameBuffer::Clear and sometimes it gets called after. In the cases where it gets called after, it is required that we store the clear information in the FrameBuffer object so we can call bgfx::setViewClear and that information does not get lost if a new bgfx::View gets created.

This will also require a PR in Babylon.js to change how NativeEngine calls setViewPort. Babylon.js PR

Extra Information

This functionality started as a request in the forum:
Forum issue
Target web behaviour

@SergioRZMasson SergioRZMasson added the bug Something isn't working label Dec 21, 2022
@SergioRZMasson SergioRZMasson marked this pull request as draft December 21, 2022 18:50
Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

LGTM. We should test XR and other scenarios are still working.

@SergioRZMasson
Copy link
Contributor Author

Working fine on Android XR.

@SergioRZMasson
Copy link
Contributor Author

Working fine on IOS XR.

@SergioRZMasson SergioRZMasson marked this pull request as ready for review January 17, 2023 15:45
@SergioRZMasson SergioRZMasson force-pushed the bugfix/multi-camera-rendering branch from b9b3b7c to a93c72e Compare January 17, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing Viewport multiple times during rendering does not work properly
2 participants