Skip to content

container: Implement remaining PtrArray functions#255

Open
german77 wants to merge 2 commits into
open-ead:masterfrom
german77:ptrarray
Open

container: Implement remaining PtrArray functions#255
german77 wants to merge 2 commits into
open-ead:masterfrom
german77:ptrarray

Conversation

@german77
Copy link
Copy Markdown
Contributor

@german77 german77 commented Apr 26, 2026

PtrArray is a core element of any sead game. We should have a full implementation of this container. The code is a bit finicky and I did not managed to keep the match and make it look clean.

insertArray is not matching. There's a register that is flipped and I didn't manage to fix it. https://decomp.me/scratch/fBexb


This change is Reviewable

Copy link
Copy Markdown
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


modules/src/container/seadPtrArray.cpp line 152 at r1 (raw file):

    createVacancy(idx, arrayLength);

    for (s32 i = 0; i < arrayLength; ++i)

Suggestion:

for (s32 i = 0; i < arrayLength; i++)

modules/src/container/seadPtrArray.cpp line 171 at r1 (raw file):

    {
        s32 lastSwapForward = left;
        for (s32 i = left; i < right; ++i)

Suggestion:

for (s32 i = left; i < right; i++)

modules/src/container/seadPtrArray.cpp line 187 at r1 (raw file):

        s32 lastSwapBackward = right;
        for (s32 i = right; i > left; --i)

Suggestion:

for (s32 i = right; i > left; i--)

modules/src/container/seadPtrArray.cpp line 234 at r1 (raw file):

    {
        void* val = ptrs[n - 1];
        ptrs[--n] = ptrs[0];

Suggestion:

        n--;
        void* val = ptrs[n];
        ptrs[n] = ptrs[0];

modules/src/container/seadPtrArray.cpp line 248 at r1 (raw file):

            ptrs[parent - 1] = ptrs[child - 1];
            parent = child;
        }

this part could be a common inline function, restoreTree(...) or similar?

Code quote:

        while (parent * 2 <= n)
        {
            s32 child = parent * 2;
            if (child < n && cmp(ptrs[child - 1], ptrs[child]) < 0)
                child++;

            if (cmp(val, ptrs[child - 1]) >= 0)
                break;

            ptrs[parent - 1] = ptrs[child - 1];
            parent = child;
        }

modules/src/container/seadPtrArray.cpp line 260 at r1 (raw file):

            return 1;

        const s32 c = cmp(mPtrs[i], other.unsafeAt(i));

might have been optimized?

Suggestion:

other[i]

modules/src/container/seadPtrArray.cpp line 270 at r1 (raw file):

void PtrArrayImpl::uniq(CompareCallbackImpl cmp)
{
    for (s32 i = 0; i < size() - 1; i++)

might have been optimized

Suggestion:

for (s32 i = 0; i < size(); i++)

Copy link
Copy Markdown
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MonsterDruide1 made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on german77).


a discussion (no related file):
To fix BotW's compilation errors, add __attribute__((noinline)) to the first binarySearch in seadObjArray.h.

Copy link
Copy Markdown
Contributor Author

@german77 german77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@german77 made 7 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on MonsterDruide1).


modules/src/container/seadPtrArray.cpp line 248 at r1 (raw file):

Previously, MonsterDruide1 wrote…

this part could be a common inline function, restoreTree(...) or similar?

Both this and the block above should be normal operations of a heap sorting algorithm
But I can't quite make it match https://decomp.me/scratch/w0n8V


modules/src/container/seadPtrArray.cpp line 260 at r1 (raw file):

Previously, MonsterDruide1 wrote…

might have been optimized?

Nope. Is necessary


modules/src/container/seadPtrArray.cpp line 270 at r1 (raw file):

Previously, MonsterDruide1 wrote…

might have been optimized

Nope. Is necessary


modules/src/container/seadPtrArray.cpp line 152 at r1 (raw file):

    createVacancy(idx, arrayLength);

    for (s32 i = 0; i < arrayLength; ++i)

Done.


modules/src/container/seadPtrArray.cpp line 171 at r1 (raw file):

    {
        s32 lastSwapForward = left;
        for (s32 i = left; i < right; ++i)

Done.


modules/src/container/seadPtrArray.cpp line 187 at r1 (raw file):

        s32 lastSwapBackward = right;
        for (s32 i = right; i > left; --i)

Done.


modules/src/container/seadPtrArray.cpp line 234 at r1 (raw file):

    {
        void* val = ptrs[n - 1];
        ptrs[--n] = ptrs[0];

Done.

Copy link
Copy Markdown
Contributor Author

@german77 german77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


a discussion (no related file):

Previously, MonsterDruide1 wrote…

To fix BotW's compilation errors, add __attribute__((noinline)) to the first binarySearch in seadObjArray.h.

Done.

Copy link
Copy Markdown
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


modules/src/container/seadPtrArray.cpp line 234 at r1 (raw file):

Previously, german77 (Narr the Reg) wrote…

Done.

No, unchanged


modules/src/container/seadPtrArray.cpp line 248 at r1 (raw file):

Previously, german77 (Narr the Reg) wrote…

Both this and the block above should be normal operations of a heap sorting algorithm
But I can't quite make it match https://decomp.me/scratch/w0n8V

https://decomp.me/scratch/u34rU
This matches, but I didn't try the second block yet - does that help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants