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

OpenXR: Allow composition layers to enable hole punching #91485

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented May 2, 2024

I would probably have tried to include this in PR #89880, which added the OpenXRCompositionLayer nodes, but I thought it wasn't supported by Godot's renderer, until last week @BastiaanOlij let us know about the magical shadow_to_opacity shader render mode (and fixed it in PR #91096).

This adds an enable_hole_punch property on OpenXRCompositionLayer that can enable a technique called "hole punching", which allows putting the composition layer behind the main projection layer (ie setting sort_order to a negative value) while "punching a hole" through everything rendered by Godot so that the layer is still visible.

This can be used to create the illusion that the composition layer exists in the same 3D space as everything rendered by Godot, allowing objects to appear to pass both behind or in front of the composition layer.

Anyway, given that it is a small change to a feature added in Godot 4.3, perhaps we can sneak it in?

@dsnopek dsnopek added this to the 4.3 milestone May 2, 2024
@dsnopek dsnopek requested review from m4gr3d and BastiaanOlij May 2, 2024 22:17
@devloglogan
Copy link
Contributor

Gave it a try, seems to work well! If I understand correctly, enable_hole_punch will make the alpha_blend property useless. Perhaps it's worth mentioning that in the property description?

@dsnopek
Copy link
Contributor Author

dsnopek commented May 3, 2024

Gave it a try, seems to work well!

Thanks for testing!

If I understand correctly, enable_hole_punch will make the alpha_blend property useless. Perhaps it's worth mentioning that in the property description?

I think it's still potentially useful, it depends on what's behind the composition layer. If there's a passthrough layer at the back, you could use alpha_blend to make the composition layer partially transparent, and show the passthrough behind it.

@dsnopek dsnopek force-pushed the openxr-composition-layers-hole-punch branch 2 times, most recently from 74d9b8b to 9e17cdb Compare May 3, 2024 19:30
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Just a few nit-picking things, looks good!

modules/openxr/scene/openxr_composition_layer.cpp Outdated Show resolved Hide resolved
remove_child(fallback);
fallback = nullptr;
openxr_session_running = false;
if (fallback && !_should_use_fallback_node()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly safer to just do if (fallback). If there shouldn't be a fallback node, it won't have been created.

This could potentially create a leak where the fallback was created, but since then the dev disabled hole punch, so we don't remove the fallback because now the _should_use_fallback_node returns false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. There's really no risk of a leak here, because fallback is a node that's a child of the composition layer node, so it'll automatically get cleaned whenever the composition layer node is cleaned up. And, if the dev changed the hole punch setting, it would already have gotten cleaned up in set_enable_hole_punch(). But even beyond that, if _should_use_fallback_node() now returned false, we would still clean it up, because we're checking for !_should_use_fallback_node() (negated with !) in the above code.

So, I personally think this code is fine as it is.

@@ -212,7 +262,19 @@ void OpenXRCompositionLayer::_reset_fallback_material() {
return;
}

if (layer_viewport) {
if (enable_hole_punch && !Engine::get_singleton()->is_editor_hint() && is_natively_supported()) {
Ref<ShaderMaterial> material = fallback->get_surface_override_material(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading this wrong but what happens if I need the fallback and I have hole punch enabled? Wouldn't this override the material that renders my viewport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if you were on a platform that didn't natively support composition layers and you needed the "real" fallback mesh, then this code block wouldn't run because is_natively_supported() would return false, and you'd end up in the next code block which sets up the ViewportTexture

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good!

@dsnopek dsnopek force-pushed the openxr-composition-layers-hole-punch branch from 9e17cdb to 666bf60 Compare May 7, 2024 15:02
@dsnopek
Copy link
Contributor Author

dsnopek commented May 7, 2024

@BastiaanOlij @AThousandShips @m4gr3d Thanks for the review!

My latest push has integrated all of your suggestions, except for two where I personally think the current code is fine - both are explained in my responses above.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Looks good!

@akien-mga akien-mga merged commit 316b87d into godotengine:master May 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dsnopek dsnopek deleted the openxr-composition-layers-hole-punch branch July 22, 2024 15:34
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