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

expose XRInterface::get_transform_for_view to gdscript #68390

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

cheece
Copy link
Contributor

@cheece cheece commented Nov 7, 2022

Hello again

By exposing XRInterface::get_transform_for_view, it's possible to implement true stereoscopic mirrors and portals in VR previously impossible in godot.

Previously i made a PR for 3.x only since viewport textures seemed to be not working on godot 4 but I found the workaround was to set the viewport size to something different than the default 512, it's now working.

I made a demo for godot 4 using the method to render stereoscopic mirrors in VR, it can be found here:
https://github.com/cheece/godot-vr-mirror-test-g4

20221107191406_1_vr

@dsnopek
Copy link
Contributor

dsnopek commented Nov 8, 2022

Personally, I think exposing this is fine! Should we also expose get_projection_for_view()?

As far as PR review, it'd be nice to have a description for the API docs!

@BastiaanOlij Do you see any issues with exposing this?

@BastiaanOlij
Copy link
Contributor

@dsnopek in Godot 4 I have no problems in exposing this and agree that we should also expose get_projection_for_view now that Projection (was CameraMatrix) is supported in GDScript.

@cheece
Copy link
Contributor Author

cheece commented Nov 9, 2022

@dsnopek in Godot 4 I have no problems in exposing this and agree that we should also expose get_projection_for_view now that Projection (was CameraMatrix) is supported in GDScript.

thanks, should I edit the PR to include get_projection_for_view then?

@dsnopek
Copy link
Contributor

dsnopek commented Nov 9, 2022

thanks, should I edit the PR to include get_projection_for_view then?

Yes, please!

@cheece cheece requested review from a team as code owners November 9, 2022 16:36
@cheece cheece force-pushed the expose_get_transform_for_view branch from e419258 to 33065ed Compare November 9, 2022 16:43
@dsnopek
Copy link
Contributor

dsnopek commented Nov 9, 2022

Other than the minor parameter naming thing I pointed out above, (UPDATE: I was wrong about the parameter names) the code changes here are looking good to me!

However, you're also going to need to squash your changes down into a single commit (per Godot's PR workflow):

https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

@cheece cheece force-pushed the expose_get_transform_for_view branch from 9473478 to bceff0f Compare November 10, 2022 12:27
@cheece cheece force-pushed the expose_get_transform_for_view branch from bceff0f to 84f6791 Compare November 10, 2022 12:34
@cheece
Copy link
Contributor Author

cheece commented Nov 10, 2022

Ok 👍 , I think it should be fixed now

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me!

@clayjohn clayjohn merged commit d2bd8e5 into godotengine:master Nov 10, 2022
@clayjohn
Copy link
Member

Looks great, thanks!

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.

None yet

6 participants