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

Scaffold initial bevy plugin #951 #952

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

tychedelia
Copy link
Collaborator

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.

    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;
Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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!

Copy link
Member

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 👍

Copy link
Member

@mitchmindtree mitchmindtree left a 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;
Copy link
Member

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 👍

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

🚀

@tychedelia tychedelia merged commit d13d1a4 into nannou-org:bevy-refactor Jan 18, 2024
9 checks passed
@@ -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"] }
Copy link
Member

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:

Suggested change
bevy = { git = "https://github.com/bevyengine/bevy", branch = "main", features = ["default"] }
bevy = "0.12"

Copy link
Collaborator Author

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.

Copy link
Member

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)?

Copy link
Collaborator Author

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!

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

Successfully merging this pull request may close these issues.

3 participants