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

remove far plane from perspective projection #2118

Open
wants to merge 14 commits into
base: master
from

Conversation

@joonazan
Copy link

joonazan commented Feb 2, 2020

I switched to near to 1 and far to 0, which should make depth much more
accurate.

3d doesn't have a far plane anymore, as it should be much more accurate than
before even with infinite Z.

Infinite-z does not make sense for orthographic projections, because error must
be small regardless of distance there.

I switched to near to 1 and far to 0, which should make depth much more
accurate.

3d doesn't have a far plane anymore, as it should be much more accurate than
before even with infinite Z.

Infinite-z does not make sense for orthographic projections, because error must
be small regardless of distance there.
@@ -222,7 +222,7 @@ fn build_lines_pipeline<B: Backend>(
blend: Some(pso::BlendState::ALPHA),
}])
.with_depth_test(pso::DepthTest {
fun: pso::Comparison::LessEqual,

This comment has been minimized.

Copy link
@joonazan

joonazan Feb 2, 2020

Author

I'm a bit worried about this one. The skybox had LessEqual for good reason, but I fail to see why it was like this here.

GreaterEqual would be useful if debuglines was somehow modified to sometimes draw lines at infinity.

This comment has been minimized.

Copy link
@joonazan

joonazan Feb 4, 2020

Author

Changed it to GreaterEqual. That causes later lines to be drawn on top of earlier ones. It should also make them visible in a 2d game.

@joonazan joonazan closed this Feb 2, 2020
@joonazan joonazan reopened this Feb 2, 2020
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 2, 2020

Please rerun CI for me. It failed to fetch the repository on nightly.

@joonazan joonazan requested a review from jaynus Feb 2, 2020
This matches the LessEqual it previously had.

It makes debug_lines_ortho render like it did before these changes. I believe
that rendering later lines on top of earlier ones is the expected behaviour.
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 4, 2020

Now fetching failed on stable...

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 7, 2020

bors try

bors bot added a commit that referenced this pull request Feb 7, 2020
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 7, 2020

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 7, 2020

Heya, looks like some camera tests need to be updated.

failures:

---- camera::tests::extract_perspective_values stdout ----
thread 'camera::tests::extract_perspective_values' panicked at 'assert_ulps_eq!(0.1, proj.near())

    left  = 0.1
    right = inf

', amethyst_rendy/src/camera.rs:961:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- camera::tests::orthographic_depth_usage stdout ----
thread 'camera::tests::orthographic_depth_usage' panicked at 'assert_abs_diff_eq!(projected_point[2] / projected_point[3], 0.0)

    left  = 1.0
    right = 0.0

', amethyst_rendy/src/camera.rs:1206:9

---- camera::tests::extract_orthographic_values stdout ----
thread 'camera::tests::extract_orthographic_values' panicked at 'assert_ulps_eq!(-5.0, proj.near())

    left  = -5.0
    right = 100.0

', amethyst_rendy/src/camera.rs:992:9

---- camera::tests::perspective_depth_usage stdout ----
thread 'camera::tests::perspective_depth_usage' panicked at 'assert_abs_diff_eq!(projected_point[2] / projected_point[3], 0.0)

    left  = 1.000001
    right = 0.0

', amethyst_rendy/src/camera.rs:1180:9

---- camera::tests::screen_to_world_2d stdout ----
thread 'camera::tests::screen_to_world_2d' panicked at 'assert_ulps_eq!(camera.projection().screen_to_world_point(center_screen, diagonal, &transform), Point3::new(0.0, 0.0, -0.1))

    left  = Point { coords: Matrix { data: [0.0, 0.0, -2000.0] } }
    right = Point { coords: Matrix { data: [0.0, 0.0, -0.1] } }

', amethyst_rendy/src/camera.rs:862:9

---- camera::tests::screen_to_world_3d stdout ----
thread 'camera::tests::screen_to_world_3d' panicked at 'assert_ulps_eq!(camera.projection().screen_to_world_point(center_screen, diagonal, &transform), Point3::new(0.0, 0.0, -0.1))

    left  = Point { coords: Matrix { data: [0.0, 0.0, -1.0000001] } }
    right = Point { coords: Matrix { data: [0.0, 0.0, -0.1] } }

', amethyst_rendy/src/camera.rs:819:9

---- camera::tests::perspective_orientation stdout ----
thread 'camera::tests::perspective_orientation' panicked at 'assertion failed: `(left < right)`
  left: `0.1`,
 right: `0.0`', amethyst_rendy/src/camera.rs:1132:9

failures:
    camera::tests::extract_orthographic_values
    camera::tests::extract_perspective_values
    camera::tests::orthographic_depth_usage
    camera::tests::perspective_depth_usage
    camera::tests::perspective_orientation
    camera::tests::screen_to_world_2d
    camera::tests::screen_to_world_3d
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 7, 2020

Yeah, looks like I missed those. What command should I use to run all tests?

@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 7, 2020

Those failures are legit and point to a bug I've introduced. I haven't updated the getters.

I want to remove them in a future PR, so I'll do that now. It would be a waste of effort to first change them and then remove them.

The setters and getters for parts of a projection have been removed. They were
broken, as the setters did not update the inverse matrix. If updating is
desired, a new Camera can be build, which should be about as fast.

This commit breaks auto_fov. I'll rewrite that utility in the next commit.
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 8, 2020

Now the tests fail because the depth of a thing on the near plane is 1.000001 instead of 1.0. Can we make the test more lenient? I don't think I can improve the accuracy because there's just a single division.

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 8, 2020

I haven't yet had time to review, but is the test with that inaccuracy using assert_ulps_eq!? That normally adjusts for floating point errors. If it already is, I think there's a version of the macro that allows taking in n times inaccuracy, perhaps set that to 2x.

@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 8, 2020

Yes, I'm using that macro.

I found the root cause of the inaccuracy. 0.1 / (3 - 2.9) is 1.000001 in single precision. I'll fix that test by choosing values that can be represented in binary.

joonazan added 2 commits Feb 8, 2020
One eighth is a good value because it can be represented accurately unlike one
tenth.

The small change hopefully doesn't break people's code.
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 8, 2020

Still TODO:

  • rewrite auto_fov
  • check that screen_to_world is pixel perfect
It turns out the calculations needed for auto_fov are already done when
constructing a perspective matrix.
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 8, 2020

I noticed that auto_fov does not work for orthographic cameras. Maybe it should be extended to take an enum of Perspective or Orthographic?

Also, Camera::perspective (previously Projection::perspective) makes a projection matrix so that the vertical fov is the fov given as argument. So I think I should make a more general version of it that does not assume that vertical fov is what you want to specify.

@joonazan joonazan force-pushed the joonazan:master branch from fcfc4ce to ee6b153 Feb 8, 2020
@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 10, 2020

I noticed that auto_fov does not work for orthographic cameras. Maybe it should be extended to take an enum of Perspective or Orthographic?

Perhaps; or store it in the Camera? The previous behaviour seems to ignore changing the camera if it's not Perspective, but my game which uses an Orthographic camera and AutoFovSystem does inherit changes, so I think I went wrong somewhere when tracing code.

Also, I tried reviewing, but the math went over my head -- roundabout way of saying I'd trust you to get it right more than myself in reviewing 😅. @jaynus / @Frizi, do either of you have time to review?

Previously it was only updated when the screen dimensions changed, making it not
work if the screen is never resized or if parameters are changed.
@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 19, 2020

Heya, I tried out this rebased on master, and the sprites_ordered / sprite_camera_follow examples don't render sprites (these use an orthographic camera).

I haven't managed to figure out how to update them to make them work, and can't seem to understand the code (sorry! but am getting lost in the math / code).

cargo run --example sprite_camera_follow --features "vulkan"
cargo run --example sprites_ordered --features "vulkan"
cargo run --example rendy --features "vulkan animation gltf"
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 19, 2020

@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 20, 2020

@azriel91 I was able to reproduce the issue. Investigating it now.

@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 20, 2020

@azriel91 I fixed it. I had forgotten to change sprites to render with a depth test that accepts fragments with depth greater than the buffer.

I still want to improve the screen -> world and vice versa tests before this is merged. I have done some research on the topic and I've learned that in Vulkan screen coordinates 0.0 and 1.0 are the edges of the screen.

Is this true in Amethyst as well? For example, is mouse position measured like that, or is (0, 0) the center of the first pixel?

If there is no consensus, I'll use the Vulkan scheme and document it in the world_to_screen functions documentation.

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 20, 2020

Heya, thanks for that (of course it's a one line change 😛).

Is this true in Amethyst as well? For example, is mouse position measured like that, or is (0, 0) the center of the first pixel?

Based on the current master, it looks like do follow Vulkan's convention for screen coordinates -- (0.0, 0.0) top left, (1.0, 1.0) bottom right.

Thanks again for putting in so much work.

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 21, 2020

By the way, even though screen coordinates are 0.0, 0.0 on the top left, world coordinates are 0.0, 0.0 on the bottom left by current convention. The UI coordinate system was changed to match world coordinates instead of screen coordinates at some point.

@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Feb 22, 2020

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Feb 23, 2020

Heya, sorry forgot to reply over the weekend. I'd recommend https://community.amethyst.rs/ for initial discussion (and visibility) -- though it may switch over to https://github.com/amethyst/rfcs/ to be immortalized.

Personally I don't mind either orientation, as long as we stick with something.

joonazan added 3 commits Mar 1, 2020
They used to use a version that was off by one pixel for some reason.
@joonazan

This comment has been minimized.

Copy link
Author

joonazan commented Mar 1, 2020

I think this PR is ready now.

@azriel91

This comment has been minimized.

Copy link
Member

azriel91 commented Mar 22, 2020

@joonazan hey, am going be releasing a version of amethyst tomorrow, but am sorry that am unable to comprehend the changes in this PR (math / graphics are difficult for myself), so have chosen to defer this until a later version. Promise to try get it in the next time 🙇

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

2 participants
You can’t perform that action at this time.