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

Proposed RFC Feature =Shadow Enhancement for FullscreenBlur and fix polygon light bug= #130

Open
Zeang opened this issue Mar 24, 2023 · 6 comments
Labels
rfc-feature Request for Comments for a Feature

Comments

@Zeang
Copy link

Zeang commented Mar 24, 2023

Shadow Enhancement for FullscreenBlur

Summary:

Refactor the pixel shader and compute shader for full-screen shadow blur; Disable the fullscreen blur pass if don't use it.

What is the relevance of this feature?

Fix some problems of shadow and light.

Feature design description:

The directional shadow has the fullscreen blur pass to improve the shadow effect. However, whether we use it or not, it will cost some performance.
What's more, the code of compute shader doesn't work correctly, so I fixed it. And compute shader has a little bit faster than the pixel shader for fullscreen blur.
The polygon light only has one quad effect, it's not right. So I enable another 4 vertexes.

Technical design description:

  1. Here is the Pass Viewer we can see Fullscreen Shadow pass is enabled whether we use it or not.
    pic2

We use directionalLightFeatureProcessor to control it. And disable the pass if don't need full-screen blur which average costs 62us per directional light.
3. I refactor the shader code for the user who wants to use compute shader or pixel shader. And extract the common code to FullscreenShadowCommon.azsli. As you can see:
Snipaste_2023-03-23_20-17-24

If the user wants to change the shader for rendering. Just change the macros in DirectionalLightFeatureProcessor and ShadowParent.pass. Before that, the code of compute shader was wrong.

What are the advantages of the feature?

Save the cost for every directional Light.
Fix the bug with compute shader of the fullscreen blur pass.

What are the disadvantages of the feature?

The Fullscreen blur pass's default option is compute shader.

How will this be implemented or integrated into the O3DE environment?

It will be integrated into Atom and AtomLyintegration gems.

Are there any alternatives to this feature?

As we talked about above, users can choose pixel shader for full-screen blur, it was supported by more devices. and the user can choose to compute shader for full-screen blur, which costs less performance.

How will users learn this feature?

It is an enhancement, just use it.

@Zeang Zeang added the rfc-feature Request for Comments for a Feature label Mar 24, 2023
@Zeang Zeang changed the title Proposed RFC Feature =description= Proposed RFC Feature =Shadow Enhancement for FullscreenBlur and fix polygon light bug= Mar 24, 2023
@invertednormal
Copy link

+1 on the fullscreen shadow changes. Would love to see shared local memory in use to improve the performance in compute.

Any more details on the polygon light? I implemented the original a few years back and I know it worked correctly at the time, but it's possible there's been some regression.

@Zeang
Copy link
Author

Zeang commented Mar 28, 2023

Well, it is not a big change. Do you use the polygon light by other mesh or default mesh, a cube in Editor. If you use default mesh, you will find the change of height will don't change the effect, because in C++ o3de doesn't add 8 vertexes for GPU. I add another 4 vertexes for this. It's a little change.

@invertednormal
Copy link

Well, it is not a big change. Do you use the polygon light by other mesh or default mesh, a cube in Editor. If you use default mesh, you will find the change of height will don't change the effect, because in C++ o3de doesn't add 8 vertexes for GPU. I add another 4 vertexes for this. It's a little change.

The polygon height is not supposed to have any affect on the light, polygon lights are only for flat polygons. I think the light explicitly sets the height of the polygon prism to 0 but there's not currently a way to lock it at 0. It uses the polygon prism shape because it already existed and was close enough for what was needed, but ideally the user wouldn't even see a height parameter on that component when used for a light.

The only way you could possibly support height in the polygon prism would be to add a separate individual light for each plane of the geometry, which would be prohibitively expensive, it's just not what they're made for. Just adding more vertices won't work. See https://eheitzresearch.wordpress.com/415-2/ for more info on the technique.

@Zeang
Copy link
Author

Zeang commented Mar 29, 2023

All right, so I will cancel the change of polygon light.

@Zeang
Copy link
Author

Zeang commented Apr 11, 2023

o3de/o3de#15665

@moudgils
Copy link
Contributor

moudgils commented Apr 12, 2023

I am doing a pass on RFCs. Once a RFC is accepted (which this is as you have already done the work) it needs to go into this folder (https://github.com/o3de/sig-graphics-audio/tree/main/rfcs). Are you able to move this rfc to this folder so that we can track this as a RFC and not as an issue? You will have to open a PR to move this into the rfc folder. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc-feature Request for Comments for a Feature
Projects
None yet
Development

No branches or pull requests

3 participants