Skip to content

gfx: Partially implement Projection#228

Merged
MonsterDruide1 merged 4 commits intoopen-ead:masterfrom
german77:projection
Jan 14, 2026
Merged

gfx: Partially implement Projection#228
MonsterDruide1 merged 4 commits intoopen-ead:masterfrom
german77:projection

Conversation

@german77
Copy link
Contributor

@german77 german77 commented Nov 20, 2025

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 of doUpdateDeviceMatrix since I couldn't get a match on this one.


This change is Reviewable

@german77 german77 force-pushed the projection branch 2 times, most recently from c21a532 to 1f64553 Compare November 20, 2025 19:07
@german77 german77 mentioned this pull request Nov 20, 2025
Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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();

Copy link
Contributor Author

@german77 german77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*, mul and setMul to Vector.h/Vector.hpp as well

Done.


modules/src/gfx/seadProjection.cpp line 55 at r2 (raw file):

Previously, MonsterDruide1 wrote…

Use operator*, mul or setMul, 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.

@german77 german77 force-pushed the projection branch 2 times, most recently from c52a8ec to c776698 Compare January 13, 2026 18:16
@german77
Copy link
Contributor Author

On a second look I was wrong about inheritance.

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MonsterDruide1 reviewed 5 files and all commit messages, and resolved 4 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @german77).

@MonsterDruide1 MonsterDruide1 merged commit 0da4e3c into open-ead:master Jan 14, 2026
4 checks passed
@german77 german77 deleted the projection branch January 14, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants