Skip to content

Conversation

kecho
Copy link
Contributor

@kecho kecho commented Sep 14, 2021

Purpose of this PR

Celstials lens flare was not position correctly when the camera was very far from the origin. This was caused because the relative position trick only works well when the camera is very close to a light source. If something is far enough it will incurr into small precision issues that might accumulate (due to the light position being so far away).

https://fogbugz.unity3d.com/f/cases/1363291/

Before:
image

After:
image


Testing status

Tested the fogbugz scene.


Comments to reviewers

@Unity-Technologies/gfx-qa-hdrp Change has been verified by Sean Puller

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions github-actions bot added the SRP label Sep 14, 2021
@kecho kecho requested a review from skhiat September 14, 2021 15:00
@Aydindeveloper
Copy link

Lens flare in URP does not work properly with Render scale.
For the solution, I applied the change in universal/decal-fixes to DoLensFlareDatadriven.

postprocess.cs

void DoLensFlareDatadriven(Camera camera, CameraData camdata, CommandBuffer cmd, RenderTargetIdentifier source, bool usePanini, float paniniDistance, float paniniCropToFit)
{
var gpuView = camera.worldToCameraMatrix;
var gpuNonJitteredProj = GL.GetGPUProjectionMatrix(camera.projectionMatrix, true);
// Zero out the translation component.
gpuView.SetColumn(3, new Vector4(0, 0, 0, 1));
var gpuVP = gpuNonJitteredProj * camera.worldToCameraMatrix;
float renderScale = camdata.isSceneViewCamera ? 1f : camdata.renderScale;
float scaledCameraWidth = (float)camera.pixelRect.width * renderScale;
float scaledCameraHeight = (float)camera.pixelRect.height * renderScale;

        LensFlareCommonSRP.DoLensFlareDataDrivenCommon(m_Materials.lensFlareDataDriven, LensFlareCommonSRP.Instance, camera, scaledCameraWidth, scaledCameraHeight,

@kecho
Copy link
Contributor Author

kecho commented Sep 14, 2021

Lens flare in URP does not work properly with Render scale.
For the solution, I applied the change in universal/decal-fixes to DoLensFlareDatadriven.

postprocess.cs

void DoLensFlareDatadriven(Camera camera, CameraData camdata, CommandBuffer cmd, RenderTargetIdentifier source, bool usePanini, float paniniDistance, float paniniCropToFit)
{
var gpuView = camera.worldToCameraMatrix;
var gpuNonJitteredProj = GL.GetGPUProjectionMatrix(camera.projectionMatrix, true);
// Zero out the translation component.
gpuView.SetColumn(3, new Vector4(0, 0, 0, 1));
var gpuVP = gpuNonJitteredProj * camera.worldToCameraMatrix;
float renderScale = camdata.isSceneViewCamera ? 1f : camdata.renderScale;
float scaledCameraWidth = (float)camera.pixelRect.width * renderScale;
float scaledCameraHeight = (float)camera.pixelRect.height * renderScale;

        LensFlareCommonSRP.DoLensFlareDataDrivenCommon(m_Materials.lensFlareDataDriven, LensFlareCommonSRP.Instance, camera, scaledCameraWidth, scaledCameraHeight,

@Aydindeveloper I believe this has been fixed already. Check this PR #5373
It should be available in 12.x soon

@kecho kecho force-pushed the core/fix-celestial-flare-far-cam branch from 9ca5d53 to e21a27d Compare September 15, 2021 13:04
@SeanPuller SeanPuller self-requested a review September 15, 2021 13:23
@Aydindeveloper
Copy link

Sorry, I was referring to URP's Render Scale lens flare issues but the PR you linked seems to for HDRP

@kecho
Copy link
Contributor Author

kecho commented Sep 15, 2021

All tests pass, only a few fails due to unrelated issues.

@kecho kecho marked this pull request as ready for review September 15, 2021 16:25
@kecho kecho requested a review from a team September 16, 2021 13:17
@kecho
Copy link
Contributor Author

kecho commented Sep 16, 2021

NOTE
@Unity-Technologies/gfx-qa-hdrp Change has been verified by Sean Puller

@kecho
Copy link
Contributor Author

kecho commented Sep 16, 2021

@Aydindeveloper

Sorry, I was referring to URP's Render Scale lens flare issues but the PR you linked seems to for HDRP

I recommend you open a bug, ill notify the URP team on our side. Cheers

@kecho kecho merged commit 29471e8 into master Sep 16, 2021
@kecho kecho deleted the core/fix-celestial-flare-far-cam branch September 16, 2021 14:14
sebastienlagarde pushed a commit that referenced this pull request Sep 20, 2021
* Celestial relative flare camera position fixed, using regular matrices instead to fix precision issues

* Fix in logical split of viewport computation, and adding CHANGES.md info for lensflare
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants