-
Notifications
You must be signed in to change notification settings - Fork 24
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
Bevy 0.6 Upgrade #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is still in draft form, but I thought I'd give it a quick review to help speed it along 😉
And for others' reference, this PR is a temporary fix until the other solutions proposed in #5 have been worked out. This is just so we can get something up and running for Bevy 0.6 while that change goes into development. |
prototype: &'c dyn Prototypical, | ||
commands: EntityCommands<'a, 'b>, | ||
) -> ProtoCommands<'a, 'b, 'c> { | ||
pub fn get_commands<'w, 's, 'a, 'p>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I like keeping the lifetimes consistent with what bevy names its lifetimes. Since these are essentially wrappers on those underlying objects, it makes it quite a bit more straightforward to reason about which lifetime corresponds to which object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree (hence the 'a
and 'b
used previously). Thanks for doing that!
Cargo.toml
Outdated
@@ -1,7 +1,7 @@ | |||
[package] | |||
name = "bevy_proto" | |||
version = "0.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to bump the minor version with this change at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I'll definitely bump the major version haha, just prefer to do that last in case other PRs or changes need to be merged before bumping. Just keeps everything on the same page 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I still need to run it myself to verify, but I'll try to get this out asap.
Cargo.toml
Outdated
@@ -1,7 +1,7 @@ | |||
[package] | |||
name = "bevy_proto" | |||
version = "0.2.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I'll definitely bump the major version haha, just prefer to do that last in case other PRs or changes need to be merged before bumping. Just keeps everything on the same page 🙂
prototype: &'c dyn Prototypical, | ||
commands: EntityCommands<'a, 'b>, | ||
) -> ProtoCommands<'a, 'b, 'c> { | ||
pub fn get_commands<'w, 's, 'a, 'p>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree (hence the 'a
and 'b
used previously). Thanks for doing that!
I'll try my hand at implementing the other solution today as well, but if I can't get it done, I'll just release what you have here. I don't want to delay a 0.6-compatible release for those that need it. |
All the examples seem to run fine. Thank you for the changes! |
Will resolve #5
Right now this only fixes the basic and templates examples. The other two depend on changes in the renderer that still need work.
Does not yet implement the discussed better
ProtoComponent
approach. This branch just hacks by makingProtoComponent
extendSend + Sync + 'static