ksys/act: Add LinkTag

[MonsterDruide1]

Base research point: LinkTag::construct method with an inlined ctor at 0x7100D3778C. All functions are already labeled in the CSV before this PR, so they should already be named correctly in the database even without these names and implementations.

---

This change is 

[leoetlino]

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

---

src/KingSystem/ActorSystem/actLinkTag.h, line 16 at r1 (raw file):

class LinkTag : public BaseProc {
public:
    enum Type : u8 { And, Or, NAnd, NOr, XOr, Count, Pulse, None };

enum class

---

src/KingSystem/ActorSystem/actLinkTag.h, line 18 at r1 (raw file):

    enum Type : u8 { And, Or, NAnd, NOr, XOr, Count, Pulse, None };

    SEAD_RTTI_OVERRIDE(LinkTag, BaseProc)

Please put this between the class { and public:

---

src/KingSystem/ActorSystem/actLinkTag.h, line 22 at r1 (raw file):

    static LinkTag* construct(const BaseProc::CreateArg& arg, sead::Heap* heap);

    LinkTag(const BaseProc::CreateArg& arg);

This should be marked explicit. explicit LinkTag(...);

---

src/KingSystem/ActorSystem/actLinkTag.h, line 26 at r1 (raw file):

    BaseProcJobHandlerT<LinkTag> mJob;
    unsigned int triggeredLinkFlags[3] = {0,0,0};

u32 mTriggeredLinkFlags[3]{}; + naming convention for members below

---

src/KingSystem/ActorSystem/actLinkTag.h, line 27 at r1 (raw file):

    BaseProcJobHandlerT<LinkTag> mJob;
    unsigned int triggeredLinkFlags[3] = {0,0,0};
    LinkTag::Type linkTagType = And;

just "Type" should be enough

---

src/KingSystem/ActorSystem/actLinkTag.h, line 28 at r1 (raw file):

    unsigned int triggeredLinkFlags[3] = {0,0,0};
    LinkTag::Type linkTagType = And;
    char field_1DD = 0xFF;

u8 _1dd

---

src/KingSystem/ActorSystem/actLinkTag.h, line 29 at r1 (raw file):

    LinkTag::Type linkTagType = And;
    char field_1DD = 0xFF;
    char field_1DE = 0;

u8 _1de and so on

---

src/KingSystem/ActorSystem/actLinkTag.h, line 31 at r1 (raw file):

    char field_1DE = 0;
    char field_1DF = 0xFF;
    unsigned short flags = 0;

This was probably a sead::TypedBitFlag<LinkTagFlag, u16> where LinkTagFlag is enum class LinkTagFlag { ... }. Values TBD. (The reason you should avoid calling this just Flag is because BaseProc already has an enum with a similar name.)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 32 at r1 (raw file):

    char field_1DF = 0xFF;
    unsigned short flags = 0;
    char tagCount = 0;

char is almost always incorrect if the variable does not in fact store a character. This should either be u8 or s8 (depending on the usage in other functions). If you don't have further details prefer u8 over s8.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 36 at r1 (raw file):

    u32 counter = 0;
    int field_1E8 = 0;
    unsigned int mCalcFrameFlags = 0;

It looks like this should be an atomic variable:

sead::TypedBitFlag<JobFlag, sead::Atomic<u32>> mJobFlags;

where JobFlag is yet another enum class (values TBD).

---

src/KingSystem/ActorSystem/actLinkTag.h, line 37 at r1 (raw file):

    int field_1E8 = 0;
    unsigned int mCalcFrameFlags = 0;
    int hashId = 0;

Since this is a hash ID, this should probably be a u32.

[leoetlino]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 26 at r1 (raw file):

    BaseProcJobHandlerT<LinkTag> mJob;
    unsigned int triggeredLinkFlags[3] = {0,0,0};

This appears to be a sead::LongBitFlag<96> (32 bits times 3).

[MonsterDruide1]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 16 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

enum class

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 18 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Please put this between the class { and public:

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 22 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This should be marked explicit. explicit LinkTag(...);

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 26 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

u32 mTriggeredLinkFlags[3]{}; + naming convention for members below

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 26 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This appears to be a sead::LongBitFlag<96> (32 bits times 3).

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 27 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

just "Type" should be enough

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 28 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

u8 _1dd

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 29 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

u8 _1de and so on

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 32 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

char is almost always incorrect if the variable does not in fact store a character. This should either be u8 or s8 (depending on the usage in other functions). If you don't have further details prefer u8 over s8.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 37 at r1 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Since this is a hash ID, this should probably be a u32.

Done.

[leoetlino]

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

---

src/KingSystem/ActorSystem/actLinkTag.h, line 20 at r2 (raw file):

public:
    enum class Type : u8 { And, Or, NAnd, NOr, XOr, Count, Pulse, None };
    enum class JobFlag { JOB_PLACEHOLDER };

FYI there's no need to put "placeholder" in the enum class; enum class Foo {}; is valid C++.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 34 at r2 (raw file):

    u8 _1de = 0;
    u8 _1df = 0xFF;
    sead::TypedBitFlag<LinkTagFlag, u16> mFlags = LinkTagFlag::FLAG_PLACEHOLDER;

<prim/seadTypedBitFlag.h> needs to be included. Also you don't need the = PLACEHOLDER part; TypedBitFlags are initialised to 0 by default.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 35 at r2 (raw file):

    u8 _1df = 0xFF;
    sead::TypedBitFlag<LinkTagFlag, u16> mFlags = LinkTagFlag::FLAG_PLACEHOLDER;
    // u16 mFlags = 0;

should be removed.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 39 at r2 (raw file):

    u8 field_1E3 = 0;
    u32 mCounter = 0;
    // int field_1E8 = 0;

This should be uncommented.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 40 at r2 (raw file):

    u32 mCounter = 0;
    // int field_1E8 = 0;
    sead::TypedBitFlag<JobFlag, sead::Atomic<u32>> mJobFlags = JobFlag::JOB_PLACEHOLDER;

<thread/seadAtomic.h> needs to be included.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 41 at r2 (raw file):

    // int field_1E8 = 0;
    sead::TypedBitFlag<JobFlag, sead::Atomic<u32>> mJobFlags = JobFlag::JOB_PLACEHOLDER;
    u32 mCalcFrameFlags = 0;

This should be removed.

[MonsterDruide1]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @leoetlino)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 34 at r2 (raw file):

Previously, leoetlino (Léo Lam) wrote…

<prim/seadTypedBitFlag.h> needs to be included. Also you don't need the = PLACEHOLDER part; TypedBitFlags are initialised to 0 by default.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 35 at r2 (raw file):

Previously, leoetlino (Léo Lam) wrote…

should be removed.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 39 at r2 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This should be uncommented.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 40 at r2 (raw file):

Previously, leoetlino (Léo Lam) wrote…

<thread/seadAtomic.h> needs to be included.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 41 at r2 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This should be removed.

No, it shouldn't... mCalcFrameFlags was the wrong one, this one is fine.

[MonsterDruide1]

Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @leoetlino)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 41 at r2 (raw file):

Previously, MonsterDruide1 wrote…

No, it shouldn't... mCalcFrameFlags was the wrong one, this one is fine.

Nevermind, thought this comment was on mHashId. Done.

[leoetlino]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 24 at r3 (raw file):

    enum class JobFlag {};
    enum class LinkTagFlag {
        FLAG_PLACEHOLDER_4 = 0x4,

Just _4 is fine, this is kind of verbose.

[leoetlino]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 57 at r3 (raw file):

    sead::TypedBitFlag<JobFlag, sead::Atomic<u32>> mJobFlags;
    u32 mHashId = 0;
    int field_1F4 = 0;

Apparently this doesn't actually exist. Or at least it is never initialised in the ctor.

[MonsterDruide1]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @leoetlino)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 24 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Just _4 is fine, this is kind of verbose.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 57 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Apparently this doesn't actually exist. Or at least it is never initialised in the ctor.

It does however exist in the IDA database... and probably would need to be gapped when removed. Or the mHashId could be u64 instead?

[leoetlino]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 57 at r3 (raw file):

Previously, MonsterDruide1 wrote…

It does however exist in the IDA database... and probably would need to be gapped when removed. Or the mHashId could be u64 instead?

The IDA database can be incorrect :)

No, a hash ID is always 32-bit. I think the 4 bytes at offset 0x1f4 are probably just padding, considering map::MubinIter has an 8-byte alignment.

[MonsterDruide1]

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @leoetlino)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 57 at r3 (raw file):

Previously, leoetlino (Léo Lam) wrote…

The IDA database can be incorrect :)

No, a hash ID is always 32-bit. I think the 4 bytes at offset 0x1f4 are probably just padding, considering map::MubinIter has an 8-byte alignment.

Done. Apparantly is only the padding for alignment, as nothing gets a diffferent offset when removed.

[leoetlino]

Reviewed 1 of 2 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MonsterDruide1)

[leoetlino]

Reviewed 2 of 3 files at r12, 3 of 4 files at r13, 6 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @MonsterDruide1)

---

src/KingSystem/ActorSystem/actActor.h, line 94 at r14 (raw file):

    static constexpr size_t getCreatorListNodeOffset() { return offsetof(Actor, mCreatorListNode); }

//protected:

Please don't. Things will get extremely messy if all the internals are exposed.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 485 at r14 (raw file):

bool LinkTag::isTriggered(map::ObjectLink* link, u8 linkIdx) {
    auto* otherObj = link->other_obj;
    if (!otherObj)

Not sure they used an early return here since the return value is a bit long to type.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 490 at r14 (raw file):

    Actor* actor = otherObj->getActor();
    if (actor && (u8(actor->getState()) | 2) != 3) {  // some weirdness with " | 2"
        if (!actor->checkDerivedRuntimeTypeInfo(LinkTag::getRuntimeTypeInfoStatic())) {

This looks like sead::IsDerived.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 491 at r14 (raw file):

    if (actor && (u8(actor->getState()) | 2) != 3) {  // some weirdness with " | 2"
        if (!actor->checkDerivedRuntimeTypeInfo(LinkTag::getRuntimeTypeInfoStatic())) {
            if (!actor->checkDerivedRuntimeTypeInfo(Actor::getRuntimeTypeInfoStatic())) {

another sead::IsDerived

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 498 at r14 (raw file):

            else
                setBitAfterCheck(&mTriggeredLinkFlags, linkIdx);
        } else if ((actor->TEMP_0x17c[0x38] & 0x40) != 0) {

This won't fly. Please add a member correctly. (For maintainability reasons using TEMP members is a big no-no and bad member accesses might affect codegen considering how LLVM works.)

---

src/KingSystem/System/MCMgr.h, line 17 at r14 (raw file):

private:
    void* actorChemicalStuff;

member names

---

src/KingSystem/System/MCMgr.h, line 31 at r14 (raw file):

    void* _0; // TODO: Add gsys::ISystemTaskCallback, as it is a base class of this one
    sead::FixedSizeJQ queue0;

member names

[MonsterDruide1]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 58 at r11 (raw file):

Previously, leoetlino (Léo Lam) wrote…

override?

'prepareForPreDelete' marked 'override' but does not override any member functions. Is this class missing a base class?

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 485 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Not sure they used an early return here since the return value is a bit long to type.

How else would this statement appear in HexRays' output? Did they use a giant if-brace?

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 490 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This looks like sead::IsDerived.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 491 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

another sead::IsDerived

Done.

---

src/KingSystem/System/MCMgr.h, line 17 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

member names

Done.

---

src/KingSystem/System/MCMgr.h, line 31 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

member names

Done.

[leoetlino]

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

---

src/KingSystem/ActorSystem/actLinkTag.h, line 58 at r11 (raw file):

Previously, MonsterDruide1 wrote…

'prepareForPreDelete' marked 'override' but does not override any member functions. Is this class missing a base class?

No, the compile error is probably because you're just overriding the wrong function. The base function is called prepareForPreDelete_.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 485 at r14 (raw file):

Previously, MonsterDruide1 wrote…

How else would this statement appear in HexRays' output? Did they use a giant if-brace?

Possibly. This can affect codegen so I'd try to invert the condition.

[MonsterDruide1]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

src/KingSystem/ActorSystem/actActor.h, line 94 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Please don't. Things will get extremely messy if all the internals are exposed.

Done.

---

src/KingSystem/ActorSystem/actLinkTag.h, line 58 at r11 (raw file):

Previously, leoetlino (Léo Lam) wrote…

No, the compile error is probably because you're just overriding the wrong function. The base function is called prepareForPreDelete_.

enum "ksys::act::BaseProc::PreDeletePrepareResult" is inaccessible. I can't return the correct value type, as the enum is marked as protected...

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 498 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

This won't fly. Please add a member correctly. (For maintainability reasons using TEMP members is a big no-no and bad member accesses might affect codegen considering how LLVM works.)

Replaced by LiveActor, so resolved.

[leoetlino]

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

---

src/KingSystem/ActorSystem/actLinkTag.h, line 58 at r11 (raw file):

Previously, MonsterDruide1 wrote…

enum "ksys::act::BaseProc::PreDeletePrepareResult" is inaccessible. I can't return the correct value type, as the enum is marked as protected...

That doesn't make sense considering LinkTag is derived from BaseProc... Please post your source code.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 498 at r14 (raw file):

Previously, MonsterDruide1 wrote…

Replaced by LiveActor, so resolved.

You mean LinkTag but ok.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 490 at r16 (raw file):

    BaseProc* baseProc = otherObj->getProc();
    if (baseProc && (u8(baseProc->getState()) | 2) != 3) {  // some weirdness with " | 2"

(state | 2) == 3 is true if and only if state is 1 or 3.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 501 at r16 (raw file):

                setBitAfterCheck(&mTriggeredLinkFlags, linkIdx);
        } else if (linkTag->mFlags.isOn(LinkTagFlag::_40)) {
            if (linkTag->mFlags.isOn(LinkTagFlag::_10) ==

1 << 5 isn't 0x10, it's 0x20.

[MonsterDruide1]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

---

src/KingSystem/ActorSystem/actLinkTag.h, line 58 at r11 (raw file):

Previously, leoetlino (Léo Lam) wrote…

That doesn't make sense considering LinkTag is derived from BaseProc... Please post your source code.

Nevermind, reconfiguring the project fixed it... Done.

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 498 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

You mean LinkTag but ok.

Yeah... working on two decomp projects with similar named classes is hard! 🙂

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 501 at r16 (raw file):

Previously, leoetlino (Léo Lam) wrote…

1 << 5 isn't 0x10, it's 0x20.

Done.

[MonsterDruide1]

Reviewable status: 8 of 11 files reviewed, 4 unresolved discussions (waiting on @leoetlino)

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 485 at r14 (raw file):

Previously, leoetlino (Léo Lam) wrote…

Possibly. This can affect codegen so I'd try to invert the condition.

Tried that, but the codegen remained unchanged...

---

src/KingSystem/ActorSystem/actLinkTag.cpp, line 490 at r16 (raw file):

Previously, leoetlino (Léo Lam) wrote…

(state | 2) == 3 is true if and only if state is 1 or 3.

Done. Improved the diff by quite a bit!