System: Implement TempSaveData

[german77]

I'm attempting to find the correct implementation for StrTreeMap::allocBuffer this did not trigger the issue I was looking for.

TempSaveData seems to store a list obj bool values and stage objects. writeHashInWorld is not matching https://decomp.me/scratch/OCYs2

---

This change is 

---

Report for 1.0 (0de3c75 - c92df4d)

📈 Matched code: 15.40% (+0.04%, +4736 bytes)

✅ 23 new matches

Unit	Item	Bytes	Before	After
System/TempSaveData	TempSaveData::TempSaveData()	+696	0.00%	100.00%
System/TempSaveData	sead::StrTreeMap<32, bool>::insert(sead::SafeStringBase<char> const&, bool const&)	+636	0.00%	100.00%
System/TempSaveData	TempSaveData::writeInWorld(al::PlacementId const*, char const*)	+364	0.00%	100.00%
System/TempSaveData	TempSaveData::writeInWorldResetMiniGame(al::PlacementId const*, char const*)	+364	0.00%	100.00%
System/TempSaveData	TempSaveData::writeInScenario(al::PlacementId const*, char const*)	+364	0.00%	100.00%
System/TempSaveData	TempSaveData::writeHashInWorld(char const*, bool)	+348	0.00%	100.00%
System/TempSaveData	TempSaveData::findHashValueInWorld(char const*) const	+304	0.00%	100.00%
System/TempSaveData	deleteUniqObj(FixedHeapArray<UniqObjInfo, 64>, al::PlacementId const*, char const*)	+256	0.00%	100.00%
System/TempSaveData	TempSaveData::isOnInWorld(al::PlacementId const*, char const*) const	+256	0.00%	100.00%
System/TempSaveData	TempSaveData::isOnInWorldResetMiniGame(al::PlacementId const*, char const*) const	+256	0.00%	100.00%
System/TempSaveData	TempSaveData::isOnInScenario(al::PlacementId const*, char const*) const	+256	0.00%	100.00%
System/TempSaveData	TempSaveData::init()	+168	0.00%	100.00%
System/TempSaveData	void sead::TreeMapImpl<sead::SafeStringBase<char> >::forEach<sead::Delegate1<sead::StrTreeMap<32, bool>, sead::TreeMapNode<sead::SafeStringBase<char> >*> >(sead::TreeMapNode<sead::SafeStringBase<char> >*, sead::Delegate1<sead::StrTreeMap<32, bool>, sead::TreeMapNode<sead::SafeStringBase<char> >*> const&)	+116	0.00%	100.00%
System/TempSaveData	sead::Delegate1<sead::StrTreeMap<32, bool>, sead::TreeMapNode<sead::SafeStringBase<char> >*>::clone(sead::Heap*) const	+92	0.00%	100.00%
System/TempSaveData	TempSaveData::initForScenario()	+68	0.00%	100.00%
System/TempSaveData	TempSaveData::resetMiniGame()	+68	0.00%	100.00%
System/TempSaveData	sead::Delegate1<sead::StrTreeMap<32, bool>, sead::TreeMapNode<sead::SafeStringBase<char> >*>::invoke(sead::TreeMapNode<sead::SafeStringBase<char> >*)	+48	0.00%	100.00%
System/TempSaveData	sead::StrTreeMap<32, bool>::Node::erase_()	+32	0.00%	100.00%
System/TempSaveData	sead::StrTreeMap<32, bool>::eraseNodeForClear_(sead::TreeMapNode<sead::SafeStringBase<char> >*)	+16	0.00%	100.00%
System/TempSaveData	TempSaveData::setInfo(int, int)	+8	0.00%	100.00%
System/TempSaveData	TempSaveData::deleteInWorld(al::PlacementId const*, char const*)	+8	0.00%	100.00%
System/TempSaveData	TempSaveData::deleteInWorldResetMiniGame(al::PlacementId const*, char const*)	+8	0.00%	100.00%
System/TempSaveData	sead::StrTreeMap<32, bool>::Node::~Node()	+4	0.00%	100.00%

[MonsterDruide1]

@MonsterDruide1 reviewed 4 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on german77).

---

src/System/TempSaveData.h line 19 at r1 (raw file):

    void initForScenario();
    void resetMiniGame();
    void setInfo(s32 worldIndex, s32 unknown);

Suggestion:

void setInfo(s32 worldIndex, s32 scenarioIndex);

---

src/System/TempSaveData.h line 34 at r1 (raw file):

private:
    UniqObjInfo* mWordObjects = nullptr;

Suggestion:

UniqObjInfo* mWorldObjects = nullptr;

---

src/System/TempSaveData.h line 36 at r1 (raw file):

    UniqObjInfo* mWordObjects = nullptr;
    UniqObjInfo* mMiniGameObjects = nullptr;
    UniqObjInfo* mScenarioObjects = nullptr;

Might be turned into FixedHeapArray, moving its declaration from GameDataFile into some common header? Removes all those magical 64s.

Code quote:

    UniqObjInfo* mWordObjects = nullptr;
    UniqObjInfo* mMiniGameObjects = nullptr;
    UniqObjInfo* mScenarioObjects = nullptr;

---

src/System/TempSaveData.h line 38 at r1 (raw file):

    UniqObjInfo* mScenarioObjects = nullptr;
    s32 mWorldIndex = -1;
    s32 _1c = -1;

same arg as for GameDataFile::startStage in GameDataHolder::startStage

Suggestion:

s32 mScenarioIndex = -1;

---

src/System/TempSaveData.cpp line 19 at r1 (raw file):

        mMiniGameObjects[i].clear();
        mScenarioObjects[i].clear();
        // BUG: Clearing multiple times the hashed values

This does not (potentially) result in faulty behaviour - clearing a map multiple times should be handled just fine (I hope!), so no further weird behaviour could emerge from this.

Suggestion:

// NOTE: Clearing multiple times the hashed values

---

src/System/TempSaveData.cpp line 51 at r1 (raw file):

}

void deleteUniqObj(UniqObjInfo* objInfo, const al::PlacementId* placementId,

Suggestion:

static ALWAYS_INLINE s32 getUniqObjId(const UniqObjInfo* objInfo, const al::PlacementId* placementId,
                        const char* stageName) {
    al::StringTmp<128> str;
    placementId->makeString(&str);

    s32 id = -1;
    for (s32 i = 0; i < 64; i++) {
        if (!objInfo[i].getStageName().isEmpty() &&
            al::isEqualString(objInfo[i].getStageName().cstr(), stageName) &&
            al::isEqualString(objInfo[i].getObjId(), str)) {
            id = i;
            break;
        }
    }
    return id;
}

static ALWAYS_INLINE void deleteUniqObj(UniqObjInfo* objInfo, const al::PlacementId* placementId,

---

src/System/TempSaveData.cpp line 78 at r1 (raw file):

            break;
        }
    }

This block seems to be shared among three functions. Can it be made a separate static function to deduplicate?

Code quote:

    s32 id = getUniqObjId(objInfo, placementId, stageName);
    if (id != -1)
        return;

    for (s32 i = 0; i < 64; i++) {
        if (objInfo[i].getStageName()->isEmpty()) {
            placementId->makeString(objInfo[i].getObjId());
            objInfo[i].getStageName()->format("%s", stageName);
            break;
        }
    }

---

src/System/TempSaveData.cpp line 134 at r1 (raw file):

}

// NON_MATCHING: Wrong loading order https://decomp.me/scratch/OCYs2

Issue is that it your current code loads mWorldValues from the offset within TempSaveData twice, while the original only does it once.

However, I can't find a match with that information either. Best I can do is add this to StrTreeMap:

    void put(const SafeString& key, const Value& value) {
        auto* node = find(key);
        if (node) {
            node->value() = value;
            return;
        }
    
        insert(key, value);
    }

... which results in a mismatch because the SafeString constructor is only called once at the very top, while it should be called twice (again for insert). Changing the type to const char* requires an inline to keep the function inline, but then results in the same mismatch as observed right now.

---

src/System/UniqObjInfo.h line 9 at r1 (raw file):

}

class UniqObjInfo {

Given the Info name and full required transparency for member variables, maybe this is just a struct?

[german77]

@german77 made 9 comments.
Reviewable status: 1 of 8 files reviewed, 9 unresolved discussions (waiting on MonsterDruide1).

---

src/System/TempSaveData.h line 36 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Might be turned into FixedHeapArray, moving its declaration from GameDataFile into some common header? Removes all those magical 64s.

Done.

---

src/System/TempSaveData.h line 38 at r1 (raw file):

Previously, MonsterDruide1 wrote…

same arg as for GameDataFile::startStage in GameDataHolder::startStage

Done.

---

src/System/TempSaveData.cpp line 19 at r1 (raw file):

Previously, MonsterDruide1 wrote…

This does not (potentially) result in faulty behaviour - clearing a map multiple times should be handled just fine (I hope!), so no further weird behaviour could emerge from this.

Done. It might have a performance hit

---

src/System/TempSaveData.cpp line 78 at r1 (raw file):

Previously, MonsterDruide1 wrote…

This block seems to be shared among three functions. Can it be made a separate static function to deduplicate?

Done.

---

src/System/TempSaveData.cpp line 134 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Issue is that it your current code loads mWorldValues from the offset within TempSaveData twice, while the original only does it once.

However, I can't find a match with that information either. Best I can do is add this to StrTreeMap:

    void put(const SafeString& key, const Value& value) {
        auto* node = find(key);
        if (node) {
            node->value() = value;
            return;
        }
    
        insert(key, value);
    }

... which results in a mismatch because the SafeString constructor is only called once at the very top, while it should be called twice (again for insert). Changing the type to const char* requires an inline to keep the function inline, but then results in the same mismatch as observed right now.

I will update this function once is pushed into sead. I will keep trying for a few days to find a match.

---

src/System/UniqObjInfo.h line 9 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Given the Info name and full required transparency for member variables, maybe this is just a struct?

Done. Agreed

---

src/System/TempSaveData.h line 19 at r1 (raw file):

    void initForScenario();
    void resetMiniGame();
    void setInfo(s32 worldIndex, s32 unknown);

Done.

---

src/System/TempSaveData.h line 34 at r1 (raw file):

private:
    UniqObjInfo* mWordObjects = nullptr;

Done.

---

src/System/TempSaveData.cpp line 51 at r1 (raw file):

}

void deleteUniqObj(UniqObjInfo* objInfo, const al::PlacementId* placementId,

Done deleteUniqObj has a symbol and is not inlined

[german77]

Depends on open-ead/sead#274

[MonsterDruide1]

@MonsterDruide1 reviewed 9 files and all commit messages, made 7 comments, and resolved 8 discussions.
Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on german77).

---

src/System/TempSaveData.cpp line 51 at r1 (raw file):

Previously, german77 (Narr the Reg) wrote…

Done deleteUniqObj has a symbol and is not inlined

Cannot confirm, I don't see this function anywhere, neither with nor without symbol?

---

lib/sead at r2 (raw file):
blocked by open-ead/sead#274

---

src/System/TempSaveData.cpp at r2 (raw file):
TODO: skim entire file again

---

src/System/TempSaveData.cpp line 36 at r2 (raw file):

static ALWAYS_INLINE s32 getUniqObjId(const UniqObjInfo* objInfo,
                                      const al::PlacementId* placementId, const char* stageName) {

Might need template <...> for FixedHeapArray again

then remove all ::maxObjEntries usages

Suggestion:

static ALWAYS_INLINE s32 getUniqObjId(const FixedHeapArray& objInfo,
                                      const al::PlacementId* placementId, const char* stageName) {

---

src/System/TempSaveData.cpp line 52 at r2 (raw file):

}

static ALWAYS_INLINE void writeUniqObj(UniqObjInfo* objInfo, const al::PlacementId* placementId,

same here

---

src/System/TempSaveData.cpp line 67 at r2 (raw file):

}

void deleteUniqObj(UniqObjInfo* objInfo, const al::PlacementId* placementId,

same here

---

src/System/TempSaveData.h line 35 at r2 (raw file):

    s32 getWorldIndex() const { return mWorldIndex; }

    static const s32 maxObjEntries = 64;

Mark it not only as constant (= rodata), but "definitely known at compile time" - not sure if it has an effect on codegen, I could imagine const still putting this into .rodata.

Suggestion:

static constexpr s32 maxObjEntries = 64;

[german77]

@german77 made 1 comment.
Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on MonsterDruide1).

---

src/System/TempSaveData.cpp line 51 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Cannot confirm, I don't see this function anywhere, neither with nor without symbol?

does 0x710053d55c is something you can see?

[german77]

@german77 made 4 comments.
Reviewable status: 9 of 10 files reviewed, 7 unresolved discussions (waiting on MonsterDruide1).

---

src/System/TempSaveData.h line 35 at r2 (raw file):

Previously, MonsterDruide1 wrote…

Mark it not only as constant (= rodata), but "definitely known at compile time" - not sure if it has an effect on codegen, I could imagine const still putting this into .rodata.

Done.

---

src/System/TempSaveData.cpp line 36 at r2 (raw file):

Previously, MonsterDruide1 wrote…

Might need template <...> for FixedHeapArray again

then remove all ::maxObjEntries usages

I already tried it messes up quite a bit of stuff for all functions https://decomp.me/scratch/G6BuN I believe FixedHeapArray is not providing any benefit to this class.

---

src/System/TempSaveData.cpp line 52 at r2 (raw file):

Previously, MonsterDruide1 wrote…

same here

Ditto

---

src/System/TempSaveData.cpp line 67 at r2 (raw file):

Previously, MonsterDruide1 wrote…

same here

Ditto

[MonsterDruide1]

@MonsterDruide1 reviewed 2 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 9 of 10 files reviewed, 5 unresolved discussions (waiting on german77).

---

src/System/TempSaveData.cpp line 51 at r1 (raw file):

Previously, german77 (Narr the Reg) wrote…

does 0x710053d55c is something you can see?

Ah, function sorting broke for me for some reason. Agreed, that function exists.

---

src/System/TempSaveData.cpp line 36 at r2 (raw file):

Previously, german77 (Narr the Reg) wrote…

I already tried it messes up quite a bit of stuff for all functions https://decomp.me/scratch/G6BuN I believe FixedHeapArray is not providing any benefit to this class.

Fixed by removing & and just having it be an object of type FixedHeapArray.
Only looks slightly weird, because this would usually result in a copy of the object - but kind-of makes sense that a Heap array doesn't copy its entire contents, but just results in a shallow copy pointing to the same data, so ... I think it's still better than raw pointers.

[german77]

@german77 made 3 comments.
Reviewable status: 7 of 10 files reviewed, 5 unresolved discussions (waiting on MonsterDruide1).

---

src/System/TempSaveData.cpp line 36 at r2 (raw file):

Previously, MonsterDruide1 wrote…

Fixed by removing & and just having it be an object of type FixedHeapArray.
Only looks slightly weird, because this would usually result in a copy of the object - but kind-of makes sense that a Heap array doesn't copy its entire contents, but just results in a shallow copy pointing to the same data, so ... I think it's still better than raw pointers.

Done.

---

src/System/TempSaveData.cpp line 52 at r2 (raw file):

Previously, german77 (Narr the Reg) wrote…

Ditto

Done.

---

src/System/TempSaveData.cpp line 67 at r2 (raw file):

Previously, german77 (Narr the Reg) wrote…

Ditto

Done.

[MonsterDruide1]

@MonsterDruide1 reviewed 1 file and all commit messages, and resolved 3 discussions.
Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions.

[MonsterDruide1]

@MonsterDruide1 made 1 comment and resolved 1 discussion.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on german77).

---

lib/sead at r2 (raw file):

Previously, MonsterDruide1 wrote…

blocked by open-ead/sead#274

has been merged, this PR can be turned towards the finish line now.
Requires rebase, and this file should not be modified.

[german77]

@german77 made 1 comment.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).

---

lib/sead at r2 (raw file):

Previously, MonsterDruide1 wrote…

has been merged, this PR can be turned towards the finish line now.
Requires rebase, and this file should not be modified.

Done.

[MonsterDruide1]

@MonsterDruide1 reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on german77).