Conversation
|
I just pushed an update that added some things to the Camera, Projection, and Viewport libs. In a few cases, the matrix math can likely be simplified by some of the internal CalcCommon functions, in other cases I'm looking for, "What am I missing?" For instance, I think once |
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 of 10 files at r2, 3 of 8 files at r4, all commit messages.
Reviewable status: 4 of 14 files reviewed, 7 unresolved discussions
a discussion (no related file):
I will slowly review this file-by-file.
modules/src/gfx/seadProjection.cpp line 284 at r4 (raw file):
{ offset->x = ((float)0.5 * (this->mLeft + this->mRight)) / (this->mRight - this->mLeft); offset->y = ((float)0.5 * (this->mTop + this->mBottom)) / (this->mTop - this->mBottom);
Suggestion:
offset->x = (0.5f * (mLeft + mRight)) / (mRight - mLeft);
offset->y = (0.5f * (mTop + mBottom)) / (mTop - mBottom);include/gfx/seadDrawContext.h line 15 at r4 (raw file):
public: DrawContext(); virtual ~DrawContext() = default;
Not header-defined
Suggestion:
virtual ~DrawContext();include/gfx/seadProjection.h line 145 at r4 (raw file):
f32 mFovyCos; f32 mFovyTan; f32 mAspect = 1.333333f;
Suggestion:
f32 mAspect = 4.0f/3.0f;include/gfx/seadCamera.h line 15 at r4 (raw file):
class Ray : public T { };
This seems wrong. Looking at sead::Camera::unprojectRayByMatrix, Ray<sead::Vector3f> contains at least two sead::Vector3f. Also, : public T is very unusual. I would rather suggest the below, based on these two facts.
Also, mStart and mEnd come from 7100B67F34 in SMO, which does something like this:
sead::Vector2f a = _0;
sead::Vector2f b = _8 - _0;
I would definitely leave in a note that those names are not really known and just a wild guess here.
Suggestion:
class Ray
{
// TODO: names are very guessed
T mStart;
T mEnd;
};include/gfx/seadCamera.h line 84 at r4 (raw file):
SEAD_RTTI_OVERRIDE(DirectCamera, Camera) public: DirectCamera();
inlined in its usages
Suggestion:
DirectCamera() = default;include/gfx/seadCamera.h line 91 at r4 (raw file):
private: Matrix34f mDirectMatrix = Matrix34f::ident; // I don't think this is right, the Matrix has 0, 2 // and 1, 3 populated as 1.0f
Seems right to me, I literally see a memcpy(someOffsetWithinDirectCamera, &sead::Matrix34f::ident, 0x30);
Suggestion:
Matrix34f mDirectMatrix = Matrix34f::ident;include/gfx/seadCamera.h line 98 at r4 (raw file):
SEAD_RTTI_OVERRIDE(OrthoCamera, LookAtCamera) public: OrthoCamera() = default;
This one is not inlined.
Suggestion:
OrthoCamera();|
I have an implementation of template <typename T>
struct Ray
{
Ray();
T position;
T direction;
};I've changed the constructors, is there something I'm missing in terms of when they're |
|
Should be in |
…eedback. Remove extraneous 'this->' from a function.
…:getWorldPosByMatrix.
…txt, update the function definitions in seadGeometry (even though they are commented out), and match Camera::worldPosToCameraPosByMatrix.
…ted while working through other Projection code.
a1536c0 to
91186da
Compare
MonsterDruide1
left a comment
There was a problem hiding this comment.
If it shows up as a function in the game, it's implemented in the cpp. If it's inlined in all usages, it's header-defined.
If the constructor has no content, it can often be defined as = default; (preferred). Sometimes, this doesn't match for some reason, so try {} too if you're having problems.
@MonsterDruide1 reviewed 1 of 1 files at r1, 1 of 9 files at r6, 1 of 5 files at r7, 1 of 5 files at r8, 1 of 1 files at r10, 1 of 1 files at r12, all commit messages.
Reviewable status: 7 of 17 files reviewed, 9 unresolved discussions (waiting on @Esras)
modules/src/math/seadGeometry.cpp line 6 at r12 (raw file):
{ float Geometry::calcSquaredDistancePointToRay(Vector2<float>* point, Ray<Vector2<float>>* ray,
Suggestion:
{
// BUG: returns the distance from point to ray origin, ignoring its direction
// (closestRayPos is fine)
float Geometry::calcSquaredDistancePointToRay(Vector2<float>* point, Ray<Vector2<float>>* ray,modules/src/math/seadGeometry.cpp line 7 at r12 (raw file):
float Geometry::calcSquaredDistancePointToRay(Vector2<float>* point, Ray<Vector2<float>>* ray, float* scalar)
Suggestion:
f32 Geometry::calcSquaredDistancePointToRay(Vector2f* point, Ray2f* ray, f32* closestRayPos)modules/src/math/seadGeometry.cpp line 11 at r12 (raw file):
auto diff = *point - ray->position; auto numerator = diff.dot(ray->direction);
Suggestion:
auto dot = diff.dot(ray->direction);modules/src/math/seadGeometry.cpp line 12 at r12 (raw file):
auto numerator = diff.dot(ray->direction); auto squaredLength = diff.squaredLength();
avoid auto in general
Suggestion:
sead::Vector2f diff = *point - ray->position;
f32 numerator = diff.dot(ray->direction);
f32 squaredLength = diff.squaredLength();include/geom/seadLine.h line 12 at r12 (raw file):
{ public: using T = typename VectorType::ValueType;
... or is that used anywhere?
Suggestion:
template <typename T>
class Segment
{include/geom/seadLine.h line 15 at r12 (raw file):
public: Segment() : mP0(VectorType::zero), mP1(VectorType::ex) {}
Use VectorType mP0 = VectorType::zero; (similar for mP1) instead
Suggestion:
Segment() = default;include/geom/seadLine.h line 42 at r12 (raw file):
public: T position; T direction;
Suggestion:
public:
const VectorType& getPos() const { return mP; }
void setPos(const VectorType& p) { mP = p; }
const VectorType& getDir() const { return mD; }
void setDir(const VectorType& d) { mD = d; }
private:
T mP;
T mD;CMakeLists.txt line 116 at r4 (raw file):
modules/src/gfx/seadCamera.cpp modules/src/gfx/seadColor.cpp modules/src/gfx/seadDrawContext.cpp
Why is this one removed?
|
I just noticed you implemented projection here. I just did just the base projection implementation in #228 Since it wasn't my intention to "steal" any of your work I can either add you as a co-autor or close my PR and help this PR so is up to standard and get it merged. |
Try 2.
This PR attempts to implement many of the Projection libraries and tries to update their dependencies where needed and from @aboood40091's repo.
Goals
Addresses
0xB1D898-0xB1F2B0are the bulk of the Projection functions in BotW.0xB1FFF0-0xB20718are thesead::ViewportfunctionsWaiting on dependencies
Projection::unproject(Ray<>, ...)Non-Matching Functions
Viewport::applyViewport::applyViewportViewport::applyScissorProjection::doScreenPosToCameraPosTo(sead::Vector3<float>*, sead::Vector3<float> const&) constIn trying to remember from when I was doing this originally, this looks like it's mostly just registers moving around?
Projection::project(sead::Vector2<float>*, sead::Vector3<float> const&, sead::Viewport const&) constIf I'm reading this right, it's just loading an offset into the register and using that to jump instead of jumping to the absolute address - could be because of other function sizes messed up?
Projection::unproject(sead::Vector3<float>*, sead::Vector3<float> const&, sead::Camera const&) constProjection::doUpdateDeviceMatrix(sead::Matrix44<float>*, sead::Matrix44<float> const&, sead::Graphics::DevicePosture) constPerspectiveProjection::PerspectiveProjection()OrthoProjection::OrthoProjection()OrthoProjection::OrthoProjection(float, float, sead::Viewport const&)OrthoProjection::setByViewport(sead::Viewport const&)OrthoProjection::doUpdateMatrix(sead::Matrix44<float>*) constFrustumProjection::doScreenPosToCameraPosTo(Vector3f, Vector3f)DirectProjection::DirectProjection()DirectProjection::DirectProjection(sead::Matrix44<float> const*, sead::Graphics::DevicePosture)DirectProjection::setDirectProjectionMatrix(sead::Matrix44<float> const*, sead::Graphics::DevicePosture)DirectProjection::updateAttributesForDirectProjection()DirectProjection::doUpdateMatrix(sead::Matrix44<float>*) constDirectProjection::doScreenPosToCameraPosTo(sead::Vector3<float>*, sead::Vector3<float> const&) constIn the prior PR, the static_assert in
seadGraphicsNvn.hhad been commented out because it was failing while editing. I'm not entirely sure if that's related to the work here.Next Steps
I'm looking for feedback on the non-matching functions, and which of the library files that have been modified that need to be updated here.
Note that in the two years since I wrote the initial one, I'm not entirely sure about what I mean by the library files anymore. I think mostly Ray.
This change is