Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump entities to u128 to avoid collisions (#117) #393

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Aug 30, 2020

Resolves #369
Also lets ecs_bench_suit run the add_remove bench for bevy_ecs

Copy link
Member

@CleanCut CleanCut left a comment

Choose a reason for hiding this comment

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

This seems to fix the issue in #369, at least I didn't reproduce the crash when running on macOS for several minutes. Without the change, the longest I was able to run the code in #369 before crashing was ~3.1s.

I can't speak to how this affects the other factors discussed in #117, but it seems like a very pragmatic resolution to current issue(s) and could still be replaced in favor of one of the other implementations in the future.

@karroffel karroffel added the A-ECS Entities, components, systems, and events label Aug 30, 2020
@cart
Copy link
Member

cart commented Aug 30, 2020

I ran some perf tests earlier today using my ecs_bench fork. This has no effect on iteration performance (as expected), but adding entities/components does take a measurable hit:

  • u32: 270 us
  • u128: 357 us

First, I still consider 357us to be acceptable. This is still competitive with other frameworks, but it does take us much farther away from legion and hecs perf.

In the short term, I am inclined to merge this to resolve the collision bugs people are encountering. But the drop is significant enough that I will likely be more open to reverting to an incrementing index and adding a separate "stable" entity id for when its needed.

@zicklag
Copy link
Member

zicklag commented Aug 31, 2020

In the short term, I am inclined to merge this to resolve the collision bugs people are encountering. But the drop is significant enough that I will likely be more open to reverting to an incrementing index and adding a separate "stable" entity id for when its needed.

That sounds like a good plan to me. This will keep things operating for users while we come up with a more long-term solution.

@cart cart merged commit 57177c9 into bevyengine:master Aug 31, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spawing many Entities and then despawing them leads to NoSuchEntity.
5 participants