lots of AreaObj.* functions #23

whaleymar · 2024-03-08T04:28:37Z

pasted PR comment from the other repo:

This covers

Reversed AreaObjDirector's methods and fixed some errors in its header
AreaObjFactory header + reversed both methods
Headers for AreaObjMtxConnecter and AreaObjMtxConnecterHolder
2 inlined functions in Factory to get AreaObj + AreaObjDirector stuff matching
Headers for area objects used in the factory (includes TrafficArea, CameraStartParam, SeBarrierArea, SePlayArea, ViewCtrlArea, BirdGatheringSpotArea, ExtForceArea, ForceRecoveryKidsArea, MoveArea2D, NpcForceMaterialCodeArea, RouteGuideArea, and StainArea)
ProjectAreaFactory header and a non-matching constructor

Things to sanity check:

AreaObjDirector::getAreaObjGroup did not match when using the inlined _getAreaObjGroup. Instead, it only matches when I paste the exact same code into the public method. Inlining the binary search was necessary for several functions to match, but having 2 identical functions feels wrong.
I don't know if I defined BirdGatheringSpotArea::AreaClippingInfo in the right spot

This change is

whaleymar · 2024-03-08T04:35:35Z

I'll deal with the linting stuff later I'm going to bed

MonsterDruide1 · 2024-03-08T13:57:53Z

Alright, I'll review when the linter is (mostly) fixed.

MonsterDruide1

Reviewed 2 of 29 files at r1, 26 of 26 files at r2.
Reviewable status: 28 of 29 files reviewed, 26 unresolved discussions (waiting on @whaleymar)

lib/al/include/Library/Area/AreaInitInfo.h line 6 at r2 (raw file):

namespace al {
class PlacementInfo;

It has to be included either way (to be able to inherit from it), so this is useless.

lib/al/include/Library/Area/AreaInitInfo.h line 12 at r2 (raw file):

class AreaInitInfo : public PlacementInfo {
public:
    AreaInitInfo(){};

Defining it in the header means that the function is not generated in the binary. As this constructor does exist in the game, this seems wrong.

lib/al/include/Library/Area/AreaObj.h line 25 at r2 (raw file):

    virtual void init(const AreaInitInfo& initInfo);
    virtual bool isInVolume(const sead::Vector3f& position) const;
    virtual bool isInVolumeOffset(const sead::Vector3f&, f32 offset) const;

Naming this as position for consistency across the functions?

Code quote:

const sead::Vector3f&

lib/al/include/Library/Area/AreaObjDirector.h line 37 at r2 (raw file):

    u32 mAreaGroupCount = 0;

    AreaObjGroup* _getAreaObjGroup(const char* name) const {

I don't think this belongs in the header, but I'll do some experiments later.

lib/al/include/Library/Area/AreaObjMtxConnecter.h line 41 at r2 (raw file):

private:
    MtxConnector** mMtxConnectors;

Isn't this a al::AreaObjMtxConnecter**?

lib/al/include/Library/Area/TrafficArea.h line 17 at r2 (raw file):

    bool mNpcFull = false;
    bool mNpcUnavailable = false;
    char mUnkChar = '\0';

Considering the above is mNpcUnavailable, this should probably be bool mCarUnavailable.

lib/al/include/Library/Factory/Factory.h line 35 at r2 (raw file):

    s32 getNumFactoryEntries() const { return mNumFactoryEntries; }

    s32 getEntryIndex(const char* entryName, T* creationPtr) const {

If that still matches, I would prefer to swap the two parameters to have T*, const char*. Other functions in this game have it that way around (out, in1, in2, ...).

lib/al/include/Library/Play/Area/ViewCtrlArea.h line 4 at r2 (raw file):

#include "Library/Area/AreaObj.h"
#include "Library/Placement/PlacementId.h"

Do not include, but just declare as class PlacementId;

lib/al/src/Library/Area/AreaObj.cpp line 44 at r2 (raw file):

    mAreaShape->setBaseMtxPtr(&mMatrixTR);
    sead::Vector3f scale({1.0, 1.0, 1.0});

sead::Vector3f scale = {1.0, 1.0, 1.0}; for consistency with = nullptr from above

lib/al/src/Library/Area/AreaObjDirector.cpp line 19 at r2 (raw file):

    mMtxConnecterHolder = new AreaObjMtxConnecterHolder(0x100);
    s32 nFactoryEntries = mFactory->getNumFactoryEntries();
    s32 areaGroupsSize = nFactoryEntries;

Is this double-copying required?

lib/al/src/Library/Area/AreaObjDirector.cpp line 79 at r2 (raw file):

void AreaObjDirector::createAreaObjGroupBuffer() {
    for (s32 i = 0; i < mFactory->getAreaGroupCount(); i++) {
        AreaGroupInfo* addBuffer = mFactory->getAreaGroupInfo() + i;

Ugly pointer arithmetic.
Suggestion: AreaGroupInfo& addBuffer = mFactory->getAreaGroupInfo()[i];, if that matches.

lib/al/src/Library/Area/AreaObjDirector.cpp line 108 at r2 (raw file):

void AreaObjDirector::placementAreaObj(const AreaInitInfo& initInfo) {
    PlacementInfo pInfo(initInfo);

Should not be required, I think - just using initInfo everywhere should be fine, as it is the superclass

lib/al/src/Library/Area/AreaObjDirector.cpp line 127 at r2 (raw file):

        areaObj->init(initInfo2);

        AreaObjGroup* areaGroup = _getAreaObjGroup(pInfoName);

Maybe calling getAreaObjGroup instead, removing the need for _getAreaObjGroup in the first place?

lib/al/src/Library/Area/AreaObjDirector.cpp line 135 at r2 (raw file):

AreaObjGroup* AreaObjDirector::getAreaObjGroup(const char* name) const {
    // return _getAreaObjGroup(name); // doesn't match

(see above)

lib/al/src/Library/Area/AreaObjDirector.cpp line 152 at r2 (raw file):

bool AreaObjDirector::isExistAreaGroup(const char* name) const {
    return _getAreaObjGroup(name) != nullptr;

(see above)

lib/al/src/Library/Area/AreaObjDirector.cpp line 156 at r2 (raw file):

AreaObj* AreaObjDirector::getInVolumeAreaObj(const char* name, const sead::Vector3f& position) {
    AreaObjGroup* areaGroup = _getAreaObjGroup(name);

(see above)

lib/al/src/Library/Area/AreaObjFactory.cpp line 19 at r2 (raw file):

            return 0;
    }
    return mAreaGroupInfo[offset].size;

Using a for-loop instead?

for (s32 i=0; i<mNumBuffers; i++) {
  if (isEqualString(...)) return mAreaGroupInfo[offset].size;
}
return 0;

Code quote:

    s32 offset = 0;
    while (!isEqualString(bufferName, mAreaGroupInfo[offset].name)) {
        if (++offset >= mNumBuffers)
            return 0;
    }
    return mAreaGroupInfo[offset].size;

lib/al/src/Library/Area/AreaObjGroup.cpp line 10 at r2 (raw file):

void AreaObjGroup::createBuffer() {
    if (mCapacity < 1) {

Is this required, or does it auto-generate?

lib/al/src/Library/Area/AreaObjGroup.cpp line 40 at r2 (raw file):

void AreaObjGroup::incrementCount() {
    mCapacity += 1;

Uhh, are you sure about this?

src/AreaObj/BirdGatheringSpotArea.h line 23 at r2 (raw file):

    void init(const al::AreaInitInfo& initInfo) override;

    void calcRandomGroundTrans(sead::Vector3f trans) const;

sead::Vector3f*

src/AreaObj/ExtForceArea.h line 11 at r2 (raw file):

    void init(const al::AreaInitInfo& areaInitInfo) override;

    void calcExtForce(sead::Vector3f, sead::Vector3f, sead::Vector3f, sead::Vector3f);

Pointers for all of them

src/AreaObj/ForceRecoveryKidsArea.h line 3 at r2 (raw file):

#pragma once

#include <math/seadVectorFwd.h>

Should be normal seadVector.h, not Fwd

src/AreaObj/MoveArea2D.h line 3 at r2 (raw file):

#pragma once

#include <math/seadVectorFwd.h>

seadVector.h

src/AreaObj/RouteGuideArea.h line 3 at r2 (raw file):

#pragma once

#include <math/seadVectorFwd.h>

seadVector.h

src/Scene/ProjectAreaFactory.cpp line 3 at r2 (raw file):

#include "Scene/ProjectAreaFactory.h"

#include "AreaObj/BirdGatheringSpotArea.h"

See linter: Swap the blocks

src/Scene/ProjectAreaFactory.cpp line 169 at r2 (raw file):

static al::AreaGroupInfo sAreaGroupInfo[] = {{"GraphicsArea", 1}};

// not matching

decomp.me?

whaleymar · 2024-03-09T03:33:51Z

lib/al/src/Library/Area/AreaObjGroup.cpp line 40 at r2 (raw file):