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

Make RenderSceneData take projection correction into account #93630

Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jun 26, 2024

This came out of #90148 and deals with the comment made by @xcodian

We now make sure that get_cam_projection and get_view_projection return corrected projections to deal both with our reverse-z and flip-y logic.

This also adds logic to keep the flip_y state in our scene data object and remove passing this around as a parameter, in doing so it highlights there are a few other scenarios where we flip y.
The compatibility renderer has not been touched as it currently does not support compositor effects but has similar logic we'll look at as part of a planned cleanup in 4.4.

For testing, this is @jake-low's original MVP adjusted for master including fixing memory leakage and adjusting it for this change.
MVP-90148-corrected-v2.zip
Note:

  • Reverse-Z means that near-depth is 1.0
  • I could not reproduce the gamma issue, it's possible that this was fixed alongside the reverse-z changes
  • The reported "upside down" orientation is the reality we live with. We are working on internal buffers which need this orientation. This PR embeds the needed flip in the projection matrix making it less likely mistakes are made.

image
(note, the cube was moved into the cone on purpose, was just testing if depth was correct here)

@BastiaanOlij
Copy link
Contributor Author

Just double checking, @akien-mga @AThousandShips it was 4.2-stable.expected where I need to add checks for compatibility breakage? I'm not fixing the virtual functions because there is no possible scenario where someone would extend this class with GDExtension, but calling these functions from GDScript is a use case we need to catch.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 26, 2024

This was exposed in 4.3 AFAIK so not even sure compatibility binds are necessary

Edit: Yep, there's no RenderSceneData in 4.2, so the validation doesn't need to change (unless this is added in 4.4), but I think the compat binds are relevant still for smooth transition

@BastiaanOlij
Copy link
Contributor Author

This was exposed in 4.3 AFAIK so not even sure compatibility binds are necessary

DOH! off course!

@BastiaanOlij
Copy link
Contributor Author

@kebabskal would you mind checking this out as well and see if this has any effects on your project? Yours is probably the most extensive use case of compositor effects so if we're overlooking anything, hopefully we can uncover it quickly.

@BastiaanOlij BastiaanOlij force-pushed the scene_data_projection_correction branch 3 times, most recently from 7a2d92d to a31e958 Compare June 26, 2024 13:38
@BastiaanOlij BastiaanOlij force-pushed the scene_data_projection_correction branch from a31e958 to fd8a313 Compare June 26, 2024 13:41
@BastiaanOlij BastiaanOlij marked this pull request as ready for review June 26, 2024 13:41
@BastiaanOlij BastiaanOlij requested review from a team as code owners June 26, 2024 13:41
@@ -41,8 +41,12 @@ Transform3D RenderSceneDataRD::get_cam_transform() const {
return cam_transform;
}

Projection RenderSceneDataRD::get_cam_projection() const {
return cam_projection;
Projection RenderSceneDataRD::get_cam_projection(bool p_flip_y) const {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way that we could detect if the RenderSceneData is being used in ReflectionProbes? I would really like to remove the p_flip_y option if we can.

For starters, it will always be false in the GLES3 version of this class. So the user shouldn't have to think about that.

Perhaps we could store whether a ReflectionProbe is being used when we create the class?

Alternatively, we could overhaul the renderer so we aren't rendering upside down. As far as I can tell there has never been a benefit to rendering upside down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clue everyone else in on our discussion this morning.

Our decision for now is that for 4.3 we'll try and make the logic in RenderSceneData* automatic, I'll look into the needed changes to make this possible and update this PR.

Than for 4.4 we're going to investigate removing the flip logic all together. I'll raise a proposal with the details for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we could just render right side up always, in both RD and compatibility, that would be amazing! :-)

@BastiaanOlij BastiaanOlij force-pushed the scene_data_projection_correction branch from fd8a313 to 6ed6212 Compare June 27, 2024 00:44
@@ -2562,6 +2562,7 @@ void RenderForwardClustered::_render_shadow_append(RID p_framebuffer, const Page
SceneState::ShadowPass shadow_pass;

RenderSceneDataRD scene_data;
scene_data.flip_y = !p_flip_y; // Q: Why is this inverted? Do we assume flip in shadow logic?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something to investigate later, I'm pretty sure we've hardcoded the flip in the shaders used here,. I've left this comment here so we are reminded to do this as part of the planned flip-y clean-up (ditto on mobile renderer).

@kebabskal
Copy link

@kebabskal would you mind checking this out as well and see if this has any effects on your project? Yours is probably the most extensive use case of compositor effects so if we're overlooking anything, hopefully we can uncover it quickly.

As you saw, tried it on stream and it seems to work perfectly! Thanks!

@BastiaanOlij
Copy link
Contributor Author

As you saw, tried it on stream and it seems to work perfectly! Thanks!

Yup, and really cool to see using the scene data UBO directly working

@xcodian
Copy link

xcodian commented Jul 7, 2024

Taking the projection correction matrix into account seems to have fixed the issue completely, compositor effect shaders are behaving as expected.

Can't wait for this to get merged :)

@akien-mga akien-mga merged commit 6f9c0aa into godotengine:master Jul 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Very Bad
Development

Successfully merging this pull request may close these issues.

7 participants