gfx: Partially implement Projection#228
Conversation
c21a532 to
1f64553
Compare
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 5 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @german77).
include/math/seadVectorCalcCommon.h line 45 at r2 (raw file):
/// Apply a transformation `m` (rotation then translation) to the vector `a`. static void mul(Base& o, const Mtx34& m, const Base& a); /// Apply a transformation `m` (rotation, translation, normalize) to the vector `a`.
Suggestion:
/// Apply a transformation `m` (rotation, translation, homogenous coordinates) to the vector `a`.include/math/seadVectorCalcCommon.h line 46 at r2 (raw file):
static void mul(Base& o, const Mtx34& m, const Base& a); /// Apply a transformation `m` (rotation, translation, normalize) to the vector `a`. static void mul(Base& o, const Mtx44& m, const Base& a);
Add operator*, mul and setMul to Vector.h/Vector.hpp as well
modules/src/gfx/seadProjection.cpp line 55 at r2 (raw file):
void Projection::cameraPosToScreenPos(Vector3f* screen_pos, const Vector3f& camera_pos) const { Vector3CalcCommon<f32>::mul(*screen_pos, getProjectionMatrix(), camera_pos);
Use operator*, mul or setMul, whichever works
include/gfx/seadProjection.h line 40 at r2 (raw file):
const Matrix44f& getProjectionMatrix() const; void updateMatrixImpl_() const; Matrix44f& getProjectionMatrixMutable();
Suggestion:
Matrix44f* getProjectionMatrixMutable();
german77
left a comment
There was a problem hiding this comment.
Uh. Something weird came up. During review al::CameraVerticalAbsorber dtor mismatched due not calling the correct function. Well after checking I noticed the vtable contains OrthoProjection entries this only means PerspectiveProjection depends on OrthoProjection.
The issue appears on DirectProjection. It apparently overrides members from OrthoProjection with a Matrix44f which seems really wrong to me.
@german77 made 5 comments.
Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @MonsterDruide1).
include/math/seadVectorCalcCommon.h line 46 at r2 (raw file):
Previously, MonsterDruide1 wrote…
Add
operator*,mulandsetMultoVector.h/Vector.hppas well
Done.
modules/src/gfx/seadProjection.cpp line 55 at r2 (raw file):
Previously, MonsterDruide1 wrote…
Use
operator*,mulorsetMul, whichever works
Done.
include/gfx/seadProjection.h line 40 at r2 (raw file):
const Matrix44f& getProjectionMatrix() const; void updateMatrixImpl_() const; Matrix44f& getProjectionMatrixMutable();
Done.
include/math/seadVectorCalcCommon.h line 45 at r2 (raw file):
/// Apply a transformation `m` (rotation then translation) to the vector `a`. static void mul(Base& o, const Mtx34& m, const Base& a); /// Apply a transformation `m` (rotation, translation, normalize) to the vector `a`.
Done.
c52a8ec to
c776698
Compare
|
On a second look I was wrong about inheritance. |
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 5 files and all commit messages, and resolved 4 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @german77).
I will be working with projection for a while and needed a couple of functions. I noticed
updateMatrixImpl_didn't match so I went ahead and fixed the function and implemented the rest with the exception ofdoUpdateDeviceMatrixsince I couldn't get a match on this one.This change is