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

Add external camera feed and occlusion based on a depthmap #95187

Conversation

elmajime
Copy link

@elmajime elmajime commented Aug 6, 2024

This version of occlusion makes use of the depth buffer initialisation. No blending of the occlusion edges is done.

This version of occlusion makes use of the depth buffer initialisation. No blending of the occlusion edges is done.
@AThousandShips AThousandShips changed the title Added external camera feed and occlusion based on a depthmap Add external camera feed and occlusion based on a depthmap Aug 6, 2024
@AThousandShips
Copy link
Member

Please open a proposal to track the support and details of this feature

Copy link
Member

Choose a reason for hiding this comment

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

Please do not include your own build scripts in your PR

@elmajime
Copy link
Author

elmajime commented Aug 6, 2024

@AThousandShips yes, this draft is only for communication purposes with the XR team

@@ -748,6 +753,27 @@ void TextureStorage::texture_free(RID p_texture) {
texture_owner.free(p_texture);
}

RID TextureStorage::texture_set_external(RID p_texture, int p_width, int p_height) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've discussed this before, but if we're going to call this "external" (which I think makes sense because GL_TEXTURE_EXTERNAL_OES) we need to rename TextureStorage::texture_create_external() to something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed but still open to suggestions :)

Copy link
Author

Choose a reason for hiding this comment

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

@dsnopek I'm going to open a proposal and clean the code so that it can be reviewed. What would be a good replacement for texture_set_external ? Something like texture_reallocate ?

@BastiaanOlij
Copy link
Contributor

Alright, had a quick review of the code during our ARCore meeting. In general lines this is a nice re-implementation of the original ARCore functionality from Godot 3's defunct ARCore PR.

The only thing I think we need to change is how occlusion is handled, these are a lot of changes that are very specific to how ARCore handles this, while we have a similar need to solve this for OpenXR when pulling in the depth buffer from the depth sensor but there we're leaving it to the XR compositor to overlay things.

I'd like to change this to remove the current occlusion implementation and just focus on re-introducing the external texture support and camera feed for the background. Then deal with occlusion in a more holistic way that also covers the needs for OpenXR.

This will involve copying the depth data into the depth buffer before the opaque pass (is also more efficient), where the depth buffer data can come either from ARCore, OpenXR, or any other source, and then applying the camera image later as a copy possibly with an alpha blend (ergo we'll have the background set to transparent, where we haven't rendered geometry due to the prepopulated depth buffer, we'll be underlying the camera image.

@BastiaanOlij
Copy link
Contributor

cc @clayjohn we'll need to discuss in more detail during one of the render meetings to finalise the approach for preloading a depth buffer.

@elmajime
Copy link
Author

elmajime commented Sep 7, 2024

Please open a proposal to track the support and details of this feature

As this is my first proposal I hope I did it right, tell me if this is ok:
godotengine/godot-proposals#10678

@elmajime elmajime closed this Sep 8, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Sep 9, 2024
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.

4 participants