fishfolk / bones Goto Github PK

We might be able to just just pass references with a lifetime for acessing it, instead of cloning the Arc like we do here.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

We want you to be able to set the sprite "color" or "tint": the color the sprite pixels are multiplied by, which is pure white ( no change ) by default.

This should be a simple change.

We just need to add a color field to Sprite here:

And we need to set the Bevy sprite color from the bones sprite color here:

This issue was generated by todo-issue based on a TODO comment in 6164d35.

Right now when you have multiple system params that borrow the same resource or component you get a vague Already mutably borrowed error with no extra explanation.

We should give a much better panic message, making note of which resource/component is already borrowed and which system if at all possible.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

From chat:

It already removes dead entities from entities.iter_with(), and that's because it requires you to use the entities list to iterate, so it knows which ones are alive.

[17:57]
It just isn't automatic when using some_component.get() because it can't know which entities are alive without borrowing the entities.

[17:57]
Maybe we make some equivalent to some_component.get() that takes entities as an argument?

[17:57]
(edited)
Or maybe we do entities.get(some_component, entity) instead.

[17:58]
And then we recommend against using some_component.get() because it won't detect dead entities.

Yendor#2746
Yesterday at 18:14
how does bevy currently deal with this?

[18:14]
i think it might error out that entity doersnt exist

Zicklag
Yesterday at 18:14
Good question

[18:15]
I think the closest analogy to bones might be that all systems/components implicitly borrow Res<Entities> and therefore have access to the alive entities list, but that makes it impossible to borrow ResMut<Entities> in any system, and you are required to use Commands in order to create entities.

[18:16]
The tricky part might be how to allocate new entity IDs across multiple systems that might be running in parallel.

[18:17]
I don't think there's a way to do that deterministically.

[18:17]
Right now we don't do any multi-threading in bones, so maybe it's just not a problem.

[18:17]
(edited)
If we did multi-threading at all I think that might kill determinism gaurantees anyway.

Yendor#2746
Yesterday at 18:18
well thats the issue with bevy currently is that there is no determinism when running systems without explict system ordering

Zicklag
Yesterday at 18:18
Yeah, so if we say that we only guarantee determinism when not multi-threading...

[18:19]
Then we might be able to use an AtomicUsize to allocate new entities when you need to create them in a system.

[18:20]
Maybe we can just make the whole Entities resource completely thread safe.

[18:20]
So you can create and kill entities with only a Res<Entities>.

[18:21]
And then all of the Comp<SomComponent> borrows would have a handle to Entities that it could use to know when each entity is alive or not.

Yendor#2746
Yesterday at 18:30
Having the comp have access to entities is definitely a win there. I am a bit hesitant to hand the user the api to kill and create entities on demand

[18:32]
I think if we could hide that implementation and only use it within bones for components that would be ideal. Giving the user that much control over the system could lead to a foot gun scenario

Zicklag
Yesterday at 18:56
I am a bit hesitant to hand the user the api to kill and create entities on demand

[18:56]
I think that it's not that differet from the way we have it today.

[18:56]
We have to have a way to add and delete entities somehow.

[18:59]
We might be able to steal the entity allocator from [hecs](https://docs.rs/hecs), though we'd have to see if it was a good fit.

[18:59]
I think Bevy's is too baked into the grander ECS to be stealable by itself.

In short, the idea is to make Entites thread safe so that component storage can always know when an entity is alive and avoid returning dead entities.

This would fix #82.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

We should double-check, but IIRC we don't use blocking anymore, and we definitely don't have a maximum number of matches. This is old code from when we ran entire bevy instances for each match on the matchmaking server:

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

There's been at least one, and maybe two cases where we've run into a killed entity getting re-created ( with a different generation and the same entity index ), and then reading stale components from the deleted entity. ( See also #82 )

I just realized, though, that calling world.maintain() every loop should at least remove the component storage for killed entities, so that may not actually be removing stale component data properly. We need to test and make sure it's doing what it should be.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

For troubleshooting determinism failures, it'd be good to have a way to checksum the world. We could do that by having an optional checksum function pointer in our untyped storage, similar to how we have an optional function pointer for cloning non-Copy types.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

Right now bones makes heavy use of the TypeUlid trait to identify types uniquely apart from their unstable Rust TypeIds.

I'm realizing, though, that it might be more appropriate to take the approach used by Bevy for non-Rust types, which is to require all components and resources to be registered with the world, at which point they are given a ComponentId or a ResourceId.

Additionally, at registration, I think we would allow specifying a type path of sorts, that would look like a Rust module path probably.

When registering a type, you provide it's layout and its module path, and it would fail to register if you had already inserted a type with the same module path.

The module paths give us a much more explanatory way to reference the types in future scripting APIs, too, because otherwise we would be using the ULID, which has absolutely no correlation to the actual type being referenced.

Finally, this would make it much easier to use components in Rust, because we don't have to add a TypeUlid to everything.

Luckily, this change wouldn't really break any existing code. It would just remove the requirement to have a TypeUlid. We'd have to try it out in the codebase, though, just to make sure we're not using TypeUlids in some context where a pre-registered component/resource ID isn't going to work very well, but I think we'd be fine.

I think something might be wrong with our cache for the docs job in CI. It seems to take longer than it should, but I haven't looked into it yet. I could be wrong.

We should be able to create a derive macro for SystemParam, very similar to the one in Bevy, that makes it easier to make a system param struct out of other system params.

Hello, I'm currently trying to use quinn_runtime_bevy with Bevy 0.9.1 but when I run my program it tells me thread 'IO Task Pool (0)' panicked at 'A IoTaskPool has not been initialized yet. Please call IoTaskPool::init beforehand.', [...] whereas when I don't use Bevy directly but bevy_tasks with version 0.8.1 it works fine.

I think the solution is to update bevy_tasks to the latest version for quinn_runtime_bevy.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

Some of them may not be used anymore.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

In order to get bones published to crates.io, we started depending on the released version of bevy_simple_tilemap which pulls in a lot of bevy features and dependencies that we don't need.

We have an open PR to address this: forbjok/bevy_simple_tilemap#9.

As soon as that merges and gets released we want to update so that we can strip out those dependencies again.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

The TypeUlid derive macro requires that you add the type_ulid crate to the Cargo.toml. We should fix that.

It looks like the latest rust nightly stabilized a feature, so now our CI jobs that require nightly are failing for some reason.

forbjok/bevy_simple_tilemap#9

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

Say you do this.

world.run_system(
  |mut entities: ResMut<Entities>, positions: CompMut<Pos>| 
    // Create an entity
    let entity = entities.create();
    positions.insert(entity, default());

    // Kill that entity
    entities.kill(entity);
    
    // This works
    let pos = positions.get(entity).unwrap();
  });

The issue is that the component storage doesn't know which entities are alive. So if you want this to behave as you would probably expect, you must do something like this:

world.run_system(
  |mut entities: ResMut<Entities>, positions: CompMut<Pos>| 
    // Create an entity
    let entity = entities.create();
    positions.insert(entity, default());

    // Kill that entity
    entities.kill(entity);
    
    // Pre-check entity live-ness
    if entities.is_alive(entity) {
      let pos = positions.get(entity).unwrap();
    }
  });

And that's not cool.

Maybe we can store store the list of valid generations for entities in the component stores, and update that every frame. So the example above would still work counter-intuitively, but after one frame the component store would become aware of the entity being deleted?

That's still kind of confusing.

I'm not sure if we can make some sort of atomic bitset that can efficiently update the list of alive entities across all components. We could put an Arc<AtomicRefCell<Bitset>> or something like that to keep track of alive entities, and then all component storages have a reference to the alive entities, maybe. That might hurt performance because of the locking, but it would only have to lock inefficiently when you have mutable access to the Entities in a system, so maybe that wouldn't be a problem.

I think that any solution that improves the ergonomics here, and doesn't seriously impact performance should probably be considered, even if it isn't perfect, or the fastest thing on the planet.

I haven't done a lot of in-depth thinking about different ways to design the ECS that might remedy this issue. Most of the overall design idea was borrowed from Planck, but there might be some divergence we can make here that might be more efficient and fix the confusion, I'm not sure.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue is to discuss an ergonomics issue that I've faced both in Bones ECS and in Bevy ECS, and to explore whether or not there's a good solution.

The scenario is simple:

I have a custom system parameter named CollisionWorld that contains helper methods and logic around detecting collisions and manipulating the physics world.

As a part of that CollisionWorld parameter, I borrow the Transform components mutably. This allows me to, as a part of collision world methods like translate move entities, and also to get the entities positions, as is necessary for collision detection.

This causes an ergonomics problem when any the user wants to include both a CollisionWorld system param, and a CompMut<Transforms> system param, because that is a conflict: you have two mutable borrows of Transform components.

The current solution in my WIP branch in jumpy is to make the transforms: CompMut<Transform> field of CollisionWorld public. This allows you to access a borrow of the transforms, but it's not a perfect solution, and users are not going to expect that it's impossible to borrow both their own transforms argument, and the CollisionWorld.

Also, considering the situation where the CollisionWorld and another system parameter needs to borrow CompMut<Transform>, there is no good workaround.

This is partially just a limitation of the way borrowing works. The issue is "solved" in Bevy using ParamSets which usually feels un-ergonomic, but again, there's only so much we can do in Rust, where we must make our borrowing intentions clear at compile time. We can't have two mutable references to the Transform components at the same time. The only way around this is to delay the actual borrowing of the components, requiring an extra lock()/borrow() step.

Maybe we make a Defer system parameter, that wraps around other system parameters, deferring the borrow, and requiring an extra borrow() call to do runtime borrow checking later in the function.

In that case you would be allowed to have transforms: Defer<CompMut<Transform>> and collision_world: Defer<CollisionWorld> in your system params, but you would have to .borrow() them before you could use them, and you wouldn't be able to .borrow() them at the same time without a panic at runtime.

Finally, another alternative, is to have CollisionWorld use it's own component to represent the entity positions, and this component must be synchronized somehow with the Transform component. This is also a foot-gun because it's easy to forget to synchronize the values, and that there is in fact a separate CollisionWorld version of the entity position.

This issue was generated by todo-issue based on a TODO comment in 6164d35.

This issue was generated by todo-issue based on a TODO comment in 27a70cd. It's been assigned to @zicklag because they committed the code.