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 a depth parameter to Camera::project_position() #29248

Merged
merged 1 commit into from May 28, 2019
Merged

Add a depth parameter to Camera::project_position() #29248

merged 1 commit into from May 28, 2019

Conversation

@Cheeseness
Copy link
Contributor

@Cheeseness Cheeseness commented May 28, 2019

Camera::project_position()'s current implementation makes assumptions that seem to work against the practical value of a function for converting 2D viewport coordinates into 3D coordinates. The current undocumented behaviour of returning a point on the camera's near clipping plane doesn't align with any use-cases that I can come up with, and is far enough from intuitive that I had to read the code to work out what was going on.

Having the depth exposed as a parameter allows the near clipping plane to be passed in if that ever happens to be desirable, while making the function more broadly usable without having to temporarily change the near clipping plane position.

In case preserving the current behaviour is desirable, I'd be happy to look at that, but I'm not sure how to approach function overloading with the language binding implementation.

@Cheeseness Cheeseness requested a review from as a code owner May 28, 2019
@reduz
Copy link
Member

@reduz reduz commented May 28, 2019

This is a strange function that was there since the beginning that served no real purpose (the actual one that is used everywhere is project_ray_position). It also does not match the sister function unproject_position, as it adds a near value that is unwanted.

I think the PR is good, but depth should be 0 by default

@Cheeseness
Copy link
Contributor Author

@Cheeseness Cheeseness commented May 28, 2019

I think the PR is good, but depth should be 0 by default

Thanks. Updated to default to 0.

scene/3d/arvr_nodes.cpp Outdated Show resolved Hide resolved
@owstetra
Copy link

@owstetra owstetra commented May 28, 2019

@reduz we use this function in our game , we use it to spawn a Waypoint on the Minimap

@akien-mga akien-mga added this to the 3.2 milestone May 28, 2019
@akien-mga akien-mga merged commit 0e441e9 into godotengine:master May 28, 2019
2 checks passed
@akien-mga
Copy link
Member

@akien-mga akien-mga commented May 28, 2019

Thanks for your contribution!

@Cheeseness
Copy link
Contributor Author

@Cheeseness Cheeseness commented May 29, 2019

Glad to pitch in. Thanks for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants