bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.35k stars 3.49k forks source link

View matrix and Inverse View matrix mixed up. #10909

Closed cshenton closed 3 months ago

cshenton commented 9 months ago

Bevy version

12.0

Problem

The naming of the view and inverse_view matrices in the View struct are the opposite of what those names mean everywhere else.

A view matrix transforms from world to view space. The "view matrix" in the View struct transforms from view to world space.

https://github.com/bevyengine/bevy/blob/a4c27288c773959ff307635b4ac25399e96f5de7/crates/bevy_render/src/view/mod.rs#L381

JMS55 commented 9 months ago

Duplicate of https://github.com/bevyengine/bevy/issues/8492

cshenton commented 9 months ago

That is an issue discussing alternative naming conventions. It does not cover the fact that there is an error in the existing names. view and inverse_view mean the opposite thing in Bevy than they do in every other renderer.

JMS55 commented 9 months ago

@superdump wrote it like this for some reason, I don't know the rationale. I'll leave it up to him to determine if this is a duplicate or not.

cshenton commented 9 months ago

Quick summary of places with the widely accepted convention that view == world_to_camera (i.e. not Bevy's convention)

Engines

Libraries

Gfx tutorials/docs

I could go on...

tl;dr: the convention in Bevy runs counter to the overwhelming majority of existing gfx tutorials, libraries, and engines. I can't find a single example that agrees with Bevy's convention.

IMO this is a major bug

DGriffin91 commented 9 months ago

I've also always found this odd. We should try to match the industry conventions soon than later. This is not a duplicate of https://github.com/bevyengine/bevy/issues/8492 And this issue should not be closed.

superdump commented 9 months ago

This has been discussed elsewhere. It isn't a duplicate of #8492, and I think we should resolve it by implementing #8492. I had done this, but I didn't want to disrupt @DGriffin91 's helper implementations.

superdump commented 9 months ago

I haven't thoroughly checked all of the links. Foundations of Game Engine Development 2, Rendering by Eric Lengyel uses the inverse camera matrix to transform from 'world' to 'camera' space. That uses a right-side multiplication of matrices convention. I don't remember where I originally saw the argument about "if you have a camera in space, looking into a scene, and you rotate the camera to anti-clockwise about its up axis, what happens visually to the objects the camera sees? They rotate about the camera position along the camera's up axis, but clockwise".

Not that it matters other than for trying to understand where it came from in Bevy, but I'm not certain I was the one who originally wrote this, but I feel I have understood it since, and there were definitely references.

I think to take the project forward without wasting people's time discussing whether view or inverse view is necessarily correct, we were anyway intending to do #8492 and this issue mostly suggests to me that we should either avoid referring to the 'view' matrix at all in documentation or anywhere. If we have to, we can discuss it, but maybe we could try this approach first?

DGriffin91 commented 9 months ago

This is the bit from Foundations of Game Engine Development 2.

image

It uses the terminology Mcamera as the matrix for the camera itself an in: camera.transform.compute_matrix(). So Mcamera is from camera space to world space.

Note though that once it switches to using view (in the form of V), V represents the inverse camera image

image

cshenton commented 9 months ago

Yes, the "camera matrix" is the transform matrix of the camera from its local space into world space. The "view matrix" is the inverse of this matrix. This is an introductory graphics concept, and I can't find a single source that agrees with bevy's stance that "view == camera_to_world", because it's wrong!

I also think the only reason there is any confusion in your community that warrants more verbose names is because there is a massive error in the engine.

It's also not great that I've reported a bug which took me hours to track down, and the response is not to acknowledge it is bug, but to equivocate about "wasting peoples time" disccussing the issue.

At the very least, add documentation for this minor version on the wgsl and rust structs to say "hey we screwed up, this means the opposite of what it should". Such a comment would have saved me so much time.

ZainlessBrombie commented 9 months ago

I'd like to back up cshenton here: the question of what the "correct" function of the view matrix is unambigously answered by his list of references.
This is a unanimous agreement in graphics programming, there is no discussion about interpretation to be had.
I think the only reasonable way forward is to acknowledge that a serious bug has slipped through and to fix it quickly. Mistakes happen and that's okay.

Personally, naming the fields "world_to_camera" etc. and putting a doc on it that points out that this is also called the view matrix seems like a good choice to me, since that is more easily understandable for people less familiar with graphics programming.

superdump commented 9 months ago

I feel like the tone here has turned quite negative. I am listening and have good intentions.

When I wrote about ‘wasting time’ I was thinking how many of us maintaining the project have other full time jobs. Personally there are things I could do instead which would likely bring everyone more value. And I have seen discussions on topics like this become long, and take energy, time, and focus away from other things.

I also am ill at the moment, have seen this same discussion elsewhere, haven’t had the energy to find the origin of why it is the way it is in bevy, and I knew we were going to change the naming to be clearer anyway.

I wanted to look through the references in more detail to understand how those matrices are updated each frame in the code.

All that said, it seems there is consensus among many others. So now my proposal is the same as @ZainlessBrombie - that we do #8492 and mention that view to world is also known as the view matrix or view transformation. That way, we only break API once and address the bug.

ZainlessBrombie commented 9 months ago

I feel like the tone here has turned quite negative. I am listening and have good intentions.

When I wrote about ‘wasting time’ I was thinking [...]

You're right, let's take a breath here. I think there has just been a misunderstanding. Thank you, @superdump and get better soon :)

superdump commented 9 months ago

I checked git blame, and it looks like I introduced the incorrect meaning of the camera matrix as being same as the view matrix way back when I added clustered-forward rendering to Bevy.

cart commented 9 months ago

I've added this to the 0.13 milestone as I do think its worth resolving this in a timely fashion. Worth re-evaluating what naming conventions we want (ex: the long descriptive names vs the short "common" names).

superdump commented 9 months ago

@cart as far as I remember, we (you included) had previously decided on local, world, view, clip, and ndc for space names, and local_to_world etc for transformation names.

darzu commented 4 months ago

I highly favor the "y_from_x" convention for all matrices b/c matrix multiplication is left-to-right so when multiplying zFromY * yFromX * myVec it makes it clear that "myVec" is expected to start in X and the result will be in Z. Thus viewFromWorld and worldFromView instead of view and inverse. Space transforms are confusing enough it's worth the few extra letters.

More behind this rational here: https://tomforsyth1000.github.io/blog.wiki.html#%5B%5BMatrix%20maths%20and%20names%5D%5D

ricky26 commented 4 months ago

This issue seemed quite quiet but had the 0.14 milestone tag on it, so I figured I'd have a go at it, primarily as food for thought. x_from_y personally feels a little weird compared to y_to_x but I can definitely see the benefit when combining transformations. Replacing x_ and inv_x (or x_inverse) with y_from_x and x_from_y is a nice win since they line up.