-
Notifications
You must be signed in to change notification settings - Fork 143
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
SergioRZMasson
merged 16 commits into
BabylonJS:master
from
SergioRZMasson:bugfix/multi-camera-rendering
Jan 17, 2023
Merged
Fixed issue when setting ViewPort multiple times per frame. #1183
SergioRZMasson
merged 16 commits into
BabylonJS:master
from
SergioRZMasson:bugfix/multi-camera-rendering
Jan 17, 2023
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bghgary
approved these changes
Jan 3, 2023
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. We should test XR and other scenarios are still working.
Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>
Working fine on Android XR. |
Working fine on IOS XR. |
b9b3b7c
to
a93c72e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 modifyBabylonNative::FrameBuffer
since in some situations theFrameBuffer::SetViewPort
gets called beforeFrameBuffer::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 callbgfx::setViewClear
and that information does not get lost if a newbgfx::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