Use Ark.jl as the ECS Backend#1
Conversation
|
@ameligrana is attempting to deploy a commit to the sinistermage's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request integrates the Ark ECS library, replacing the project's internal entity and component management system. EntityID changes from a simple UInt64 to an Ark-based type, world creation delegates to Ark, and all component operations (add, get, remove, query) now use Ark's abstractions instead of internal COMPONENT_STORES. Entity creation, removal, and serialization paths are updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Project.toml (1)
25-40:⚠️ Potential issue | 🟡 MinorAdd a
[compat]entry for Ark to avoid resolver drift.Ark is added to
[deps]but not constrained in[compat], which can allow breaking versions and cause registry CI failures. Pin Ark to a compatible range, such as"0.3"(for v0.3.x) to match the latest available versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Project.toml` around lines 25 - 40, Add a compat entry for Ark to avoid resolver drift by adding a line under the [compat] table that constrains Ark to a compatible range (e.g., "0.3" to pin to v0.3.x); update the Project.toml [compat] block so it contains a matching Ark = "0.3" entry to mirror the Ark dependency declared in [deps] and prevent unconstrained upgrades.src/ecs.jl (1)
258-263:⚠️ Potential issue | 🟡 MinorStale docstring — "entity counter" no longer exists.
The docstring still says "Clears ECS stores, entity counter, …" but the entity counter was removed when Ark took over entity management.
📝 Proposed fix
-Reset all engine globals for scene switching. Clears ECS stores, entity counter, -physics world, trigger state, particle pools, terrain cache, LOD cache, +Reset all engine globals for scene switching. Clears ECS world state (via Ark), +physics world, trigger state, particle pools, terrain cache, LOD cache,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ecs.jl` around lines 258 - 263, Update the stale docstring on the Reset all engine globals block (the docstring for reset_engine_globals / reset_engine_globals!) to remove the phrase "entity counter" and any implication that this function resets entity ID management; instead state it clears ECS stores, physics world, trigger state, particle pools, terrain/LOD/world-transform caches, and asset-manager cache while leaving audio device/context untouched and noting that Ark now manages entities. Keep the rest of the docstring intact and ensure the wording reflects that entity management is handled by Ark.
🧹 Nitpick comments (1)
src/ecs.jl (1)
58-59:ComponentStoreis exported but has no implementation or usages in the codebase.The struct is declared as empty and explicitly exported (line 308 in
src/OpenReality.jl), with a comment labeling it as a "Compatibility Layer". If it serves a legitimate purpose for external consumers or backward compatibility, keep it; if it's no longer needed, remove it via proper deprecation rather than silently deleting exported API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ecs.jl` around lines 58 - 59, The exported struct ComponentStore{T <: Component} is declared empty and unused; either implement it or deprecate/remove it intentionally: if it must remain for backwards compatibility, add a minimal documented implementation (e.g., a concrete container field like Dict{Int,T} or Vector{T}) and a comment explaining the compatibility layer and intended API in the ComponentStore definition, otherwise mark it deprecated by keeping a thin shim that throws a DeprecationWarning (or call `@deprecate` from the module where it is exported) and update the public export list to remove or replace ComponentStore; also update any module-level exports referencing ComponentStore and add/adjust tests and docs accordingly to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ecs.jl`:
- Around line 115-126: The nil-check branches are unreachable because the
functions' signatures accept EntityID (Ark.Entity) so Julia will never dispatch
with nothing; update all four functions—add_component!, get_component,
has_component, remove_component!—to either remove the ark_entity === nothing
guards and the hidden-entity creation logic, or change their argument type to
Union{EntityID, Nothing} and handle creating a new entity when ark_entity ===
nothing (ensuring the created entity is used when adding the provided component
in add_component! rather than discarded). Decide one approach and apply it
consistently across the four functions, removing dead code if you choose the
stricter EntityID signature.
- Around line 244-254: The query-created world lock can remain held if the body
function throws; update iterate_components (and likewise collect_components and
entities_with_component) to create the query into a local variable (e.g. q =
Ark.Query(world, (T,))) and wrap the iteration in a try/finally so that in the
finally block you explicitly close the query (call the query's close/cleanup
method or appropriate Ark API to release the query and world lock) ensuring the
query is always closed even if f(ark_ent, col[i]) throws.
- Around line 19-22: The call to Ark.World in initialize_world uses a keyword
allow_mutable which is not in Ark.jl docs; verify the Ark.World constructor
signature in your environment and either remove that unsupported keyword or
replace it with the correct supported keyword (e.g. initial_capacity or the
actual mutability flag) so initialize_world(types..., ...) calls a valid
constructor; update the initialize_world function where Ark.World is invoked
and, if you need to support multiple Ark versions, implement a small conditional
(feature-check or try/catch) around Ark.World(...) to call the correct parameter
set and fall back to a constructor without allow_mutable, referencing the
initialize_world function, Ark.World, COMPONENT_TYPES and the module-level
_WORLD initialization for context.
- Around line 14-16: Constructors EntityID(x::Int) and EntityID(x::Int, y::Int)
currently call private Ark internals (Ark._new_entity, fields ._id and ._gen)
and Base.isless only compares ._id which breaks total ordering for recycled IDs;
update the EntityID constructors to use the public World API (e.g.
new_entity!(world, ...) or another exposed factory) instead of Ark._new_entity,
stop referencing private fields directly, and change Base.isless(a::EntityID,
b::EntityID) to compare both a._id and a._gen (e.g. compare _id first then _gen)
so that entities with equal ids but different generations are ordered
deterministically (preserving correctness for
SortedDict/sort!/binary-search/dedup).
In `@src/export/scene_export.jl`:
- Around line 165-168: The loop that writes entity IDs uses Ark internals
(eid._id, eid._gen) and packs the bits backwards; replace the direct field
access with Ark's public accessors (use the appropriate public functions your
Ark version exposes to obtain the entity id and generation) and pack as
(UInt64(gen) << 32) | UInt64(id) before calling write(io, ...); also update the
ORSB serialization comment/spec and the corresponding deserialization/loader to
expect generation in the high 32 bits.
In `@src/rendering/camera_utils.jl`:
- Around line 10-17: The function find_active_camera currently uses
iterate_components(CameraComponent) with a do-block and a bare return which only
exits the closure, so it ends up returning the last active camera; fix by
enabling early exit: either (A) implement an exception-based early-return inside
find_active_camera — define a small local exception type or reuse a sentinel
(e.g., FoundActiveCamera) and inside the iterate_components do-block
throw(FoundActiveCamera(eid)) when cam.active, then wrap the iterate_components
call in try/catch to catch FoundActiveCamera and return its eid, or (B) replace
the implementation with a dedicated helper like
find_first_component(CameraComponent, predicate) that iterates components and
returns the first matching eid directly (and update find_active_camera to call
that helper); reference symbols: find_active_camera, iterate_components,
CameraComponent, active_eid, and the new FoundActiveCamera or
find_first_component helper.
In `@src/serialization/save_load.jl`:
- Around line 56-73: The load path currently resets component stores with
reset_component_stores!() but then re-attaches deserialized components to
EntityID values without ensuring those entities exist in the Ark World; because
add_component! only auto-creates entities when passed nothing, components for
deserialized IDs get dropped. Fix load_game by explicitly creating each entity
ID from save_data["entities"] in the Ark World immediately after
reset_component_stores!() (e.g., iterate save_data["entities"] and call the Ark
entity-creation API so EntityIDs exist) before running the component
re-attachment loop that calls add_component! / Ark.add_components!.
In `@test/runtests.jl`:
- Around line 14-22: Tests use fresh world = World() but call
create_entity!(World()) instead, causing hard-coded id expectations to break;
change to call create_entity!(world) so both entities are created in the same
local World instance and replace fixed assertions with relative checks (e.g.,
assert id2._id == id1._id + 1 or assert id1._id == 1 and id2._id == 2)
referencing create_entity!, world, id1._id and id2._id.
---
Outside diff comments:
In `@Project.toml`:
- Around line 25-40: Add a compat entry for Ark to avoid resolver drift by
adding a line under the [compat] table that constrains Ark to a compatible range
(e.g., "0.3" to pin to v0.3.x); update the Project.toml [compat] block so it
contains a matching Ark = "0.3" entry to mirror the Ark dependency declared in
[deps] and prevent unconstrained upgrades.
In `@src/ecs.jl`:
- Around line 258-263: Update the stale docstring on the Reset all engine
globals block (the docstring for reset_engine_globals / reset_engine_globals!)
to remove the phrase "entity counter" and any implication that this function
resets entity ID management; instead state it clears ECS stores, physics world,
trigger state, particle pools, terrain/LOD/world-transform caches, and
asset-manager cache while leaving audio device/context untouched and noting that
Ark now manages entities. Keep the rest of the docstring intact and ensure the
wording reflects that entity management is handled by Ark.
---
Nitpick comments:
In `@src/ecs.jl`:
- Around line 58-59: The exported struct ComponentStore{T <: Component} is
declared empty and unused; either implement it or deprecate/remove it
intentionally: if it must remain for backwards compatibility, add a minimal
documented implementation (e.g., a concrete container field like Dict{Int,T} or
Vector{T}) and a comment explaining the compatibility layer and intended API in
the ComponentStore definition, otherwise mark it deprecated by keeping a thin
shim that throws a DeprecationWarning (or call `@deprecate` from the module where
it is exported) and update the public export list to remove or replace
ComponentStore; also update any module-level exports referencing ComponentStore
and add/adjust tests and docs accordingly to reflect the chosen approach.
| EntityID(x::Int) = Ark._new_entity(UInt32(x), UInt32(0)) | ||
| EntityID(x::Int, y::Int) = Ark._new_entity(UInt32(x), UInt32(y)) | ||
| Base.isless(a::EntityID, b::EntityID) = a._id < b._id |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for usage of Ark's internals and EntityID comparison
rg "_new_entity|_id|_gen|isless.*EntityID" --type julia -nRepository: sinisterMage/Open-Reality
Length of output: 50381
🏁 Script executed:
#!/bin/bash
# Find Entity or EntityID definitions and imports
rg "EntityID|Ark\.Entity|import.*Ark" --type julia -n -A 2 -B 2Repository: sinisterMage/Open-Reality
Length of output: 50382
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or tests for EntityID behavior
find . -type f \( -name "*.jl" -o -name "*.md" \) -exec grep -l "EntityID\|entity.*recycle" {} \;Repository: sinisterMage/Open-Reality
Length of output: 1828
🏁 Script executed:
#!/bin/bash
# Search for SortedDict or other sorted structures using EntityID
rg "SortedDict|sort!.*EntityID|searchsortedfirst.*EntityID" --type julia -nRepository: sinisterMage/Open-Reality
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check for entity recycling or generation usage patterns
rg "recycle|generation|_gen" --type julia -n | head -50Repository: sinisterMage/Open-Reality
Length of output: 2203
🏁 Script executed:
#!/bin/bash
# Search for Ark imports and usage pattern
rg "import.*Ark|using.*Ark" --type julia -nRepository: sinisterMage/Open-Reality
Length of output: 100
🌐 Web query:
Ark.jl Julia entity component system public API Entity constructor
💡 Result:
In Ark.jl, Entity is a public type, but you generally don’t construct entities directly (i.e. there isn’t a “user-facing” Entity(...) constructor intended for creating live entities). Instead, entities are created through a World:
- Create one entity:
new_entity!(world, (Position(0,0), Velocity(1,1)))→ returns anEntity[1] - Create many entities:
new_entities!(world, n, (Position, Velocity)) do ... end(or pass default values) [1]
The only “direct” entity value meant for users is:
zero_entity::Entity(a reserved sentinel meaning “no entity”) [1]
Private Ark internals + incomplete isless definition risk data-structure corruption for recycled entity IDs.
Two issues here:
-
Ark._new_entity,._id, and._genare private implementation details (underscore convention). Any Ark.jl patch/minor release can rename or remove them, silently breaking these constructors and the ordering. -
Base.islessonly compares_id, dropping_gen. Entity IDs include a generation counter (confirmed in serialization code and game context comments for recycled entities). Two entities with the same_idbut different_genare distinct. Ignoring_genin ordering corrupts anySortedDict,sort!, binary-search, or deduplication that depends onisless.
The generation must be included to form a total order:
🐛 Proposed fix
-Base.isless(a::EntityID, b::EntityID) = a._id < b._id
+Base.isless(a::EntityID, b::EntityID) = a._id < b._id || (a._id == b._id && a._gen < b._gen)Longer term, replacing Ark._new_entity with the public World API (via new_entity!(world, ...)) removes the fragility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ecs.jl` around lines 14 - 16, Constructors EntityID(x::Int) and
EntityID(x::Int, y::Int) currently call private Ark internals (Ark._new_entity,
fields ._id and ._gen) and Base.isless only compares ._id which breaks total
ordering for recycled IDs; update the EntityID constructors to use the public
World API (e.g. new_entity!(world, ...) or another exposed factory) instead of
Ark._new_entity, stop referencing private fields directly, and change
Base.isless(a::EntityID, b::EntityID) to compare both a._id and a._gen (e.g.
compare _id first then _gen) so that entities with equal ids but different
generations are ordered deterministically (preserving correctness for
SortedDict/sort!/binary-search/dedup).
| function add_component!(ark_entity::EntityID, component::T) where T <: Component | ||
| world = World() | ||
| if ark_entity === nothing | ||
| ark_entity = Ark.new_entity!(world, ()) | ||
| end | ||
| if Ark.has_components(world, ark_entity, (T,)) | ||
| Ark.set_components!(world, ark_entity, (component,)) | ||
| else | ||
| # Add new component | ||
| push!(store.components, component) | ||
| idx = length(store.components) | ||
| store.entity_map[entity_id] = idx | ||
| store.index_to_entity[idx] = entity_id | ||
| Ark.add_components!(world, ark_entity, (component,)) | ||
| end | ||
|
|
||
| return nothing | ||
| end |
There was a problem hiding this comment.
ark_entity === nothing guards are unreachable dead code in all four component-operation functions.
EntityID is Ark.Entity; Julia's dispatch rejects Nothing before the function body is entered, so the === nothing branches in add_component! (Line 117–119), get_component (Line 136), has_component (Line 151), and remove_component! (Line 165) can never execute. The hidden entity-creation fallback in add_component! (Lines 118–119) is equally unreachable and creates an anonymous entity without the provided component before the else branch assigns it—confusing and dead.
Either remove the guards entirely, or change the signatures to Union{EntityID, Nothing} if nothing is a legitimate input.
🐛 Proposed fix for `add_component!`
-function add_component!(ark_entity::EntityID, component::T) where T <: Component
- world = World()
- if ark_entity === nothing
- ark_entity = Ark.new_entity!(world, ())
- end
- if Ark.has_components(world, ark_entity, (T,))
+function add_component!(ark_entity::EntityID, component::T) where T <: Component
+ world = World()
+ if Ark.has_components(world, ark_entity, (T,))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ecs.jl` around lines 115 - 126, The nil-check branches are unreachable
because the functions' signatures accept EntityID (Ark.Entity) so Julia will
never dispatch with nothing; update all four functions—add_component!,
get_component, has_component, remove_component!—to either remove the ark_entity
=== nothing guards and the hidden-entity creation logic, or change their
argument type to Union{EntityID, Nothing} and handle creating a new entity when
ark_entity === nothing (ensuring the created entity is used when adding the
provided component in add_component! rather than discarded). Decide one approach
and apply it consistently across the four functions, removing dead code if you
choose the stricter EntityID signature.
| function iterate_components(f::Function, ::Type{T}) where T <: Component | ||
| store = get_component_store(T) | ||
| if store === nothing | ||
| return nothing | ||
| end | ||
|
|
||
| for (entity_id, idx) in store.entity_map | ||
| f(entity_id, store.components[idx]) | ||
| world = World() | ||
| for (entities, cols...) in Ark.Query(world, (T,)) | ||
| col = cols[1] | ||
| for i in eachindex(entities) | ||
| ark_ent = entities[i] | ||
| f(ark_ent, col[i]) | ||
| end | ||
| end | ||
| return nothing | ||
| end |
There was a problem hiding this comment.
World lock is never released if f throws inside iterate_components — permanent deadlock for component operations.
"The world gets locked for component operations when a query is created." The lock is released automatically only when iteration completes naturally; when breaking out early, the query must be closed manually, and "the lock prevents entity creation and removal, as well as adding and removing components."
If f(ark_ent, col[i]) throws, the for loop is abandoned mid-iteration without releasing the lock. Every subsequent call to add_component!, remove_component!, create_entity!, etc. will deadlock.
The same exception-safety gap exists in collect_components (Line 186) and entities_with_component (Line 203), though those are less likely to encounter exceptions since they only call append!.
Wrap the loop body in a try/finally:
🔒 Proposed fix
function iterate_components(f::Function, ::Type{T}) where T <: Component
world = World()
- for (entities, cols...) in Ark.Query(world, (T,))
- col = cols[1]
- for i in eachindex(entities)
- ark_ent = entities[i]
- f(ark_ent, col[i])
- end
+ q = Ark.Query(world, (T,))
+ try
+ for (entities, cols...) in q
+ col = cols[1]
+ for i in eachindex(entities)
+ f(entities[i], col[i])
+ end
+ end
+ catch
+ Ark.close!(q)
+ rethrow()
end
return nothing
end📝 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.
| function iterate_components(f::Function, ::Type{T}) where T <: Component | |
| store = get_component_store(T) | |
| if store === nothing | |
| return nothing | |
| end | |
| for (entity_id, idx) in store.entity_map | |
| f(entity_id, store.components[idx]) | |
| world = World() | |
| for (entities, cols...) in Ark.Query(world, (T,)) | |
| col = cols[1] | |
| for i in eachindex(entities) | |
| ark_ent = entities[i] | |
| f(ark_ent, col[i]) | |
| end | |
| end | |
| return nothing | |
| end | |
| function iterate_components(f::Function, ::Type{T}) where T <: Component | |
| world = World() | |
| q = Ark.Query(world, (T,)) | |
| try | |
| for (entities, cols...) in q | |
| col = cols[1] | |
| for i in eachindex(entities) | |
| f(entities[i], col[i]) | |
| end | |
| end | |
| catch | |
| Ark.close!(q) | |
| rethrow() | |
| end | |
| return nothing | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ecs.jl` around lines 244 - 254, The query-created world lock can remain
held if the body function throws; update iterate_components (and likewise
collect_components and entities_with_component) to create the query into a local
variable (e.g. q = Ark.Query(world, (T,))) and wrap the iteration in a
try/finally so that in the finally block you explicitly close the query (call
the query's close/cleanup method or appropriate Ark API to release the query and
world lock) ensuring the query is always closed even if f(ark_ent, col[i])
throws.
| for eid in entities | ||
| write(io, UInt64(eid)) | ||
|
|
||
| # TODO: do not use internals of Ark | ||
| write(io, (UInt64(eid._id) >> 32) | UInt64(eid._gen)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and locate scene_export.jl
find . -name "scene_export.jl" -type fRepository: sinisterMage/Open-Reality
Length of output: 97
🏁 Script executed:
# Examine the code at lines 165-168
head -170 src/export/scene_export.jl | tail -20Repository: sinisterMage/Open-Reality
Length of output: 736
🏁 Script executed:
# Get more context around those lines to understand the implementation
head -180 src/export/scene_export.jl | tail -40Repository: sinisterMage/Open-Reality
Length of output: 1582
🌐 Web query:
Ark.jl EntityID serialization API stable conversion UInt64
💡 Result:
Ark.jl’s Entity is an isbits identifier made of two UInt32 fields: an ID (_id) and a generation (_gen). (raw.githubusercontent.com)
For stable serialization, you must preserve both (ID, generation); storing only the ID is not stable because IDs are reused with incremented generations.
Stable UInt64 packing (recommended)
pack_entity(e::Ark.Entity)::UInt64 =
(UInt64(getfield(e, :_gen)) << 32) | UInt64(getfield(e, :_id))
unpack_entity(u::UInt64)::NamedTuple{(:id,:gen),Tuple{UInt32,UInt32}} =
(id = UInt32(u & 0xFFFF_FFFF), gen = UInt32(u >> 32))Important note about “API-stable”
Ark.jl does not expose a public Entity(id, gen) constructor; Entity instances are meant to come from a World (e.g. new_entity!). (raw.githubusercontent.com)
So, treat unpack_entity as producing a serialized representation you can compare/store, not something you can necessarily turn back into a live world entity without Ark’s (currently internal) constructors/world-loading mechanisms.
Citations:
- 1: https://raw.githubusercontent.com/ark-ecs/Ark.jl/main/src/entity.jl
- 2: https://raw.githubusercontent.com/ark-ecs/Ark.jl/main/src/entity.jl
🌐 Web query:
Ark.jl julia entity id packing format specification
💡 Result:
In Ark.jl, an Entity is a generational ID: it consists of
id(the slot / index in the world’s entity index)gen(generation counter incremented whenever thatidis recycled after removal)
This is explicitly how Ark’s entity liveness works: entities are recycled, and the generation distinguishes current vs. stale references. [1], [2]
Packing format (bit layout)
Ark.jl’s entity can be represented as a single 64-bit value by packing two 32-bit unsigned integers:
- low 32 bits:
id - high 32 bits:
gen
So the canonical packing/unpacking is:
packed = (UInt64(gen) << 32) | UInt64(id)
id = UInt32(packed & 0xFFFF_FFFF)
gen = UInt32(packed >> 32)Notes / constraints
zero_entityis the reserved “no entity” placeholder. [1]- Generations are effectively 32-bit; Ark’s docs note that a relationship target could only reappear after dying ~4,294,967,294 times (≈ 2^32). [2]
Sources: Ark.jl Public API / Entities (Entity, zero_entity, is_alive) [1]; Ark.jl Manual / Architecture (recycling + generation semantics, 2^32-scale wraparound remark) [2].
Fix incorrect Entity ID packing and use of Ark internals.
This code packs Entity ID fields with two problems: (1) it accesses internal _id and _gen fields that Ark.jl does not expose as a stable public API, risking breakage if Ark's implementation changes, and (2) the bit manipulation is backwards. The canonical format is (UInt64(gen) << 32) | UInt64(id) (generation in high 32 bits, id in low 32 bits), but the code does (UInt64(eid._id) >> 32) | UInt64(eid._gen), which shifts the id right and places gen in the low bits. Either correct the packing to (UInt64(eid._gen) << 32) | UInt64(eid._id), explicitly document this serialization format in the ORSB spec and deserialization loader, or refactor to avoid direct field access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/export/scene_export.jl` around lines 165 - 168, The loop that writes
entity IDs uses Ark internals (eid._id, eid._gen) and packs the bits backwards;
replace the direct field access with Ark's public accessors (use the appropriate
public functions your Ark version exposes to obtain the entity id and
generation) and pack as (UInt64(gen) << 32) | UInt64(id) before calling
write(io, ...); also update the ORSB serialization comment/spec and the
corresponding deserialization/loader to expect generation in the high 32 bits.
| active_eid = nothing | ||
| iterate_components(CameraComponent) do eid, cam | ||
| if cam.active | ||
| active_eid = eid | ||
| return # This return is for the closure, not find_active_camera | ||
| end | ||
| end | ||
| return nothing | ||
| return active_eid |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate iterate_components definition and check its implementation
echo "=== Searching for iterate_components definition ==="
rg -n "function iterate_components" --type=julia -A 15
echo ""
echo "=== Searching for iterate_components call sites ==="
rg -n "iterate_components" --type=julia -B 1 -A 5
echo ""
echo "=== Reading full camera_utils.jl file ==="
cat -n src/rendering/camera_utils.jlRepository: sinisterMage/Open-Reality
Length of output: 21635
return inside the closure does not short-circuit iteration — function returns the last active camera, contradicting the docstring
The comment on line 14 correctly identifies that the return exits only the do-block closure, not find_active_camera. This creates a critical inconsistency:
-
Logic bug — "first" vs "last": The docstring claims the function finds the first entity with
cam.active == true, butiterate_componentscontinues for all remaining entities. Every subsequent active camera overwritesactive_eid, so the function returns the last active entity found in iteration order, not the first. -
Performance: All
CameraComponententities are traversed regardless of when a match occurs.
The current implementation of iterate_components (defined in src/ecs.jl:244-254) has no mechanism to support early termination; it always iterates through all entities and returns nothing. Fix options:
✓ Option A — exception-based early exit (works with current iterate_components design)
+struct _StopIteration <: Exception end
+
function find_active_camera()
active_eid = nothing
- iterate_components(CameraComponent) do eid, cam
- if cam.active
- active_eid = eid
- return # This return is for the closure, not find_active_camera
- end
- end
+ try
+ iterate_components(CameraComponent) do eid, cam
+ if cam.active
+ active_eid = eid
+ throw(_StopIteration())
+ end
+ end
+ catch e
+ e isa _StopIteration || rethrow()
+ end
return active_eid
end⚠ Option B — add a dedicated findfirst helper (cleaner)
Alternatively, define a helper function that wraps the early-exit logic, or modify iterate_components to return an iterable so standard Julia patterns like findfirst work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendering/camera_utils.jl` around lines 10 - 17, The function
find_active_camera currently uses iterate_components(CameraComponent) with a
do-block and a bare return which only exits the closure, so it ends up returning
the last active camera; fix by enabling early exit: either (A) implement an
exception-based early-return inside find_active_camera — define a small local
exception type or reuse a sentinel (e.g., FoundActiveCamera) and inside the
iterate_components do-block throw(FoundActiveCamera(eid)) when cam.active, then
wrap the iterate_components call in try/catch to catch FoundActiveCamera and
return its eid, or (B) replace the implementation with a dedicated helper like
find_first_component(CameraComponent, predicate) that iterates components and
returns the first matching eid directly (and update find_active_camera to call
that helper); reference symbols: find_active_camera, iterate_components,
CameraComponent, active_eid, and the new FoundActiveCamera or
find_first_component helper.
| world = World() | ||
| for T in COMPONENT_TYPES | ||
| T in _NON_SERIALIZABLE_TYPES && continue | ||
|
|
||
| entries = Tuple{EntityID, Vector{UInt8}}[] | ||
| for (eid, idx) in store.entity_map | ||
| comp = store.components[idx] | ||
| try | ||
| buf = IOBuffer() | ||
| Serialization.serialize(buf, comp) | ||
| push!(entries, (eid, take!(buf))) | ||
| catch | ||
| # Component has non-serializable fields — skip silently | ||
| for (entities, col) in Ark.Query(world, (T,)) | ||
| for i in eachindex(entities) | ||
| eid = entities[i] | ||
| comp = col[i] | ||
| try | ||
| buf = IOBuffer() | ||
| Serialization.serialize(buf, comp) | ||
| push!(entries, (eid, take!(buf))) | ||
| catch | ||
| # Component has non-serializable fields — skip silently | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find src -name "*.jl" -type f | head -20Repository: sinisterMage/Open-Reality
Length of output: 705
🏁 Script executed:
# First, let's locate the relevant files
git ls-files src --name-only | grep -E "(save|ecs|load)" | head -20Repository: sinisterMage/Open-Reality
Length of output: 2152
🏁 Script executed:
# Get an overview of the structure
ls -la src/Repository: sinisterMage/Open-Reality
Length of output: 1378
🏁 Script executed:
# Check what files are in serialization directory
ls -la src/serialization/Repository: sinisterMage/Open-Reality
Length of output: 244
🏁 Script executed:
# Now let's examine the ecs.jl file to find add_component! definition
wc -l src/ecs.jlRepository: sinisterMage/Open-Reality
Length of output: 83
🏁 Script executed:
# Read ecs.jl to understand add_component! semantics
cat -n src/ecs.jlRepository: sinisterMage/Open-Reality
Length of output: 9954
🏁 Script executed:
# Now let's examine the save_load.jl file to see the load_game implementation
cat -n src/serialization/save_load.jlRepository: sinisterMage/Open-Reality
Length of output: 5576
Recreate entities in Ark before re-attaching components during load.
After reset_component_stores!() clears the Ark world, load_game calls add_component!() on deserialized EntityIDs without first creating those entities in the world. The add_component! function only auto-creates an entity when passed nothing; in the load path, EntityIDs are deserialized from the save file (not nothing), so the entity creation branch never triggers. Consequently, Ark.add_components! is called on non-existent entities, which will silently drop components or fail silently.
Add explicit entity creation for each EntityID in save_data["entities"] immediately after reset_component_stores!() but before the component re-attachment loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/serialization/save_load.jl` around lines 56 - 73, The load path currently
resets component stores with reset_component_stores!() but then re-attaches
deserialized components to EntityID values without ensuring those entities exist
in the Ark World; because add_component! only auto-creates entities when passed
nothing, components for deserialized IDs get dropped. Fix load_game by
explicitly creating each entity ID from save_data["entities"] in the Ark World
immediately after reset_component_stores!() (e.g., iterate save_data["entities"]
and call the Ark entity-creation API so EntityIDs exist) before running the
component re-attachment loop that calls add_component! / Ark.add_components!.
| @testset "World-based entity creation" begin | ||
| world = World() | ||
| @test world.next_entity_id == 1 | ||
|
|
||
| id1 = create_entity!(world) | ||
| @test id1 == 1 | ||
| @test world.next_entity_id == 2 | ||
| id1 = create_entity!(World()) | ||
| @test id1._id == 2 | ||
|
|
||
| id2 = create_entity!(world) | ||
| @test id2 == 2 | ||
| id2 = create_entity!(World()) | ||
| @test id2._id == 3 | ||
| end |
There was a problem hiding this comment.
Avoid hard-coded EntityID values in tests.
These assertions depend on global ECS state and can become order-dependent if any other test creates entities first. Prefer relative checks or use the local world variable consistently.
Suggested test adjustment
- id1 = create_entity!(World())
- `@test` id1._id == 2
-
- id2 = create_entity!(World())
- `@test` id2._id == 3
+ id1 = create_entity!(world)
+ id2 = create_entity!(world)
+ `@test` id2._id == id1._id + 1📝 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.
| @testset "World-based entity creation" begin | |
| world = World() | |
| @test world.next_entity_id == 1 | |
| id1 = create_entity!(world) | |
| @test id1 == 1 | |
| @test world.next_entity_id == 2 | |
| id1 = create_entity!(World()) | |
| @test id1._id == 2 | |
| id2 = create_entity!(world) | |
| @test id2 == 2 | |
| id2 = create_entity!(World()) | |
| @test id2._id == 3 | |
| end | |
| `@testset` "World-based entity creation" begin | |
| world = World() | |
| id1 = create_entity!(world) | |
| id2 = create_entity!(world) | |
| `@test` id2._id == id1._id + 1 | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runtests.jl` around lines 14 - 22, Tests use fresh world = World() but
call create_entity!(World()) instead, causing hard-coded id expectations to
break; change to call create_entity!(world) so both entities are created in the
same local World instance and replace fixed assertions with relative checks
(e.g., assert id2._id == id1._id + 1 or assert id1._id == 1 and id2._id == 2)
referencing create_entity!, world, id1._id and id2._id.
|
Woah! thank you so much for taking the time to integrate ark.jl!! about the test fails, yeah i need to fix those issues, thanks for pointing it out! also, sorry for the noisy coderabbitai, didnt remember i gave it permission, but anyways, LGTM |
2 participants
Hi @sinisterMage, I tried to help integrating Ark.jl into Open-Reality.
Consider though that I don't think this is what you want in full, though I think that starting from this, you should be able to easily change the things you are interested in. There are also some usage of Ark internals, which can be avoided, but to move fast, I just used them, but also those should be easy to avoid. I encourage you to change anything you'd like at this stage. I think also that a lot of stuff can be improved. Tests should pass, but I had some failures even before applying the changes, so it is possible that there are some fails even when all rendering backends are correctly imported. Also, I didn't update docs.
Benchmarks are already much better (though still not as good as using Ark directly):
unfortunately I'm a bit tight on time, so if you can help out, please do!