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

MapObj: Implement FurnitureStateWait #280

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

Naii-the-Baf
Copy link
Contributor

@Naii-the-Baf Naii-the-Baf commented Jan 16, 2025

Wait state calls for beds and chairs. Probably used when Mario sits or sleeps on things.


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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Naii-the-Baf)


src/MapObj/FurnitureStateWait.cpp line 52 at r1 (raw file):

    bool isPlayerOnGround = rs::isPlayerOnGround(actor);
    const sead::Vector3f& actorPos = rs::getPlayerPos(actor);

Suggestion:

const sead::Vector3f& newPlayerPos = rs::getPlayerPos(actor);

src/MapObj/FurnitureStateWait.cpp line 53 at r1 (raw file):

    bool isPlayerOnGround = rs::isPlayerOnGround(actor);
    const sead::Vector3f& actorPos = rs::getPlayerPos(actor);
    bool isNearZero = al::isNearZero(mPlayerPos - actorPos, 0.001f);

Suggestion:

bool isPlayerStationary = al::isNearZero(mPlayerPos - actorPos, 0.001f);

src/MapObj/FurnitureStateWait.cpp line 68 at r1 (raw file):

        f32 angle = al::calcAngleOnPlaneDegree(frontDir, plane, sead::Vector3f::ey);
        angle = sead::Mathf::abs(angle);
        _24 = 90.0f < angle;

Suggestion:

        _24 = 90.0f < sead::Mathf::abs(angle);

src/MapObj/FurnitureStateWait.cpp line 77 at r1 (raw file):

        return;
    _24 = 2;
    al::setNerve(this, &NrvFurnitureStateWait.BindRequest);

Your way of writing this is preferred in general. However, with Nerves being a kind-of state machine, the "standard path" is not switching state - so isGreaterEqualStep is the special case and should have its code indented.

Suggestion:

    s32 waitTime = (mFurnitureType == FurnitureType::Chair) ? 30 : 120;
    if (al::isGreaterEqualStep(this, waitTime)) {
        _24 = 2;
        al::setNerve(this, &NrvFurnitureStateWait.BindRequest);
    }

src/MapObj/FurnitureStateWait.cpp line 97 at r1 (raw file):

        return;

    al::setNerve(this, &NrvFurnitureStateWait.Wait);

Same here - player moving off of chair/bed => switching state is the special case.

Suggestion:

    if (!isPlayerOnSomething)
        al::setNerve(this, &NrvFurnitureStateWait.Wait);

src/MapObj/FurnitureStateWait.h line 12 at r1 (raw file):

}  // namespace al

enum FurnitureType : u64 {

Suggestion:

enum class FurnitureType : u32 {

src/MapObj/FurnitureStateWait.h line 26 at r1 (raw file):

private:
    u32 mFurnitureType;

Suggestion:

FurnitureType mFurnitureType;

src/MapObj/FurnitureStateWait.h line 27 at r1 (raw file):

private:
    u32 mFurnitureType;
    u32 _24 = 3;

Seems like that could be another enum with four values about the player:
0: in-air, facing "along" the furniture
1: in-air, otherwise
2: being binded to the object (sitting/... on it)
3: on-ground, not bounded to object

Keep in mind that appear is also called when the player "unbinds" from the object, since the StateWait will be killed when sitting down and "appear" again afterwards.

Copy link
Contributor Author

@Naii-the-Baf Naii-the-Baf 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: all files reviewed, 8 unresolved discussions (waiting on @MonsterDruide1)


src/MapObj/FurnitureStateWait.h line 27 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Seems like that could be another enum with four values about the player:
0: in-air, facing "along" the furniture
1: in-air, otherwise
2: being binded to the object (sitting/... on it)
3: on-ground, not bounded to object

Keep in mind that appear is also called when the player "unbinds" from the object, since the StateWait will be killed when sitting down and "appear" again afterwards.

Done.


src/MapObj/FurnitureStateWait.cpp line 77 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Your way of writing this is preferred in general. However, with Nerves being a kind-of state machine, the "standard path" is not switching state - so isGreaterEqualStep is the special case and should have its code indented.

Done.


src/MapObj/FurnitureStateWait.cpp line 97 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Same here - player moving off of chair/bed => switching state is the special case.

Done.


src/MapObj/FurnitureStateWait.cpp line 52 at r1 (raw file):

    bool isPlayerOnGround = rs::isPlayerOnGround(actor);
    const sead::Vector3f& actorPos = rs::getPlayerPos(actor);

Done.


src/MapObj/FurnitureStateWait.cpp line 53 at r1 (raw file):

    bool isPlayerOnGround = rs::isPlayerOnGround(actor);
    const sead::Vector3f& actorPos = rs::getPlayerPos(actor);
    bool isNearZero = al::isNearZero(mPlayerPos - actorPos, 0.001f);

Done.


src/MapObj/FurnitureStateWait.cpp line 68 at r1 (raw file):

        f32 angle = al::calcAngleOnPlaneDegree(frontDir, plane, sead::Vector3f::ey);
        angle = sead::Mathf::abs(angle);
        _24 = 90.0f < angle;

Done.
Because _24 is now an enum, this has been replaced with an if-else.


src/MapObj/FurnitureStateWait.h line 12 at r1 (raw file):

}  // namespace al

enum FurnitureType : u64 {

Done.
Doing this will cause a mismatch in the constructor function, as the binary seems to be casting a 64-bit number to a 32-bit one. However, the alternative is to cast mFurnitureType to a 32-bit size while changing its type to FurnitureType, which seems to not cause any mismatches.


src/MapObj/FurnitureStateWait.h line 26 at r1 (raw file):

private:
    u32 mFurnitureType;

Done. See above.

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 @Naii-the-Baf)

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Naii-the-Baf)


src/MapObj/FurnitureStateWait.h line 33 at r2 (raw file):

private:
    FurnitureType mFurnitureType : 32;

The linter doesn't like this - fix the linter as instructed via Discord to allow the pipeline(s) to pass.

Code quote:

FurnitureType mFurnitureType : 32;

Copy link
Contributor Author

@Naii-the-Baf Naii-the-Baf 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 5 files reviewed, 1 unresolved discussion (waiting on @MonsterDruide1)


src/MapObj/FurnitureStateWait.h line 33 at r2 (raw file):

Previously, MonsterDruide1 wrote…

The linter doesn't like this - fix the linter as instructed via Discord to allow the pipeline(s) to pass.

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 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Naii-the-Baf)

@MonsterDruide1 MonsterDruide1 changed the title MapObj: Implement FurnitureStateWait MapObj: Implement FurnitureStateWait Jan 22, 2025
@MonsterDruide1 MonsterDruide1 merged commit 3de2bfd into MonsterDruide1:master Jan 22, 2025
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