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

Library/Camera: Complete CameraUtil.h header #203

Merged
merged 5 commits into from
Nov 30, 2024

Conversation

tetraxile
Copy link
Contributor

@tetraxile tetraxile commented Nov 28, 2024

This change is Reviewable

Copy link
Owner

@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.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tetraxile)


lib/al/Library/Play/Camera/ActorCameraSubTarget.h line 23 at r1 (raw file):

private:
    const LiveActor* mActor;
    const sead::Vector3f* _20;

Suggestion:

const sead::Vector3f* mOffset;

lib/al/Library/Camera/CameraUtil.h line 47 at r1 (raw file):

const sead::Matrix34f& getProjectionMtx(const SceneCameraInfo* info, s32 viewIdx);
sead::Matrix34f* getProjectionMtxPtr(const IUseCamera* user, s32 viewIdx);
sead::Matrix34f* getProjectionMtxPtr(const SceneCameraInfo* info, s32 viewIdx);

Suggestion:

const sead::Matrix44f& getProjectionMtx(const IUseCamera* user, s32 viewIdx);
const sead::Matrix44f& getProjectionMtx(const SceneCameraInfo* info, s32 viewIdx);
sead::Matrix44f* getProjectionMtxPtr(const IUseCamera* user, s32 viewIdx);
sead::Matrix44f* getProjectionMtxPtr(const SceneCameraInfo* info, s32 viewIdx);

lib/al/Library/Camera/CameraUtil.h line 53 at r1 (raw file):

sead::Projection* getProjectionSead(const SceneCameraInfo* info, s32 viewIdx);
Projection* getProjection(const IUseCamera* user, s32 viewIdx);
Projection* getProjection(const SceneCameraInfo* info, s32 viewIdx);

All of them return const T&s, as can be seen in the constructor of al::CameraViewInfo

Code quote:

sead::LookAtCamera* getLookAtCamera(const IUseCamera* user, s32 viewIdx);
sead::LookAtCamera* getLookAtCamera(const SceneCameraInfo* info, s32 viewIdx);
sead::Projection* getProjectionSead(const IUseCamera* user, s32 viewIdx);
sead::Projection* getProjectionSead(const SceneCameraInfo* info, s32 viewIdx);
Projection* getProjection(const IUseCamera* user, s32 viewIdx);
Projection* getProjection(const SceneCameraInfo* info, s32 viewIdx);

lib/al/Library/Camera/CameraUtil.h line 245 at r1 (raw file):

void setCameraPlacementSubTarget(IUseCamera* user, CameraSubTargetBase* target);
void resetCameraPlacementSubTarget(IUseCamera* user, CameraSubTargetBase* target);
CameraDistanceCurve* getCameraDistanceRocketFlowerCurve();

Suggestion:

const CameraDistanceCurve* getCameraDistanceRocketFlowerCurve();

lib/al/Library/Camera/CameraUtil.h line 272 at r1 (raw file):

bool isAnimCameraAnimPlaying(const CameraTicket* ticket, const char*);
s32 getAnimCameraStepMax(const CameraTicket* ticket);
s32 getAnimCameraStep(const CameraTicket* ticket);

Are you really sure about those, where did you get them from?

Code quote:

s32 getAnimCameraStepMax(const CameraTicket* ticket);
s32 getAnimCameraStep(const CameraTicket* ticket);

lib/al/Library/Obj/CameraWatchPoint.h line 4 at r1 (raw file):

#include "Library/LiveActor/LiveActor.h"
#include "Library/Play/Camera/ActorCameraSubTarget.h"

Forward-declare instead

Copy link
Contributor Author

@tetraxile tetraxile left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 7 files reviewed, 6 unresolved discussions (waiting on @MonsterDruide1)


lib/al/Library/Camera/CameraUtil.h line 53 at r1 (raw file):

Previously, MonsterDruide1 wrote…

All of them return const T&s, as can be seen in the constructor of al::CameraViewInfo

done


lib/al/Library/Camera/CameraUtil.h line 272 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Are you really sure about those, where did you get them from?

i don't really understand what it's trying to access but i was able to at least rule out f32 because it returns the value in w0 rather than s0. beyond that i don't know, it could be u32 or some other type but based on other instances of "animation frame steps" i think s32 makes the most sense, as they often have a default value of -1.


lib/al/Library/Obj/CameraWatchPoint.h line 4 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Forward-declare instead

i included this because i need to show the compiler that ActorCameraSubTarget inherits from CameraSubTargetBase. i moved it into the source file instead.


lib/al/Library/Camera/CameraUtil.h line 47 at r1 (raw file):

const sead::Matrix34f& getProjectionMtx(const SceneCameraInfo* info, s32 viewIdx);
sead::Matrix34f* getProjectionMtxPtr(const IUseCamera* user, s32 viewIdx);
sead::Matrix34f* getProjectionMtxPtr(const SceneCameraInfo* info, s32 viewIdx);

right, must've forgotten to check the inner functions here


lib/al/Library/Camera/CameraUtil.h line 245 at r1 (raw file):

void setCameraPlacementSubTarget(IUseCamera* user, CameraSubTargetBase* target);
void resetCameraPlacementSubTarget(IUseCamera* user, CameraSubTargetBase* target);
CameraDistanceCurve* getCameraDistanceRocketFlowerCurve();

done


lib/al/Library/Play/Camera/ActorCameraSubTarget.h line 23 at r1 (raw file):

private:
    const LiveActor* mActor;
    const sead::Vector3f* _20;

done

Copy link
Owner

@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.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tetraxile)


lib/al/Library/Camera/CameraUtil.h line 272 at r1 (raw file):

Previously, tetraxile wrote…

i don't really understand what it's trying to access but i was able to at least rule out f32 because it returns the value in w0 rather than s0. beyond that i don't know, it could be u32 or some other type but based on other instances of "animation frame steps" i think s32 makes the most sense, as they often have a default value of -1.

Oh, true, that leaves either s32 or u32. Verifying this requires understanding the nn::g3d::ResFile-stuff a lot closer, and ... this difference probably doesn't even matter enough here. Let's just go with s32.

@MonsterDruide1 MonsterDruide1 merged commit 80afd49 into MonsterDruide1:master Nov 30, 2024
6 checks passed
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