Add component existence check#216
Open
nuskey8 wants to merge 4 commits into
Open
Conversation
Owner
|
Noted, I think it's time to finally introduce safety checks. Im gonna look at this once I have some free time and merge it :) |
Owner
|
Just took a deeper look at it. Are there any performance drawbacks? An small benchmark would be great :) |
Author
|
Sorry for the delay with my assignment. Below is a simple benchmark code and its results. using Arch.Core;
namespace Arch.Benchmarks;
[MemoryDiagnoser]
public class AddRemoveBenchmark
{
World world;
Entity entity;
[GlobalSetup]
public void Setup()
{
world = World.Create();
entity = world.Create();
}
[GlobalCleanup]
public void Cleanup()
{
world.Dispose();
}
[Benchmark]
public void AddRemove()
{
for (int i = 0; i < 100000; i++)
{
world.Add<Velocity>(entity);
world.Remove<Velocity>(entity);
}
}
}before: after: Surprisingly, the modified version is slightly faster. However, this difference is negligible and, in practice, there is almost no difference in execution speed. |
Owner
|
Thanks that looks promising! Could you also benchmark your version against the arch nugget? It might make the benchmark more complicated, but the goal is to benchmark both versions in the same benchmark running once. Not before and after ^^ This would verify it and we can merge it :) |
penspanic
added a commit
to penspanic/Arch
that referenced
this pull request
Jul 14, 2026
Adding a component an entity already has, or removing one it does not, produces a destination archetype identical to the source. World.Move then appended the entity to a second slot of the same archetype and the following source-side removal overwrote a sibling entity's component data. A Debug.Assert guarded this in debug builds but is compiled out of the shipped Release package, so the corruption was completely silent. All structural entry points funnel through Move (Add/Remove, the non-generic and *Range overloads, and CommandBuffer playback), so all were affected. Turn that debug-only assert into a real InvalidOperationException. Empty AddRange / RemoveRange are now guarded as legitimate no-ops so they no longer trip the check. Adds regression tests for the direct, non-generic and deferred paths, and reworks the AddRemove benchmark to measure the success-path cost of the guard. Completes genaray#216. Fixes genaray#224, genaray#253. Addresses genaray#305. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
penspanic
added a commit
to penspanic/Arch
that referenced
this pull request
Jul 14, 2026
RemoveNonExistentComponentsGeneratedThrows is #if !EVENTS: under EVENTS the generated remove fires OnComponentRemoved<T>(entity) before Move, and that hook Get<T>s the absent component — the pre-existing Get-of-absent hazard (other half of upstream genaray#216) crashes with an AccessViolation before the same-archetype guard is reached. Reproducible on upstream master. WorldConcurrencyTest is #if !PURE_ECS: the static World registry it exercises (and the Entity extension methods it uses) do not exist there. Suite green across Debug/Release x Default/Events/PureECS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 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.
Add checks and exceptions for Add/Remove/Get/Set related to #174. This PR will change it to throw
InvalidOperationExceptionif you specify a component type that does not exist.It is recommended to use exceptions instead of assertions here. These are public APIs of the library, and an exception should be thrown if the user specifies an incorrect parameter. When these defects occur during release builds, Arch makes heavy use of unsafe memory operations, which can cause crashes or unexpected behavior, making it difficult to identify the cause. For the above reasons, I changed
Debug.AssertofWorld.Moveto an exception.The reason why I use a dedicated
ThrowHelperto throw exceptions is due to optimization. This method is widely used in .NET.