-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Scaffold initial bevy plugin #951 #952
Conversation
bevy-nannou - the "new" top level crate that will be eventually renamed nannou. bevy-nannou-wgpu - port of the nannou-wgpu crate to work with bevy's wgpu types and render apis. bevy-nannou-render - implementation of the existing render pipeline needed for the draw api. bevy-nannou-draw - the draw api. This is currently in the top level crate, but I think makes sense to be split into it's own crate representing a bevy plugin. Right now, we're targeting the main branch of `bevy`. Due to the speed of the ecosystem, it's better (imo) to find out about breaking changes sooner than later as we are in our dev mode. If this becomes annoying with CI, we can always target a specific sha.
@@ -416,7 +416,7 @@ pub fn compile_isf_shader( | |||
.and_then(|(old_str, isf)| { | |||
let isf_str = crate::glsl_string_from_isf(&isf); | |||
let new_str = crate::prefix_isf_glsl_str(&isf_str, old_str); | |||
let ty = hotglsl::ShaderType::Fragment; | |||
let ty = hotglsl::ShaderStage::Fragment; |
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.
@mitchmindtree Kinda confused? Saw you made some changes last night. This is branched off master where it looks like this built clean.
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.
oh, ic it's from #950, nvm!
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.
Ahhh I think because nannou_isf
and the examples
point at the git branch of hotglsl
rather than a fixed version, and I updated hotglsl
last night 😅 I'll fix these deps in #950 when I rebase it onto this 👍
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.
This LGTM! I say we land and I can rebase #950 on top, add any new nix deps required by bevy, etc.
@@ -416,7 +416,7 @@ pub fn compile_isf_shader( | |||
.and_then(|(old_str, isf)| { | |||
let isf_str = crate::glsl_string_from_isf(&isf); | |||
let new_str = crate::prefix_isf_glsl_str(&isf_str, old_str); | |||
let ty = hotglsl::ShaderType::Fragment; | |||
let ty = hotglsl::ShaderStage::Fragment; |
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.
Ahhh I think because nannou_isf
and the examples
point at the git branch of hotglsl
rather than a fixed version, and I updated hotglsl
last night 😅 I'll fix these deps in #950 when I rebase it onto this 👍
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.
🚀
@@ -22,3 +26,6 @@ members = [ | |||
|
|||
# Required for wgpu v0.10 feature resolution. | |||
resolver = "2" | |||
|
|||
[workspace.dependencies] | |||
bevy = { git = "https://github.com/bevyengine/bevy", branch = "main", features = ["default"] } |
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.
Ah I just noticed we point to git here! Is there a reason we use git rather than the crates version here? If not I might change this to use crates.io in #950 like so:
bevy = { git = "https://github.com/bevyengine/bevy", branch = "main", features = ["default"] } | |
bevy = "0.12" |
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.
Bevy moves pretty quick but tends to keep the main branch stable. This was just an attempt to keep up to date with their changes while we're in "dev" mode so we don't have churn on updates from them. If it's a problem for nix, we can definitely just target a stable version.
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'm thinking either way we lock the dependency to a particular commit with Cargo.lock
, but if we use a crates.io version we can choose when we want to update for bevy breakage in a dedicated PR (e.g. we might want to run cargo update
to pull down patches across our deps without also bumping bevy
to the latest main
commit and needing to address any breakage)?
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.
Def makes sense to specify something stable so we can track changes to the framework in dedicates PRs rather than ad hoc. I think my preference would still be for a specific SHA on main
, but I'm really fine with either. I haven't audited what's upcoming in 0.13
, but my concern is mostly that we're using some of the less documented APIs which they break more often. If using an unreleased version causes issues I'm happy to target the latest release. We'll obviously want to target their stable version anyway once we get closer to finished. Up to you!
Provides initial crates/plugins for #951.
Right now, we're targeting the main branch of
bevy
. Due to the speed of the ecosystem, it's better (imo) to find out about breaking changes sooner than later as we are in our dev mode. If this becomes annoying with CI, we can always target a specific sha.