Dynamic object limit (Again)#1225
Open
Sunketchupm wants to merge 10 commits into
Open
Conversation
And fix the bug with it
The change in platform_displacement.c is necessary to avoid a crash
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughGlobal object pool converted from a fixed 1200-entry array to a dynamic GrowingArray* (capacity increased to 4096, init 256). Allocation/deallocation and iterations migrated to the new container. Added u32 obj_get_count() with Lua binding. Several spawn prechecks removed and NULL-safety guards added. ChangesObject Pool Migration & New Counter API
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/engine/graph_node.c (1)
595-600: ⚡ Quick winConsider adding debug logging for the NULL parentLastChild case.
The NULL guard correctly prevents a crash when the parent's child list is corrupted or improperly initialized. However, this creates a silent failure mode where the child is not actually added to the parent's list, but the function still returns
childNodeas if it succeeded.In debug builds, consider logging this condition or adding an assertion to help identify graph corruption during development.
🔍 Optional: Add debug assertion
} else { parentLastChild = parentFirstChild->prev; if (parentLastChild != NULL) { childNode->prev = parentLastChild; childNode->next = parentFirstChild; parentFirstChild->prev = childNode; parentLastChild->next = childNode; + } else { +#ifdef DEBUG + log_to_stdout("geo_add_child: parentLastChild is NULL, graph may be corrupted"); +#endif } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engine/graph_node.c` around lines 595 - 600, The code silently skips linking childNode when parentLastChild is NULL; add a debug-time guard to surface this corruption: inside the same function/block that references parentLastChild, parentFirstChild, and childNode add either an assertion (e.g., assert(parentLastChild != NULL)) or a debug log (using the project's debug/log macro) that prints parent pointer/id and childNode pointer/id when parentLastChild == NULL so developers can detect corrupted/unenitialized parent child lists during development; ensure normal release behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/lua/functions-7.md`:
- Line 3666: The markdown contains an empty-fragment link target "(#)" that
triggers MD042; replace the "(#)" token with a concrete anchor that exists in
the document (for example the heading ID you want to jump to) so the link points
to a real section instead of an empty fragment — locate the "(#)" occurrence and
update it to the appropriate anchor target (e.g., "#<existing-heading-slug>") to
resolve the lint error.
In `@src/engine/surface_load.c`:
- Around line 862-865: The loop dereferences gObjectPool without checking for
NULL; guard gObjectPool before iterating to avoid crashes when the pool isn't
initialized. Change the code around the loop that uses gObjectPool->count and
gObjectPool->buffer (and sets obj->numSurfaces) to first check if gObjectPool is
non-NULL (e.g., if (gObjectPool) { ... }), then iterate over gObjectPool->count
and reset each struct Object* in gObjectPool->buffer by setting obj->numSurfaces
= 0; otherwise skip the loop.
In `@src/game/object_list_processor.c`:
- Around line 595-599: The init currently overwrites gObjectPool with the result
of growing_array_init and on NULL calls clear_objects() then returns, leaving
gObjectPool NULL and causing later derefs; change this so failure is terminal
for the init path or preserves the previous pool: either (a) check the return of
growing_array_init into a local variable, and on NULL call LOG_ERROR and
propagate a failure code (or abort level initialization) instead of returning
after clearing globals, or (b) if preserving state is preferred, do not
overwrite gObjectPool on failure—keep the old gObjectPool and log the error;
specifically modify the block around growing_array_init/gObjectPool and replace
the unconditional assignment/return with a local temp (e.g., newPool) and proper
failure handling so gObjectPool is never left NULL for subsequent code that
dereferences it (also adjust callers to handle the propagated error if you
choose the terminal-failure approach).
In `@src/game/spawn_object.c`:
- Around line 153-158: The code currently pushes obj onto the free list
(freeList / gFreeObjectList) and then calls
growing_array_swap_and_pop(gObjectPool, obj) which frees the pool slot and makes
freeList->next and any raw pointers into the object dangling; remove the call to
growing_array_swap_and_pop here and instead keep the object slot allocated and
stable in gObjectPool (or mark it as free in-place) so pointers like
parentObj/platform pointers remain valid; implement reuse by popping from
gFreeObjectList when allocating new objects rather than immediately freeing the
backing allocation, and defer any actual freeing of the backing store until you
can guarantee no external references remain.
---
Nitpick comments:
In `@src/engine/graph_node.c`:
- Around line 595-600: The code silently skips linking childNode when
parentLastChild is NULL; add a debug-time guard to surface this corruption:
inside the same function/block that references parentLastChild,
parentFirstChild, and childNode add either an assertion (e.g.,
assert(parentLastChild != NULL)) or a debug log (using the project's debug/log
macro) that prints parent pointer/id and childNode pointer/id when
parentLastChild == NULL so developers can detect corrupted/unenitialized parent
child lists during development; ensure normal release behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dfc87cc4-0442-4510-9221-202ac5e7244d
📒 Files selected for processing (20)
autogen/lua_definitions/constants.luaautogen/lua_definitions/functions.luadocs/lua/constants.mddocs/lua/functions-7.mddocs/lua/functions.mdsrc/engine/graph_node.csrc/engine/surface_load.csrc/game/behaviors/tree_particles.inc.csrc/game/object_helpers.csrc/game/object_list_processor.csrc/game/object_list_processor.hsrc/game/platform_displacement.csrc/game/spawn_object.csrc/game/spawn_object.hsrc/pc/lua/smlua_constants_autogen.csrc/pc/lua/smlua_functions_autogen.csrc/pc/lua/utils/smlua_obj_utils.csrc/pc/lua/utils/smlua_obj_utils.hsrc/pc/network/packets/packet_level_macro.csrc/pc/network/packets/packet_level_spawn_info.c
💤 Files with no reviewable changes (1)
- src/game/spawn_object.h
As a consequence, cloning is no longer possible. I will see if I could add that back in.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/game/spawn_object.c (1)
153-158:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftKeep
struct Objectstorage stable across unload.
growing_array_swap_and_pop(gObjectPool, obj)still reclaims/compacts object storage during unload. Engine state keeps rawstruct Object *references outside the pool lists, so this turns stale references into dangling/UAF pointers instead of the old stable-slot behavior. Reuse freed slots in place, or defer reclamation until external references can no longer exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/game/spawn_object.c` around lines 153 - 158, The current call to growing_array_swap_and_pop(gObjectPool, obj) compacts the pool and can move other struct Object pointers, creating dangling external references; instead locate the object's slot in gObjectPool->buffer (search for pointer equality), do not swap or compact, set that buffer[index] = NULL, mark the object as freed (e.g., obj->alive = false or set a freed flag) and record the index for reuse (or push it onto a pool free-list) so future allocations reuse the same slot in place; update any allocation function (the pool allocator) to prefer free-list indices when creating new objects so storage remains stable across unload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/game/object_helpers.c`:
- Around line 2526-2536: Reintroduce a headroom throttle so cosmetic emitters
can't consume the last object slots: before emitting (using variables
numParticles, gPrevFrameObjectCount, OBJECT_POOL_CAPACITY), compute a small
reserve (e.g., reserve = 8–10) and clamp numParticles to at most max(0,
OBJECT_POOL_CAPACITY - gPrevFrameObjectCount - reserve). Also restore the
previous soft cap fallback (the 150/240 threshold limiting numParticles to ~10)
so bursts are reduced early; keep allocate_object() hard-fail protection in
place but ensure emitters never try to allocate the reserved slots.
---
Duplicate comments:
In `@src/game/spawn_object.c`:
- Around line 153-158: The current call to
growing_array_swap_and_pop(gObjectPool, obj) compacts the pool and can move
other struct Object pointers, creating dangling external references; instead
locate the object's slot in gObjectPool->buffer (search for pointer equality),
do not swap or compact, set that buffer[index] = NULL, mark the object as freed
(e.g., obj->alive = false or set a freed flag) and record the index for reuse
(or push it onto a pool free-list) so future allocations reuse the same slot in
place; update any allocation function (the pool allocator) to prefer free-list
indices when creating new objects so storage remains stable across unload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08ce9a90-d114-40c7-b728-121e231cd739
📒 Files selected for processing (9)
src/engine/surface_load.csrc/game/behaviors/bubble.inc.csrc/game/behaviors/water_objs.inc.csrc/game/object_helpers.csrc/game/object_list_processor.csrc/game/object_list_processor.hsrc/game/spawn_object.csrc/pc/network/packets/packet_level_macro.csrc/pc/network/packets/packet_level_spawn_info.c
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pc/network/packets/packet_level_spawn_info.c
- src/pc/network/packets/packet_level_macro.c
- src/game/object_list_processor.h
- src/engine/surface_load.c
- src/game/object_list_processor.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/game/spawn_object.c (1)
157-159:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't free the
struct Objectstorage here.
growing_array_swap_and_pop()changes the old "inactive but address-stable slot" behavior into immediate reclamation. This path only clears a few references before Line 157, so any surviving rawObject *elsewhere becomes a UAF instead of a benign pointer to a deactivated slot. Keep unloaded objects recyclable in place and reuse them on the next allocation rather than reclaiming the backing storage here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/game/spawn_object.c` around lines 157 - 159, Summary: The code currently reclaims the Object's backing storage via growing_array_swap_and_pop and clearing gObjectPool->buffer, turning other raw Object* references into UAFs; instead keep the slot address-stable and mark the object as inactive for reuse. Fix: stop calling growing_array_swap_and_pop(gObjectPool, obj) and stop clearing gObjectPool->buffer[gObjectPool->count]; leave the Object* in the pool and set an inactive flag on the object (e.g., obj->active = false or obj->loaded = false) so the slot remains address-stable and can be recycled on next allocation; remove any obj = NULL assignment and ensure no free()/deallocation is performed in spawn_object.c so existing raw pointers remain valid. Referenced symbols: growing_array_swap_and_pop, gObjectPool, obj, gObjectPool->buffer, gObjectPool->count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/game/spawn_object.c`:
- Around line 157-159: Summary: The code currently reclaims the Object's backing
storage via growing_array_swap_and_pop and clearing gObjectPool->buffer, turning
other raw Object* references into UAFs; instead keep the slot address-stable and
mark the object as inactive for reuse. Fix: stop calling
growing_array_swap_and_pop(gObjectPool, obj) and stop clearing
gObjectPool->buffer[gObjectPool->count]; leave the Object* in the pool and set
an inactive flag on the object (e.g., obj->active = false or obj->loaded =
false) so the slot remains address-stable and can be recycled on next
allocation; remove any obj = NULL assignment and ensure no free()/deallocation
is performed in spawn_object.c so existing raw pointers remain valid. Referenced
symbols: growing_array_swap_and_pop, gObjectPool, obj, gObjectPool->buffer,
gObjectPool->count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b97d8664-98ea-4921-9d6b-b16d656e5bde
📒 Files selected for processing (3)
src/game/behaviors/tree_particles.inc.csrc/game/object_helpers.csrc/game/spawn_object.c
Contributor
Author
|
Part of the reason why I'm pretty much avoiding the "reuse stale or unload later" approach is that fake objects don't crash or cause any issues, which is really the only way to maintain a reference to an unloaded object. So I don't need to complicate the code any further. |
PeachyPeachSM64
requested changes
May 22, 2026
PeachyPeachSM64
left a comment
PeachyPeachSM64
left a comment
Contributor
There was a problem hiding this comment.
Pretty solid so far
The 4096 objects bug should probably be investigated, but not in this PR
Comment on lines
+862
to
+865
| if (gObjectPool == NULL) { | ||
| fprintf(stderr, "FATAL ERROR: The object pool was not created but object surfaces were attempted to be cleared.\n"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Is it a fatal error though?
It doesn't really matter if there is no object pool, since what the following code does is simply setting the objects numSurfaces to 0
Could be replaced by:
LOG_ERROR("The object pool was not created but object surfaces were attempted to be cleared.");
Comment on lines
866
to
869
| for (u16 i = 0; i < gObjectPool->count; i++) { | ||
| struct Object* obj = gObjectPool->buffer[i]; | ||
| obj->numSurfaces = 0; | ||
| } |
Contributor
There was a problem hiding this comment.
you can do this now:
growing_array_for_each_(gObjectPool, struct Object, obj) {
obj->numSurfaces = 0;
}| if (o->oTimer > 100) | ||
| obj_mark_for_deletion(o); | ||
| if (gPrevFrameObjectCount > (OBJECT_POOL_CAPACITY * 212 / 240)) | ||
| if (gPrevFrameObjectCount > OBJECT_POOL_CAPACITY - 200) |
Contributor
There was a problem hiding this comment.
Might be good to have a macro for this, like:
#define OBJECT_POOL_PARTICLES_THRESHOLD (OBJECT_POOL_CAPACITY - 200)
|
|
||
| // If there are a lot of objects already, limit the number of particles | ||
| if (gPrevFrameObjectCount > (OBJECT_POOL_CAPACITY * 150 / 240) && numParticles > 10) { | ||
| if (gPrevFrameObjectCount > OBJECT_POOL_CAPACITY - 200 && numParticles > 10) { |
| // We're close to running out of object slots, so don't spawn particles at | ||
| // all | ||
| if (gPrevFrameObjectCount > (OBJECT_POOL_CAPACITY * 210 / 240)) { | ||
| if (gPrevFrameObjectCount > OBJECT_POOL_CAPACITY - 50) { |
Comment on lines
+25
to
+26
| for (u32 i = 0; i < gObjectPool->count; i++) { | ||
| struct Object* o = gObjectPool->buffer[i]; |
Contributor
There was a problem hiding this comment.
growing_array_for_each_
Comment on lines
+203
to
+204
| for (u32 i = 0; i < gObjectPool->count; i++) { | ||
| struct Object* o2 = gObjectPool->buffer[i]; |
Contributor
There was a problem hiding this comment.
growing_array_for_each_
Comment on lines
+214
to
+215
| for (u32 i = 0; i < gObjectPool->count; i++) { | ||
| struct Object* o2 = gObjectPool->buffer[i]; |
Contributor
There was a problem hiding this comment.
growing_array_for_each_
Comment on lines
+23
to
+24
| for (u32 i = 0; i < gObjectPool->count; i++) { | ||
| struct Object* o = gObjectPool->buffer[i]; |
Contributor
There was a problem hiding this comment.
growing_array_for_each_
| for (s32 i = 0; i < OBJECT_POOL_CAPACITY; i++) { | ||
| struct Object* o = &gObjectPool[i]; | ||
| if (gObjectPool == NULL) { | ||
| fprintf(stderr, "FATAL ERROR: The object pool was not created but object respawn infos were attempted to be checked.\n"); |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/game/platform_displacement.c`:
- Around line 177-179: The code reads player->platform into platform before
ensuring player is non-NULL, risking a null dereference; change the logic in the
function containing m->marioObj so that you first check player != NULL (and the
time-stop condition) and only then access player->platform, or alternatively
move the assignment "struct Object *platform = player->platform;" to after the
guard that verifies player is not NULL; reference the local variables
m->marioObj, player, and platform when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f136e697-2b5d-42b2-b274-861e85874be3
📒 Files selected for processing (17)
autogen/lua_definitions/constants.luaautogen/lua_definitions/functions.luadocs/lua/constants.mddocs/lua/functions-7.mddocs/lua/functions.mdsrc/engine/surface_load.csrc/game/behaviors/tree_particles.inc.csrc/game/object_helpers.csrc/game/object_list_processor.csrc/game/object_list_processor.hsrc/game/platform_displacement.csrc/game/spawn_object.csrc/pc/lua/smlua_constants_autogen.csrc/pc/lua/smlua_functions_autogen.csrc/pc/lua/utils/smlua_obj_utils.hsrc/pc/network/packets/packet_level_macro.csrc/pc/network/packets/packet_level_spawn_info.c
💤 Files with no reviewable changes (1)
- autogen/lua_definitions/functions.lua
✅ Files skipped from review due to trivial changes (3)
- src/pc/lua/smlua_functions_autogen.c
- docs/lua/functions.md
- src/pc/lua/smlua_constants_autogen.c
Comment on lines
+177
to
179
| struct Object *player = m->marioObj; | ||
| struct Object *platform = player->platform; | ||
| if (!(gTimeStopState & TIME_STOP_ACTIVE) && player != NULL && platform != NULL) { |
There was a problem hiding this comment.
Potential null dereference before guard
At Line 178, platform is read via player->platform before confirming player != NULL (the check is only at Line 179). This can crash when m->marioObj is NULL.
Suggested fix
- struct Object *player = m->marioObj;
- struct Object *platform = player->platform;
- if (!(gTimeStopState & TIME_STOP_ACTIVE) && player != NULL && platform != NULL) {
+ struct Object *player = m->marioObj;
+ if (player == NULL) { continue; }
+ struct Object *platform = player->platform;
+ if (!(gTimeStopState & TIME_STOP_ACTIVE) && platform != NULL) {
apply_platform_displacement(player, platform);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| struct Object *player = m->marioObj; | |
| struct Object *platform = player->platform; | |
| if (!(gTimeStopState & TIME_STOP_ACTIVE) && player != NULL && platform != NULL) { | |
| struct Object *player = m->marioObj; | |
| if (player == NULL) { continue; } | |
| struct Object *platform = player->platform; | |
| if (!(gTimeStopState & TIME_STOP_ACTIVE) && platform != NULL) { | |
| apply_platform_displacement(player, platform); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/game/platform_displacement.c` around lines 177 - 179, The code reads
player->platform into platform before ensuring player is non-NULL, risking a
null dereference; change the logic in the function containing m->marioObj so
that you first check player != NULL (and the time-stop condition) and only then
access player->platform, or alternatively move the assignment "struct Object
*platform = player->platform;" to after the guard that verifies player is not
NULL; reference the local variables m->marioObj, player, and platform when
making this change.
Isaac0-dev
reviewed
May 26, 2026
| gDoorAdjacentRooms[i][1] = 0; | ||
| } | ||
|
|
||
| gObjectPool = growing_array_init(gObjectPool, OBJECT_POOL_INIT_CAPACITY, malloc, free); |
Contributor
There was a problem hiding this comment.
I think the object pool should be initialised on startup, to do this use the AT_STARTUP flag with a new function.
This is because the object pool used to exist at all times during program execution, so let's try to keep that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I decided to look into growing arrays again and this time found out how to deallocate in the growing array. The dynamic object limit this time seems to be very stable. There still is a bug where having more than 4096 objects (+ some more) causes newly spawned objects to not load their model, so a limit as been set anyways.
This might be slower performance wise than just increasing the size of the object pool, since every deallocation needs to run a loop to find the object that needs to be deallocated. It may be solvable with a hash map, but I am not able to think of a way to do so that is faster.