Dynamic Resolution Tests #3579
Dynamic Resolution Tests #3579
Conversation
kecho
requested review from
sebastienlagarde,
JulienIgnace-Unity and
FrancescoC-unity
| @@ -0,0 +1,15 @@ | |||
| using System; | |||
|
|
|||
| namespace UnityEngine.Rendering.HighDefinition | |||
There was a problem hiding this comment.
Will add proper docs here, if you guys agree on this callback arguements / naming
| @@ -213,7 +213,7 @@ internal void ExecuteInternal(RenderGraph renderGraph, HDCamera hdCamera, Cullin | |||
| this.owner = owner; | |||
| this.currentRenderTarget = targets; | |||
| this.currentHDCamera = hdCamera; | |||
|
|
|||
| OnRenderEvent(new CustomPassRenderEventContext() { hdCamera = hdCamera, eventType = CustomPassRenderEventContext.EventType.OnExecute }); | |||
There was a problem hiding this comment.
Adding @alelievr for review on adding callback in custom pass
There was a problem hiding this comment.
@kecho I'm not sure to see why we need to add another user callback for such case, there is already the Execute() function of the custom pass that should be enough.
Additionally, your callback is outside of the render graph, so I'm really not sure when it would be executed.
Also, I don't understand the purpose of the enum CustomPassRenderEventContext.EventType. We already have the injection point available in the custom pass API: https://docs.unity3d.com/Packages/com.unity.render-pipelines.high-definition@10.3/api/UnityEngine.Rendering.HighDefinition.CustomPass.html#UnityEngine_Rendering_HighDefinition_CustomPass_injectionPoint
There was a problem hiding this comment.
Hey @alelievr , the problem with Execute is that it happens too late. It happens in the execution of the render graph itself. I need a callback that happens during render graph construction, in fact something before we construct the render graph for the bound camera. I thought having some sort of 'event' hook would make it easy to add other injection points later if needed to, like a bookmark system.
I need a callback that ticks once per camera, before performing any rendering / render graphing. This tick must happen during render graph construction, prior to running the render graph.
I need to specify a new upsampling filter per camera, and this must happen before render graph execution, otherwise its too late.
Essentially I need the equivalent of a PreRender. Another thing I could do is add custom events to the hdCamera itself, and have a way of calling PreRender from the hdCamera. Let me know what you recommend. Maybe I shoudlnt be using the custom pass system? I couldnt find anything else that would allow me to run some function once per camera. Let me know your thoughts
There was a problem hiding this comment.
Okay, so indeed, in that case, we don't have yet an API that provides access during the construction of the render graph. We still have the plan to expose such API when we'll move the custom passes to render graph (with full API on the user side).
So right now, I'd say that it's better to not expose the API you added because we won't be able to remove it before the replacement lands (if it's public, it needs to be documented and kept alive for migration purposes during multiple major versions :/ )
Just to make sure, right now this callback is only used during our tests? If that's the case, we can either make it internal and then replace it with a proper API when it lands. Or you can use the This callback:
https://docs.unity3d.com/ScriptReference/Rendering.RenderPipelineManager-beginCameraRendering.html
It's called at the very beginning of the frame before the culling, would this work for you?
There was a problem hiding this comment.
I need a callback that ticks once per camera, before performing any rendering / render graphing.
We have BeginCameraRendering (https://docs.unity3d.com/2021.1/Documentation/ScriptReference/Rendering.RenderPipeline.BeginCameraRendering.html) but not sure if it fit all your need.
There was a problem hiding this comment.
@sebastienlagarde this delegate will work! However I will have to make some refactors on how the state of the DynamicResolutionManager gets propagated. Because the render requests run deferredly, and the DynamicResolutionHandler is a singleton. Will have to run it by @FrancescoC-unity but sounds like the way to go then.
There was a problem hiding this comment.
Did the changes, worked like a charm. @alelievr won't need the custom pass callback. Thanks for checking in. Will wait on a review from Francesco now :)
There was a problem hiding this comment.
Did you push said changes? I see only formatting as recent push
There was a problem hiding this comment.
woops! sorry just pushed again
There was a problem hiding this comment.
Oh the change looks trivial to me, I am good with it 🟢
There was a problem hiding this comment.
I am ok with the change on my side, but prefer to let @alelievr judge about the added callback or if we want to do it another way
|
Note: you should trigger Yamato |
kecho
force-pushed
the
hdrp/drs-tests
branch
from
90fab39
to
f597634
Compare
kecho
force-pushed
the
hdrp/drs-tests
branch
from
f597634
to
d6c8a83
Compare
kecho
force-pushed
the
hdrp/drs-tests
branch
from
d6c8a83
to
51549f4
Compare
|
Need to update screenshots now :) |
|
Final screenshots have been updated. Triggering yamato one more time. |
|
Test is green, I will move forward and merge it as it can prevent to break code. If QA have feedback, please raise them, thanks. |
Purpose of this PR
Comments to reviewers
This PR includes a new callback in the CustomPass. If we dont wan't to expose this, please provide feedback on other possible ways we could change the filter mid frame.
Another possibility as well is to create 8 separate tests, and keep the filter on a per frame basis (not per camera basis). This however would add 8 new tests.