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/Nerve: Finish implementation #319

Merged
merged 2 commits into from
Feb 2, 2025
Merged

Library/Nerve: Finish implementation #319

merged 2 commits into from
Feb 2, 2025

Conversation

MonsterDruide1
Copy link
Owner

@MonsterDruide1 MonsterDruide1 commented Jan 27, 2025

Well, Library/Nerve was almost finished, I noticed - so I went ahead and did everything that was still missing. With this, the entire subfolder Library/Nerve is fully implemented and matching!


This change is Reviewable

@MonsterDruide1 MonsterDruide1 self-assigned this Jan 27, 2025
@MonsterDruide1 MonsterDruide1 changed the title Library/Nerve: Implement remainder Library/Nerve: Finish implementation Jan 27, 2025
Copy link
Contributor

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

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


lib/al/Library/Nerve/NerveStateCtrl.cpp line 13 at r1 (raw file):

}

// todo -- some scheduling problems with mStateCount's incrementation

outdated comment? this function seems to be matching based on the function list file


lib/al/Library/Nerve/NerveStateCtrl.cpp line 16 at r1 (raw file):

// adds a state to the list of states in the controller
void NerveStateCtrl::addState(NerveStateBase* state, const Nerve* nerve, const char* name) {
    mStates[mStateCount] = {.state = state, .nerve = nerve, .name = name};

is it okay that this brace styling is inconsistent with the function above it? referring to using the .state/.nerve/.name parts. i wonder which one would be best to follow across the repo, or if it's fine to just have a mix of the two.


lib/al/Library/Nerve/NerveAction.h line 24 at r1 (raw file):

private:
    NerveAction* mNextAction = nullptr;
};

static_assert for class size


lib/al/Library/Nerve/NerveAction.h line 48 at r1 (raw file):

    static NerveActionCollector* sCurrentCollector;
};

static_assert for class size


lib/al/Library/Nerve/NerveUtil.cpp line 37 at r1 (raw file):

bool isStep(const IUseNerve* user, s32 step) {
    return user->getNerveKeeper()->getCurrentStep() == step;
}

why was this moved down here? it seems like it was in the right place before, between setNerveAtStep and setNerveAtGreaterEqualStep.


lib/al/Library/Nerve/NerveUtil.cpp line 209 at r1 (raw file):

f32 calcNerveEaseInValue(const IUseNerve* user, s32 min, f32 start, f32 end) {
    return lerpValue(start, end, calcNerveEaseInRate(user, min));

min -> max. same for four other functions below this one


lib/al/Library/Math/MathUtil.h line 83 at r1 (raw file):

bool isSameSign(f32, f32);

f32 easeByType(f32, s32);

will this conflict with PR #220 ?

Verified

This commit was signed with the committer’s verified signature.
emma-sg Emma Segal-Grossman
Copy link
Owner Author

@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, 7 unresolved discussions (waiting on @tetraxile)


lib/al/Library/Math/MathUtil.h line 83 at r1 (raw file):

Previously, tetraxile wrote…

will this conflict with PR #220 ?

Yes, it does. Rebased accordingly.


lib/al/Library/Nerve/NerveAction.h line 24 at r1 (raw file):

Previously, tetraxile wrote…

static_assert for class size

As there is no new allocation for this class (and the one below), adding a static_assert is not really sensible - we don't know its real size, we can only take another guess based on other information and assumptions, and that's not helpful.


lib/al/Library/Nerve/NerveAction.h line 48 at r1 (raw file):

Previously, tetraxile wrote…

static_assert for class size

Same as above


lib/al/Library/Nerve/NerveStateCtrl.cpp line 13 at r1 (raw file):

Previously, tetraxile wrote…

outdated comment? this function seems to be matching based on the function list file

Done.


lib/al/Library/Nerve/NerveStateCtrl.cpp line 16 at r1 (raw file):

Previously, tetraxile wrote…

is it okay that this brace styling is inconsistent with the function above it? referring to using the .state/.nerve/.name parts. i wonder which one would be best to follow across the repo, or if it's fine to just have a mix of the two.

Not really, I just didn't think about changing it. Done.


lib/al/Library/Nerve/NerveUtil.cpp line 37 at r1 (raw file):

Previously, tetraxile wrote…

why was this moved down here? it seems like it was in the right place before, between setNerveAtStep and setNerveAtGreaterEqualStep.

Done. Because I trusted the order of things listed in the header.


lib/al/Library/Nerve/NerveUtil.cpp line 209 at r1 (raw file):

Previously, tetraxile wrote…

min -> max. same for four other functions below this one

Done.

Copy link
Contributor

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

Reviewed 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MonsterDruide1)


lib/al/Library/Nerve/NerveAction.h line 24 at r1 (raw file):

Previously, MonsterDruide1 wrote…

As there is no new allocation for this class (and the one below), adding a static_assert is not really sensible - we don't know its real size, we can only take another guess based on other information and assumptions, and that's not helpful.

i reckon we can be certain of the struct's size based on the places where it's initialised -- they're placed consecutively in the .bss section. you make a fair point that it (seemingly) isn't written anywhere explicitly in the binary, but i'm still not super on board for this case in particular. would you intend for every struct that doesn't use a new allocation to omit the static_assert? for other structs that have much less evidence for their true size, i think it's fair, but in my view, ideally eventually we would have one of these static_asserts for every struct in the game, even just as a matter of style. perhaps a discord thread is in order?


lib/al/Library/Nerve/NerveAction.h line 48 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Same as above

reply above

Copy link
Owner Author

@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, 2 unresolved discussions (waiting on @tetraxile)


lib/al/Library/Nerve/NerveAction.h line 24 at r1 (raw file):

Previously, tetraxile wrote…

i reckon we can be certain of the struct's size based on the places where it's initialised -- they're placed consecutively in the .bss section. you make a fair point that it (seemingly) isn't written anywhere explicitly in the binary, but i'm still not super on board for this case in particular. would you intend for every struct that doesn't use a new allocation to omit the static_assert? for other structs that have much less evidence for their true size, i think it's fair, but in my view, ideally eventually we would have one of these static_asserts for every struct in the game, even just as a matter of style. perhaps a discord thread is in order?

Re-activated the discussion thread for this topic: https://discord.com/channels/774687602996936747/1249405824032444447

As we do not enforce them at the moment (and are inconsistent across the repo), I would not add them here for now - if we decide something on that discussion later, we can re-visit, but merge this PR earlier, if that's good with you.


lib/al/Library/Nerve/NerveAction.h line 48 at r1 (raw file):

Previously, tetraxile wrote…

reply above

reply above

Copy link
Contributor

@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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MonsterDruide1)


lib/al/Library/Nerve/NerveAction.h line 24 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Re-activated the discussion thread for this topic: https://discord.com/channels/774687602996936747/1249405824032444447

As we do not enforce them at the moment (and are inconsistent across the repo), I would not add them here for now - if we decide something on that discussion later, we can re-visit, but merge this PR earlier, if that's good with you.

yeah that's fair enough

@MonsterDruide1 MonsterDruide1 merged commit e0f81e8 into master Feb 2, 2025
11 checks passed
@MonsterDruide1 MonsterDruide1 deleted the al-nerves branch February 2, 2025 09:23
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.

None yet

2 participants