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

Rename Camera3D.project_position() to Camera3D.viewport_to_world() #59592

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Mar 27, 2022

Camera3D.unproject_position() was also renamed to Camera3D.world_to_viewport(). The screen_point parameters to various Camera3D methods were also renamed to viewport_point to be more explicit.

This wording is less technically accurate, but it is much more obvious, especially for those less familiar with 3D rendering.

See #54161 (comment).

`Camera3D.unproject_position()` was also renamed to
`Camera3D.world_to_viewport()`. The `screen_point` parameters to
various Camera3D methods were also renamed to `viewport_point`
to be more explicit.

This wording is less technically accurate, but it is much more obvious,
especially for those less familiar with 3D rendering.
@Calinou Calinou force-pushed the camera-rename-project-position branch from 4ead032 to b588b31 Compare March 27, 2022 18:32
@Zireael07
Copy link
Contributor

Heck yeah, I always confuse project() and unproject() - the only place where I got them to work was somebody else's code from a tutorial :P

@clayjohn
Copy link
Member

I'm not sure about this change. I can see that the current naming is very confusing to users. But the suggested naming comes from the naming scheme that I suggested for shaders which was rejected by other contributors in the Rocketchat rendering channel and in a subsequent PR review meeting. Accordingly, the viewport_to_world() and world_to_viewport() convention may not make sense as it can't be said that the change is made for consistency.

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 23, 2022

We've discussed this in a PR meeting, and in general weren't against the idea. But given there is a comment from Clay, let's put it to discussion in one of the rendering team meetings.

Also the renames need to be added to the conversion tool before the merge.

@jejay
Copy link

jejay commented Aug 26, 2022

The alternative would just be switching unproject/project. As I said, everyone else is using these terms in their opposite meaning (#54161 (comment)). It goes so clearly (and lonely) against convention that I'd consider it a bug; and at least this computer graphics professor @seichter does so as well: #54161 (comment).

Likewise, when there is something called CAMERA_MATRIX (=C) and INV_CAMERA_MATRIX (=C^I), I would expect

p_v = C * p_w (point in world -> point in view via C)
p_w = (C^I) * p_v (point in view -> point in world via inverse of C)

But simply by looking at @clayjohn's suggestions (#54161 (comment)) it looks like the current implementation is

p_v = (C^I) * p_w (point in world -> point in view via inverse of C)
p_w = C * p_v (point in view -> world in view via C)

which is (more than?) a little bit odd. Maybe just switching the name as well could be a solution if @clayjohn's proposal was not accepted.

Here is someone else complaining about this: godotengine/godot-docs#4525. And here again @seichter: #54161 (comment)

My understanding was that the X_TO_Y naming proposal was so popular because lots of Godot users have gotten used to the counterconventional (/incorrect, if I'm harsh) names. I personally prefer a logical, unconventional naming scheme over a counterconventional one. But if you don't like the unconventional naming, the problem with the counterconventional naming scheme still persists.

I just realized you actually fixed the shader builtins by just switching their names: #59268 Awesome, that would have also been my preferred solution. But then it's only logical to also switch unproject/project and refrain from the x_to_y naming scheme.

@akien-mga akien-mga requested a review from clayjohn September 6, 2022 12:57
@clayjohn
Copy link
Member

We definitely need to find a way to fix project/unproject naming if they are indeed backwards. But the proposed rename was already rejected in the context of shaders, so I do not think it should be utilized here. Seeing as this is already getting too late to merge for 4.0 I think our best option is to move to a proposal for fixing the project/unproject and then aim to fix it in 4.x

@clayjohn clayjohn closed this Jan 24, 2023
@clayjohn clayjohn removed this from the 4.0 milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Third Option
Development

Successfully merging this pull request may close these issues.

5 participants