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

Migrate to bevy v0.6 #12

Merged
merged 5 commits into from Jan 27, 2022
Merged

Migrate to bevy v0.6 #12

merged 5 commits into from Jan 27, 2022

Conversation

mnett82
Copy link
Contributor

@mnett82 mnett82 commented Jan 10, 2022

This PR mostly deals with breaking changes from migrating to bevy version 0.6, as per the Bevy migration guide.

Some callouts:

  • Used StorageType::SparseSet for FlyCam, since I expect it to only exist approximately for one entity.
  • Added [[example]] sections to the repository config.

@mnett82
Copy link
Contributor Author

mnett82 commented Jan 10, 2022

One important note: I did not nudge the version to 0.6, please let me know if that's desirable.

@BlackPhlox
Copy link
Collaborator

BlackPhlox commented Jan 24, 2022

Code Migration looks very good! I think a lot of users use their own 0.6 impl., so an updated version is very much needed.
Thank you for your contribution! 🎉

Some notes on the callouts:

  1. As shown in the bevy 0.5 update notes, SparseSet advantage is faster add/remove operations but slower iterations, so SparseSet for FlyCam doesn't really make sense unless you need to spawn and despawn thousand of fly cams 😅
  2. [[example]] Makes sense for bevy as the repo has nested examples, thus allowing for shorter namespaces instead of specifying the full path i.e: cargo run --release --example examples/some_nested_dir/basic. However, for repos with a flat directory of examples (like this one) cargo run --release --example basic works just fine 😊

Besides the callouts, I want to publish this though please note that I do not have the ownership to publish the 0.6 crates.io, which is currently only @sburris0's domain.

@BlackPhlox
Copy link
Collaborator

Oh and yes, please do nudge to 0.6, as the plugin has breaking changes 😊

Cargo.toml Outdated Show resolved Hide resolved
mnett82 and others added 2 commits January 25, 2022 20:30
Removes `[[example]]` stanzas from Cargo config.

Co-authored-by: Mikkel Rasmussen <theluja@gmail.com>
@mnett82
Copy link
Contributor Author

mnett82 commented Jan 25, 2022

@BlackPhlox Thanks for taking a look at the PR!

I bumped the crate version and reverted the changes to the examples.

I agree, having a large number of FlyCam components and massively adding/removing them both seem like unrealistic scenarios. Given that, my choice of SparseSet for storage was with the goal to minimize memory footprint. However, I think I misunderstood the default archetype-based storage.

Before reverting this to default storage, could you confirm that this will not grow linearly with the number of entities? Thanks!

@BlackPhlox
Copy link
Collaborator

Thanks! I don't think there is any discernable difference in memory footprint between the two storage types, as they both grow linearly with the size of entities, though I've asked in the discord just to be sure. From what I can see the storage types only affect iteration and operation speed.

@BlackPhlox
Copy link
Collaborator

BlackPhlox commented Jan 25, 2022

@maniwani (Joy from discord) was amazing at taking their time to educate me on bevy_ecs, I can highly recommend reading the response. So yes, the memory footprint is mostly the same, and Sparse might even be potentially bigger than the default storage type because of the required relations.

As per discussion with @BlackPhlox, this is not actually reducing the
memory footprint in the ECS. On the flipside, removing the storage type
is expected to improve query performance.
@mnett82
Copy link
Contributor Author

mnett82 commented Jan 27, 2022

Apologies for the slow progress, have been busy at work the last days.

@BlackPhlox Thanks for digging up the details on Discord, definitely learned a lot looking through it. I've removed the SparseSet storage type from the PR.

I also pinged @sburris0 via mail since it doesn't seem that they have been active on Github for a while, but let's see what happens.

@BlackPhlox
Copy link
Collaborator

BlackPhlox commented Jan 27, 2022

Thanks and no worries, I've been in contact with Spencer and I have received ownership on crates.io so I can publish a 0.6 version 🎉 See #13 for reference.

src/lib.rs Outdated
@@ -26,6 +26,8 @@ impl Default for MovementSettings {
}

/// Used in queries when you want flycams and not other cameras
#[derive(Component)]
#[component(storage = "SparseSet")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Callout 1:

Suggested change
#[component(storage = "SparseSet")]

@BlackPhlox BlackPhlox merged commit 6a0aeb4 into sburris0:master Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants