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

Reimplement Projection in rust #143

Closed
lilizoey opened this issue Mar 5, 2023 · 4 comments · Fixed by #179
Closed

Reimplement Projection in rust #143

lilizoey opened this issue Mar 5, 2023 · 4 comments · Fixed by #179
Labels
c: core Core components good first issue Good for newcomers quality-of-life No new functionality, but improves ergonomics/internals

Comments

@lilizoey
Copy link
Member

lilizoey commented Mar 5, 2023

The classes Basis, Transform2D, Transform3D, and Projection, are all matrix-based classes. All of them except Projection have most of their methods largely reimplemented in rust, mainly for performance reasons as crossing the ffi-boundary can be expensive.

Projection is mainly used as a 4x4 graphics projection matrix. So if you're familiar with graphics programming this would be a good thing to do.

The projection impl should ideally look similar to Basis, Transform2D, and Transform3D.

In addition, writing tests would be needed. Both unit tests for code that can be tested entirely in rust, but also integration tests. See here for an example of what integration tests could look like.

The original PR, #124, that added Projection and the other matrix-types to the bindings can also be useful to look at.

For more information about Projection see:
docs
source code

@Bromeon Bromeon added good first issue Good for newcomers quality-of-life No new functionality, but improves ergonomics/internals c: core Core components labels Mar 5, 2023
@Dheatly23
Copy link
Contributor

Hello, i'm trying to crack this issue and found a problem. In static method create_perspective_hmd the argument eye can be neither 1 or 2, in Godot source the behavior is defined (although undocumented). Should we provide neither/zero value in the enum?

@Bromeon
Copy link
Member

Bromeon commented Mar 13, 2023

What happens in that case (you happen to have a link to the source)?

If it's not documented, I tend to consider it as an implementation detail that can change any moment 🤔

@Dheatly23
Copy link
Contributor

Dheatly23 commented Mar 13, 2023

In projection.cpp

	switch (p_eye) {
		case 1: { // left eye
			left = -xmax + frustumshift;
			right = xmax + frustumshift;
			modeltranslation = p_intraocular_dist / 2.0;
		} break;
		case 2: { // right eye
			left = -xmax - frustumshift;
			right = xmax - frustumshift;
			modeltranslation = -p_intraocular_dist / 2.0;
		} break;
		default: { // mono, should give the same result as set_perspective(p_fovy_degrees,p_aspect,p_z_near,p_z_far,p_flip_fov)
			left = -xmax;
			right = xmax;
			modeltranslation = 0.0;
		} break;
	}

I guess it's fine to leave it as-is. But let's just say for consideration and completion.

@lilizoey
Copy link
Member Author

lilizoey commented Mar 13, 2023

  default: { // mono, should give the same result as set_perspective(p_fovy_degrees,p_aspect,p_z_near,p_z_far,p_flip_fov)

As they note here, this should be the same as create_perspective. You could add to the documentation of the function that if people want this functionality, they should use create_perspective instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components good first issue Good for newcomers quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants