System: Implement GameDataFunction#651
Conversation
MonsterDruide1
left a comment
There was a problem hiding this comment.
Reviewed 18 of 38 files at r1, all commit messages.
Reviewable status: 18 of 38 files reviewed, 4 unresolved discussions
a discussion (no related file):
(review still in progress)
lib/agl line 0 at r1 (raw file):
(once merged and pulled in via Renovate, undo this)
src/Sequence/ChangeStageInfo.h line 39 at r1 (raw file):
bool isEmpty() const { return mChangeStageName.isEmpty(); } const char* getStartId() const { return mChangeStageId.cstr(); }
Suggestion:
const char* getStageId() const { return mChangeStageId.cstr(); }src/Scene/QuestInfo.h line 36 at r1 (raw file):
sead::Vector3f mTrans = sead::Vector3f::zero; bool mIsMainQuest = false; bool _0x19 = false;
Suggestion:
bool _19 = false;389c215 to
8bb2886
Compare
0f-0b
left a comment
There was a problem hiding this comment.
Reviewable status: 15 of 38 files reviewed, 4 unresolved discussions (waiting on @MonsterDruide1)
src/Scene/QuestInfo.h line 36 at r1 (raw file):
sead::Vector3f mTrans = sead::Vector3f::zero; bool mIsMainQuest = false; bool _0x19 = false;
Done. Should I also rename all members like this in GameDataFile?
src/Sequence/ChangeStageInfo.h line 39 at r1 (raw file):
bool isEmpty() const { return mChangeStageName.isEmpty(); } const char* getStartId() const { return mChangeStageId.cstr(); }
I called this function getStartId because of how it's used in GameDataFile::changeNextStage, but maybe getLabel or getChangeStageId would be a better name? Anyway I don't think getStageId is right.
73d83c3 to
6d18a38
Compare
06c53ae to
54f596d
Compare
ShishuTheDragon
left a comment
There was a problem hiding this comment.
Reviewable status: 10 of 38 files reviewed, 6 unresolved discussions (waiting on @MonsterDruide1)
src/System/GameDataFile.h line 72 at r2 (raw file):
My understanding was that:
Struct member variables should be formatted as noPrefixCamelCase
I’m not sure why tools/check-format.py isn’t picking this up though
src/System/GameDataFile.h line 49 at r2 (raw file):
enum class CapStatus { None, Removed, GotBack }; enum RaceType {
What’s the reason for this being an enum and not an enum class? Is it because it’s being passed in to writer->changeNextStage which expects an int? If so, would it be better to change the raceType param to match this enum?
0f-0b
left a comment
There was a problem hiding this comment.
Reviewable status: 10 of 38 files reviewed, 6 unresolved discussions (waiting on @MonsterDruide1 and @ShishuTheDragon)
src/System/GameDataFile.h line 49 at r2 (raw file):
Is it because it’s being passed in to
writer->changeNextStagewhich expects an int?
Yes.
Would it be better to change the raceType param to match this enum?
The symbol for writer->changeNextStage is _ZN14GameDataHolder15changeNextStageEPK15ChangeStageInfoi, from which it can be known that the type of the last parameter is definitely int.
src/System/GameDataFile.h line 72 at r2 (raw file):
Previously, ShishuTheDragon (Shishu the Dragon) wrote…
My understanding was that:
Struct member variables should be formatted as noPrefixCamelCase
I’m not sure why
tools/check-format.pyisn’t picking this up though
OK, I see there was a poll. Will push an update later.
And apparently tools/check-format.py only looks for mPrefixedCamelCase and not snake_case.
|
@ShishuTheDragon Also, since you are working on |
54f596d to
a572bef
Compare
a572bef to
acaa851
Compare
acaa851 to
3a456f5
Compare
Report for 1.0 (0db3e9a - 15f1cc8)📈 Matched code: 9.55% (+0.18%, +22960 bytes) ✅ 462 new matches
...and 432 more new matches |
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 11 of 38 files at r1, 8 of 11 files at r2, all commit messages.
Reviewable status: 29 of 38 files reviewed, 12 unresolved discussions (waiting on @ShishuTheDragon)
src/Scene/QuestInfoHolder.h line 15 at r4 (raw file):
namespace rs { void getQuestInfoHolder(const al::IUseSceneObjHolder*);
Suggestion:
QuestInfoHolder* getQuestInfoHolder(const al::IUseSceneObjHolder*);src/Scene/QuestInfoHolder.h line 16 at r4 (raw file):
namespace rs { void getQuestInfoHolder(const al::IUseSceneObjHolder*); void tryCreateAndRegisterQuestInfoToHolder(const al::LiveActor*, const al::ActorInitInfo&);
Suggestion:
QuestInfo* tryCreateAndRegisterQuestInfoToHolder(const al::LiveActor*, const al::ActorInitInfo&);src/Scene/QuestInfoHolder.h line 18 at r4 (raw file):
void tryCreateAndRegisterQuestInfoToHolder(const al::LiveActor*, const al::ActorInitInfo&); void createAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*, const al::PlacementInfo&, bool);
Suggestion:
QuestInfo* createAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*, const al::PlacementInfo&,
bool);src/Scene/QuestInfoHolder.h line 21 at r4 (raw file):
void invalidateQuest(const QuestInfo*); void tryCreateAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*, const al::ActorInitInfo&);
Suggestion:
sead::PtrArray<QuestInfo>* tryCreateAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*,
const al::ActorInitInfo&);src/Scene/QuestInfoHolder.h line 23 at r4 (raw file):
const al::ActorInitInfo&); void validateQuest(const QuestInfo*); const QuestInfo* const* getActiveQuestList(const al::IUseSceneObjHolder*);
Double-const is rare. Are you sure that it isn't just this?
Suggestion:
const QuestInfo** getActiveQuestList(const al::IUseSceneObjHolder*);src/Scene/QuestInfoHolder.h line 27 at r4 (raw file):
s32 getActiveQuestNumForMap(const al::IUseSceneObjHolder*); bool isActiveQuestAllEqualNo(const al::IUseSceneObjHolder*); const char* getActiveQuestLabel(const al::IUseSceneObjHolder*);
Suggestion:
al::StringTmp<128> getActiveQuestLabel(const al::IUseSceneObjHolder*);src/Scene/QuestInfoHolder.h line 28 at r4 (raw file):
bool isActiveQuestAllEqualNo(const al::IUseSceneObjHolder*); const char* getActiveQuestLabel(const al::IUseSceneObjHolder*); const char* getActiveQuestStageName(const al::IUseSceneObjHolder*);
Suggestion:
al::StringTmp<128> getActiveQuestStageName(const al::IUseSceneObjHolder*);src/System/RankingCategory.h line 5 at r4 (raw file):
#include <prim/seadEnum.h> SEAD_ENUM(RankingCategory, JumpingRope, Volleyball)
Why do you think this is a SEAD_ENUM? Where did you get the values from?
0f-0b
left a comment
There was a problem hiding this comment.
Reviewable status: 28 of 38 files reviewed, 12 unresolved discussions (waiting on @MonsterDruide1 and @ShishuTheDragon)
src/System/RankingCategory.h line 5 at r4 (raw file):
Previously, MonsterDruide1 wrote…
Why do you think this is a
SEAD_ENUM? Where did you get the values from?
I initially made RankingCategory a regular enum, but then had trouble matching GameDataFile::setUpdateJumpingRopeScoreFlag and GameDataFile::setUpdateVolleyballScoreFlag, so I changed it to a SEAD_ENUM. The values are taken from these two functions as well.
src/Scene/QuestInfoHolder.h line 15 at r4 (raw file):
namespace rs { void getQuestInfoHolder(const al::IUseSceneObjHolder*);
Done.
src/Scene/QuestInfoHolder.h line 16 at r4 (raw file):
namespace rs { void getQuestInfoHolder(const al::IUseSceneObjHolder*); void tryCreateAndRegisterQuestInfoToHolder(const al::LiveActor*, const al::ActorInitInfo&);
Done.
src/Scene/QuestInfoHolder.h line 18 at r4 (raw file):
void tryCreateAndRegisterQuestInfoToHolder(const al::LiveActor*, const al::ActorInitInfo&); void createAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*, const al::PlacementInfo&, bool);
Done.
src/Scene/QuestInfoHolder.h line 21 at r4 (raw file):
void invalidateQuest(const QuestInfo*); void tryCreateAndRegisterQuestInfoToHolderFromLinkedObj(const al::LiveActor*, const al::ActorInitInfo&);
Done.
src/Scene/QuestInfoHolder.h line 27 at r4 (raw file):
s32 getActiveQuestNumForMap(const al::IUseSceneObjHolder*); bool isActiveQuestAllEqualNo(const al::IUseSceneObjHolder*); const char* getActiveQuestLabel(const al::IUseSceneObjHolder*);
Done.
src/Scene/QuestInfoHolder.h line 28 at r4 (raw file):
bool isActiveQuestAllEqualNo(const al::IUseSceneObjHolder*); const char* getActiveQuestLabel(const al::IUseSceneObjHolder*); const char* getActiveQuestStageName(const al::IUseSceneObjHolder*);
Done.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 2 of 7 files at r3, 2 of 2 files at r6, all commit messages.
Reviewable status: 31 of 38 files reviewed, 8 unresolved discussions (waiting on @0f-0b and @ShishuTheDragon)
src/System/CollectBgm.h line 20 at r6 (raw file):
const char* tag1; const char* tag2; const char* category;
Where did you get these names from?
name and situationName are somewhat clear based on the byml reading, but what about the others?
Code quote:
const char* name;
const char* situationName;
const char* tag1;
const char* tag2;
const char* category;src/System/CollectBgmBailoutInfo.h line 0 at r6 (raw file):
Where are these used at all?
0f-0b
left a comment
There was a problem hiding this comment.
Reviewable status: 31 of 38 files reviewed, 8 unresolved discussions (waiting on @MonsterDruide1 and @ShishuTheDragon)
src/System/CollectBgm.h line 20 at r6 (raw file):
Previously, MonsterDruide1 wrote…
Where did you get these names from?
nameandsituationNameare somewhat clear based on the byml reading, but what about the others?
These are all just guesses. tag1 and tag2 are used by EventFlowNodeCheckPlayingCollectBgm.
src/System/CollectBgmBailoutInfo.h line at r6 (raw file):
Previously, MonsterDruide1 wrote…
Where are these used at all?
In GameDataFile::trySetCollectedBgm. Since it's not part of this PR, should I delete this file for now?
6fde0a3 to
15f1cc8
Compare
15f1cc8 to
e14657e
Compare
ShishuTheDragon
left a comment
There was a problem hiding this comment.
@ShishuTheDragon reviewed 1 of 38 files at r1, 1 of 7 files at r3.
Reviewable status: 29 of 38 files reviewed, 6 unresolved discussions (waiting on @0f-0b and @MonsterDruide1)
e14657e to
392cfb4
Compare
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 2 of 4 files at r7, 1 of 5 files at r8, all commit messages.
Reviewable status: 32 of 38 files reviewed, 8 unresolved discussions (waiting on @0f-0b and @ShishuTheDragon)
a discussion (no related file):
Previously, MonsterDruide1 wrote…
(review still in progress)
blocked by #759
src/System/GameDataFunction.cpp line 250 at r8 (raw file):
} static s32 blackBox(s32 value) {
Suggestion:
// TODO: check what's happening here
static s32 blackBox(s32 value) {src/System/GameDataFunction.cpp line 265 at r8 (raw file):
} s32 getWorldIdForNewReleaseShop(GameDataHolderAccessor accessor, s32 index) {
TODO: read below here
0f-0b
left a comment
There was a problem hiding this comment.
@0f-0b made 2 comments.
Reviewable status: 41 of 43 files reviewed, 9 unresolved discussions (waiting on @MonsterDruide1 and @ShishuTheDragon).
src/System/CollectBgm.cpp line 10 at r13 (raw file):
Previously, MonsterDruide1 wrote…
No, please do it manually, we're trying to avoid stdlib as much as possible.
Why, even though std::size makes the code simpler? SessionMusicianNpc uses std::string and std::stringstream, so it's not that Nintendo themselves has a strict policy of avoiding the standard library whenever possible.
src/System/GameDataFile.h line 136 at r13 (raw file):
Previously, MonsterDruide1 wrote…
But
AchievementStatusis fine?
Is it also UB ifenum class HintStatus : s32specifies its base type?
It's still UB with a fixed underlying type. status is never directly accessed through a pointer (at least in GameDataFile), so it's fine as either s32 or AchievementStatus.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 file, made 7 comments, and resolved 6 discussions.
Reviewable status: 42 of 43 files reviewed, 4 unresolved discussions (waiting on @0f-0b and @ShishuTheDragon).
src/Scene/QuestInfoHolder.h line 23 at r4 (raw file):
Previously, MonsterDruide1 wrote…
Double-
constis rare. Are you sure that it isn't just this?
bump ^
src/Sequence/ChangeStageInfo.h line 39 at r1 (raw file):
Previously, 0f-0b (ud2) wrote…
I called this function
getStartIdbecause of how it's used inGameDataFile::changeNextStage, but maybegetLabelorgetChangeStageIdwould be a better name? Anyway I don't thinkgetStageIdis right.
Given that the member variable is labeled mChangeStageId, getStartId sounds too different - so I think getChangeStageId would be good here.
src/System/CollectBgm.cpp line 10 at r13 (raw file):
Previously, 0f-0b (ud2) wrote…
Why, even though
std::sizemakes the code simpler?SessionMusicianNpcusesstd::stringandstd::stringstream, so it's not that Nintendo themselves has a strict policy of avoiding the standard library whenever possible.
Okay, that changes things. I was assuming so far that we should follow a strict policy and only use it for special things like <limits>, which additionally have the benefit of being compile-time only and leaving no trace of std:: being used in the binary. However, there are quite a few std:: functions in use across the binary, you are right... Especially as this one should also only be compile-time only and is simplified into just a number by the compiler, I guess we're fine here.
src/System/GameDataFile.h line 136 at r13 (raw file):
Previously, 0f-0b (ud2) wrote…
It's still UB with a fixed underlying type.
statusis never directly accessed through a pointer (at least inGameDataFile), so it's fine as eithers32orAchievementStatus.
Alright, thanks for the demonstration - and I finally understood what that flag is doing now.
src/System/GameDataFunction.h line 40 at r10 (raw file):
Previously, 0f-0b (ud2) wrote…
In
wearCostumethe return values of both functions are passed tors::trySavePrepoChangeClothEventwhich takess64s.
Confirmed, thanks for checking.
src/System/RankingCategory.h line 5 at r4 (raw file):
Previously, 0f-0b (ud2) wrote…
I initially made
RankingCategorya regular enum, but then had trouble matchingGameDataFile::setUpdateJumpingRopeScoreFlagandGameDataFile::setUpdateVolleyballScoreFlag, so I changed it to aSEAD_ENUM. The values are taken from these two functions as well.
Further confirmed by #906, so we're good here.
a discussion (no related file):
Needs a rebase.
f9b5f32 to
ac18c05
Compare
0f-0b
left a comment
There was a problem hiding this comment.
@0f-0b made 4 comments.
Reviewable status: 36 of 43 files reviewed, 4 unresolved discussions (waiting on @MonsterDruide1 and @ShishuTheDragon).
a discussion (no related file):
Previously, MonsterDruide1 wrote…
Needs a rebase.
Done.
src/Scene/QuestInfoHolder.h line 23 at r4 (raw file):
Previously, MonsterDruide1 wrote…
bump ^
From my reading of the code, this “active quest list” is a cache to avoid having to recompute the current objective multiple times on every frame (MapMini::calcNearHintTrans uses it, for example), and is not supposed to be written into directly.
src/Sequence/ChangeStageInfo.h line 39 at r1 (raw file):
Previously, MonsterDruide1 wrote…
Given that the member variable is labeled
mChangeStageId,getStartIdsounds too different - so I thinkgetChangeStageIdwould be good here.
Done.
src/System/GameDataFile.h line 722 at r13 (raw file):
Previously, MonsterDruide1 wrote…
Very similar structs, just swapping the order of items - I can imagine that Nintendo did this, but please confirm.
These are constructed at around address 0x517998 in GameDataFile's constructor. I don't know where the index fields are used.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 7 files and all commit messages, made 3 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @0f-0b).
src/Scene/QuestInfoHolder.h line 23 at r4 (raw file):
Previously, 0f-0b (ud2) wrote…
From my reading of the code, this “active quest list” is a cache to avoid having to recompute the current objective multiple times on every frame (
MapMini::calcNearHintTransuses it, for example), and is not supposed to be written into directly.
I mean, it has a few more usages, but none of them write to it, so it may well be const at that level as well, I just don't like the way that const is placed - but that's a problem with C++, not our code.
src/System/GameDataFile.h line 722 at r13 (raw file):
Previously, 0f-0b (ud2) wrote…
These are constructed at around address
0x517998inGameDataFile's constructor. I don't know where theindexfields are used.
I don't see a difference at 517998 between these two types, and would even argue that the current implementation is not correct. It allocates 0x18 for these structs, and initializes all 18 bytes with writing XZR to them - which would not happen for padding bytes, those should not be written to. So I believe there must be something after the index field on both types.
a discussion (no related file):
blocked by #906
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on 0f-0b).
a discussion (no related file):
Previously, MonsterDruide1 wrote…
blocked by #906
Merged, ready to rebase.
ac18c05 to
9f686d3
Compare
0f-0b
left a comment
There was a problem hiding this comment.
@0f-0b made 1 comment.
Reviewable status: 32 of 43 files reviewed, 2 unresolved discussions (waiting on MonsterDruide1).
a discussion (no related file):
Previously, MonsterDruide1 wrote…
Merged, ready to rebase.
Done.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 11 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on 0f-0b).
src/System/GameDataFile.h line 722 at r13 (raw file):
Previously, MonsterDruide1 wrote…
I don't see a difference at
517998between these two types, and would even argue that the current implementation is not correct. It allocates0x18for these structs, and initializes all 18 bytes with writingXZRto them - which would not happen for padding bytes, those should not be written to. So I believe there must be something after theindexfield on both types.
comments/information on this? ^
0f-0b
left a comment
There was a problem hiding this comment.
@0f-0b made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on MonsterDruide1).
src/System/GameDataFile.h line 722 at r13 (raw file):
Previously, MonsterDruide1 wrote…
comments/information on this? ^
There is not necessarily anything after index: value-initializing a class type (as in new T()) without a non-default constructor fills padding bytes with zero, which I think is more likely what's happening here. I can't say for sure because I don't have a matching implementation of GameDataFile's constructor yet.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on 0f-0b).
src/System/GameDataFile.h line 722 at r13 (raw file):
Previously, 0f-0b (ud2) wrote…
There is not necessarily anything after
index: value-initializing a class type (as innew T()) without a non-default constructor fills padding bytes with zero, which I think is more likely what's happening here. I can't say for sure because I don't have a matching implementation ofGameDataFile's constructor yet.
Nice, I didn't know that yet - confirmed with some experiments. Thanks for the explanation!
Needs open-ead/agl#19. Hacks were used to force a match in
getShopItemInfoListandgetWorldIdForNewReleaseShop.This change is
Report for 1.0 (92f78b4 - 9f686d3)
📈 Matched code: 13.49% (+0.18%, +22992 bytes)
✅ 464 new matches
System/GameDataFunctionGameDataFunction::makeTextureSaveDataFileName(sead::BufferedSafeStringBase<char>*, nn::g3d::ResFile const*, GameDataHolder const*, int)System/GameDataFunctionGameDataFunction::checkIsComplete(al::IUseSceneObjHolder const*, int)System/GameDataFunctionGameDataFunction::wearCostumeRandom(al::IUseSceneObjHolder*)System/GameDataFunctionGameDataFunction::wearCapRandom(al::IUseSceneObjHolder*)System/GameDataFunctionGameDataFunction::tryFindShineMessage(al::LayoutActor const*, int, int)System/GameDataFunctionGameDataFunction::tryFindShineMessage(al::LiveActor const*, al::IUseMessageSystem const*, int, int)System/GameDataFunctionGameDataFunction::getWorldIdForNewReleaseShop(GameDataHolderAccessor, int)System/GameDataFunctionGameDataFunction::getWorldNumForNewReleaseShop(GameDataHolderAccessor)System/GameDataFunctionGameDataFunction::tryGetWorldNameByFileId(al::LayoutActor const*, int)System/GameDataFunctionGameDataFunction::tryGetWorldNameCurrent(al::LayoutActor const*)System/GameDataFunctionGameDataFunction::tryGetRegionNameCurrent(al::LayoutActor const*)System/GameDataFunctionGameDataFunction::isPlayerStartLinkedObj(al::LiveActor const*, al::ActorInitInfo const&, char const*)System/GameDataFunctionGameDataFunction::calcHintTransMostNear(sead::Vector3<float>*, GameDataHolderAccessor, sead::Vector3<float> const&)System/GameDataFunctionGameDataFunction::tryGetWorldName(al::LayoutActor const*, int)System/GameDataFunctionGameDataFunction::findAreaAndChangeNextStage(GameDataHolderWriter, al::LiveActor const*, sead::Vector3<float> const*)System/GameDataFunctionGameDataFunction::checkIsNewWorldInAlreadyGoWorld(GameDataHolderAccessor)System/GameDataFunctionGameDataFunction::checkEnableUnlockWorldSpecial1(al::LiveActor const*)System/GameDataFunctionGameDataFunction::checkEnableUnlockWorldSpecial2(al::LiveActor const*)System/GameDataFunctionGameDataFunction::getCurrentShineNum(GameDataHolderAccessor)System/GameDataFunctionGameDataFunction::isForwardWorldWarpDemo(GameDataHolderAccessor)System/GameDataFunctionGameDataFunction::isRemovedCapByJango(al::LiveActor const*)System/GameDataFunctionGameDataFunction::isGetCapFromJango(al::LiveActor const*)System/GameDataFunctionGameDataFunction::removeCapByJango(al::LiveActor const*)System/GameDataFunctionGameDataFunction::getCapFromJango(al::LiveActor const*)System/GameDataFunctionGameDataFunction::setCostumeRandomMode(al::IUseSceneObjHolder*)System/GameDataFunctionGameDataFunction::setCapRandomMode(al::IUseSceneObjHolder*)System/GameDataFunctionGameDataFunction::checkShineSeaOfTreeForShineList(al::LayoutActor const*, int, int)System/GameDataFileGameDataFile::HintInfo::isEnableUnlock(int, bool, int, bool) constSystem/GameDataFunctionGameDataFunction::tryFindExistCoinCollectStagePosExcludeHomeStageInCurrentWorld(sead::Vector3<float>*, char const**, GameDataHolderAccessor)System/GameDataFunctionGameDataFunction::isPlayerStartObj(al::LiveActor const*, al::ActorInitInfo const&)...and 434 more new matches