search

servo / webxr

Bindings for WebXR
Mozilla Public License 2.0
81 stars 24 forks source link

Row-major -> Column-major on projection matrix #206

Closed msub2 closed 1 year ago

msub2 commented 1 year ago

As discovered during my experimentation the projection matrix was being constructed in row-major order, but WebXR expects it to be column-major.

msub2 commented 1 year ago

r? @jdm

Manishearth commented 1 year ago

n.b. this may be mixing up with the row/column vector representation of transforms (a row major row vector is equivalent to a column major column vector). I don't remember if we updated Euclid correctly so we should review this carefully

(the fact that it fixes things is a promising signal, but the actual bug may be elsewhere)

msub2 commented 1 year ago

This seems like the most relevant commit on Euclid's side https://github.com/servo/euclid/commit/5dba55755a43f220112ac2c37cd2c61d81d9e012. It looks like that column major change would've first been included in v21, so I believe this commit https://github.com/servo/webxr/commit/2c82ad02990ea807c2f28dc01e140b7f077c1597 updating euclid from v20 to v22 is when it would have broke.

Looking at that commit closer you can actually see that Transform3D::row_major and Transform3D::column_major were both converted to Transform3D::new without modifying the existing order of elements.

Manishearth commented 1 year ago

That makes sense. Yeah, I wasn't sure if that upgrade had happened; if it had it's a case of this code not having been upgraded with it.

jdm commented 1 year ago

@bors-servo r=manishearth

bors-servo commented 1 year ago

:pushpin: Commit a20cabb has been approved by manishearth

bors-servo commented 1 year ago

:hourglass: Testing commit a20cabb8cc853437114a9a034f4394ae3d06a991 with merge 46bbf84093add7142f2c183755bd35282bbed2f6...

bors-servo commented 1 year ago

:sunny: Test successful - checks-github Approved by: manishearth Pushing 46bbf84093add7142f2c183755bd35282bbed2f6 to master...