Skip to content
This repository has been archived by the owner on Nov 27, 2022. It is now read-only.

Update bevy, hecs, legion, specs and shipyard #30

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zakarumych
Copy link

No description provided.

src/hecs/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/hecs/frag_iter.rs Outdated Show resolved Hide resolved
src/hecs/simple_iter.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

hecs changes LGTM. Have not reviewed other parts.

@zakarumych
Copy link
Author

Who should I ask for a review before this can be merged?

Copy link

@laundmo laundmo left a comment

Choose a reason for hiding this comment

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

i'm just a random bevy user coming through here, throwing out thoughts:

  • bevy_ecs 0.8.1 is out, which didn't include any changes to 0.8.0 but may still make sense to bump just to be clear towards readers.
  • bevy_ecs::component::Component is part of the bevy_ecs::prelude::* so those lines could just be #[derive(Component)] instead of the fully qualified path.
  • bevy supports serde, though that should likely be a separate PR (i don't have experience with that part, but I'll throw a note on the bevy discord in case someone there wants to have a look)

These are just some small nitpicks, looks good to me otherwise.

@@ -1,16 +1,16 @@
use bevy_ecs::prelude::*;
use cgmath::*;

#[derive(Copy, Clone)]
#[derive(Copy, Clone, bevy_ecs::component::Component)]

Choose a reason for hiding this comment

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

For an insertion-focused test, I would suggest using Bevy's sparse set storage rather than the default table storage. Ideally there'd be benches for both, but that may be hard to display.

Copy link

@laundmo laundmo Sep 13, 2022

Choose a reason for hiding this comment

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

For using Sparse Set storage you would add a #[component(storage = "SparseSet")] below the Component derive, as it can be used per-component.

Copy link
Author

Choose a reason for hiding this comment

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

Having bench only for SparseSet based storage wouldn't be fair.
It's a tradeoff with significant iteration speed disadvantage.

Having both would be perfect.

Choose a reason for hiding this comment

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

Yep, I'm on board there. I'd love to see both storage types for iteration and insertion speed.

@CGMossa
Copy link

CGMossa commented Nov 11, 2022

Is this going to get merged or what?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants