Skip to content

use rendergraph renderArea as scissor instead of viewport #1487

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

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

Conversation

theodoregoetz
Copy link
Contributor

In our application we frequently have views which are partially outside of the window's render area. In this case, the viewport is used to determine aspect ratio and should be allowed to go outside of the rendering area. The scissor is used to clip the rendered scene to the smaller visible region and should not change any rendering with respect to the camera.

The original code sets the viewport and scissor to the render graph's renderArea during record which ignores any potential difference. This PR fixes that to only set the scissor.

While this change fixes our immediate issue, I suspect there is a better solution that will be more general or more complete. I'm just not sure what's best.

This is rework of the effort to accomplish the same thing with older VSG structure around viewport state
See these PRs:

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected as it did before)

How Has This Been Tested?

In our application we have viewport states where the viewport is partially outside the window area. The scissor is then brought to be within the window area. The render graph's renderArea should then be the scissor and not the viewport when recording the render commands.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

viewport is used to determine aspect ratio and should be allowed to go outside of the
rendering area. The scissor is used to clip the rendered scene to a smaller visible
region and should not change any rendering with respect to the camera.
@AnyOldName3
Copy link
Contributor

You should be able to get the behaviour you want without modifying the VSG by just having an explicit vsg::View and setting its camera's viewport state appropriately. We definitely want to have the default setup keep the viewport and scissor in sync as that's what most applications will want.

@theodoregoetz
Copy link
Contributor Author

@AnyOldName3 That would require reversing the organization of the graph. Currently, we have the view(s) above and below the rendergraph nodes to make a multi-pass system with nested views, each with a two-pass OIT render graph in addition to the opaque render pass. In this way, we avoid unnecessary duplication of the view classes and share the camera members where we want (some views share projection, view and viewport, some share view and viewport, others only viewport).

A more universal solution might be to remove the renderArea member variable from vsg::RenderGraph since it's redundant with viewportState. Then during record, the render graph could just use the viewportState without modifying it. The clients would then be responsible for updating the viewportState on resizes. I think maybe the renderArea member variable is a hold-over from the days before VSG was using dynamic viewport state?

We could subclass RenderGraph and reimplement RenderGraph::accept(RecordTraversal) which would be fine except that this method is rather long and introduces a maintenance burden on the users that wish to do similarly.

@AnyOldName3
Copy link
Contributor

That use case might be better served with a single rendergraph for a renderpass with multiple subpasses, each of which has its own view. Renderpasses aren't particularly cheap, and I think subpasses are supposed to be cheaper if you can live with their limitations. I think the main one is that you can only access the same pixel from previous subpasses, so it wouldn't work for something like a blur, but for OIT, I wouldn't expect that to be a problem.

@robertosfield
Copy link
Collaborator

Is there an vsg example that illustrates the problem being tackled by this PR?

If not creating one then using that as testbed for trying out changes to the VSG that could enable the required functionality. Obviously keeping the rest of the VSG example and end user applications working with these changes is crucial too.

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