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

fix some post-processing issues #7460

Closed
wants to merge 7 commits into from
Closed

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Feb 1, 2023

Objective

fix a few post-processing issues

Solution

  • fxaa now respects viewport
  • tonemapping is only done on the final camera for a given target
  • upscaling writes to the input texture (if required) instead of the output texture if the active camera is not the final camera for a given target (stops post-processing and UI from being dropped for cameras that are not last with MSAA targets, or the last effect from being dropped for non-MSAA targets)

should fix #7435 and hopefully fix #5721 (haven't tested that)

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Feb 1, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 1, 2023
@Keelar
Copy link

Keelar commented Feb 1, 2023

I just tested it and can confirm that this does seem to fix #5721

@robtfm
Copy link
Contributor Author

robtfm commented Feb 1, 2023

i think there's still one issue remaining: if MSAA is enabled, a first camera renders with an odd number of post-process swaps, and then a second camera runs without doing an msaa resolve (which the UI camera doesn't currently, for example), then the last post-process step will be dropped.

in the current core pipeline this will only occur if you're using MSAA and FXAA at the same time, so I don't think it's a big problem.

i think making MSAA a node in its own right instead of part of the 2d/3d main pass would be the best way to fix this. this is useful anyway as some post effects may want to be pre-msaa resolve. but i'll do that in a separate PR.

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I tested it and can confirm that this fixes the original issue. I haven't tested with an odd number of viewports though.

Code makes sense to me, could extract the final_order to a shared fn, but it's not super important.

@robtfm
Copy link
Contributor Author

robtfm commented Feb 2, 2023

i just realised why i said on #7435 we needed to keep tonemapping on intermediate cameras and reverse-tonemap in the next camera. this pr would cause fxaa to run before tonemapping if it is not on the last camera...

@robtfm
Copy link
Contributor Author

robtfm commented Feb 2, 2023

closing in favour of #7474

@robtfm robtfm closed this Feb 2, 2023
@Keelar Keelar mentioned this pull request Feb 2, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post processing doesn't work correctly when using multiple viewports UI only shows if in last pass
4 participants