most of AirOctaFlyUp is done, some of AirOctaDataMgr is done

[bomba1749]

This change is 

[leoetlino]

So sorry for the delay - I'll take a look at the latest changes this weekend (and hopefully this can be merged then)

[leoetlino]

Reviewed 1 of 1 files at r24, 2 of 2 files at r26, 1 of 1 files at r27, 1 of 1 files at r28, 1 of 2 files at r29, 1 of 1 files at r31, all commit messages.
Reviewable status: 17 of 18 files reviewed, 4 unresolved discussions (waiting on @bomba1749)

---

src/Game/AI/AI/aiAirOctaFlyUp.cpp line 29 at r31 (raw file):

    auto dt = ksys::VFR::instance()->getDeltaTime();
    mElapsedTime += dt;
    auto min = (sead::Mathf::min(1.0f, mElapsedTime / *mFlyUpDuration_s) * 2.f) - 1.f;

sorry, just to clarify my previous comments:

• min(1.0f, mElapsedTime / *mFlyUpDuration_s) is what should be called fly_up_cycles because it's a duration divided by the duration of one fly up cycle - that gives you the number of cycles that have occurred in the elapsed time period. the min(1.0f, ...) part is just to ensure that you have at least one cycle
• 2.0f * fly_up_cycles - 1.0f (what you are calling min) should probably be renamed, but I don't have a good suggestion. maybe adj_fly_up_cycles or something since it's just the number of fly up cycles but adjusted
• what you are calling fly_up_cycles in the current diff should be renamed to something like fly_up_multiplier because it acts as a multiplier for the target distance, and it's no longer a number of cycles since it's the result of an exponential
• it looks like literally every usage of what you're calling fly_up_cycles is followed by a multiplication by 0.5f, so consider moving that into the definition of the multiplier variable

---

src/KingSystem/ActorSystem/actActor.h line 21 at r23 (raw file):

Previously, leoetlino (Léo Lam) wrote…

please forward declare the damage manager thing as well - we really want to avoid unnecessary includes as that'll slow down builds a lot

(also, as a rule of thumb, we shouldn't be including a Game/ header in KingSystem/ because Game is built upon KingSystem and not the other way round - I wonder if that means DamageManager should actually be in KingSystem rather than in Game... not something to fix in this PR anyway.)

this comment still applies

---

src/KingSystem/ActorSystem/actActor.h line 293 at r31 (raw file):

    sead::Atomic<bool>& get68f() { return _68f; }
    float get6f0() const { return _6f0; }
    uking::dmg::DamageManagerBase* getDamageMgrDerived();

well, which is it? does this return the base class or the derived class? (you should either fix the return type or rename the function - right now it's sending mixed signals)

[bomba1749]

ok the naming issues should be all fixed, i got damagemgrbase forward declared and just removed getdamagemgrderived because getDamageMgr does the exact same thing and it isnt actually being used in airoctaflyup

[leoetlino]

you sure you got rid of getDamageMgrDerived? I still see it in the diff (also, there are 2 other unresolved comments)

Reviewed 1 of 1 files at r32, 1 of 1 files at r33, all commit messages.
Reviewable status: 17 of 18 files reviewed, 2 unresolved discussions (waiting on @bomba1749)

[bomba1749]

ok i actually uploaded actactor.h this time :P. I got all of the other comments done, but ive not made fly_up_cycles being *.5 in the multiplier variable bc that leads to it diffing a lot more. Also, ive chosen to name min fly_up_turning_pts- if its a physical cycle like a sine wave it's going to have (2x the number of cycles -1) of peaks and troughs.

[leoetlino]

Sorry for the delay in re-reviewing this. This mostly looks good to me now, but there are still 3 unresolved comments and some formatting issues (you can automatically fix them by running clang-format on the files you've edited)

Reviewed 1 of 1 files at r30, 1 of 1 files at r34, 1 of 1 files at r36, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bomba1749)

---

data/uking_functions.csv line 0 at r36 (raw file):
Seems like there are two WIP functions - could you update their status to either matching or non-matching?

[leoetlino]

Reviewed 1 of 1 files at r37, 1 of 1 files at r38, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bomba1749)

[leoetlino]

It seems the linter is unhappy about some of the formatting - could you run clang-format on the files it's complaining about? lgtm other than that

[bomba1749]

done!!

[leoetlino]

Reviewed 1 of 1 files at r39, 1 of 1 files at r40, 1 of 1 files at r41, 1 of 1 files at r42, 1 of 1 files at r43, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bomba1749)