-
Notifications
You must be signed in to change notification settings - Fork 142
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
Update with_bevy
demo package
#406
Conversation
…0.17, update with_bevy demo to support bevy 0.12
I don't think it needs to be closed. It's still useful for it to be closer to correct, and this means that when bevy main updates to 0.18, we could re-enable it immediately. I'm just not at a computer which can compile this example yet |
Thanks for implementing this! Unfortunately, wgpu 0.18 has since released (see #398), and we're expecting a Naga patch release on next Wednesday (2023-11-08) which unblocks that PR. So I think that whilst we should merge this, as the update to bevy 0.12 is useful, it's unfortunately unlikely to remain enabled for long. I've not had a chance to test it locally, so I'm not going to approve it yet. |
Hey no worries! I guess we can close it for now. I'll follow closely with your updates in my fork. |
hi @DJMcNab on the side note, is there any plans on supporting textures and colors beyond u8 datatypes? I would love to have a 32 float support for the texture colors as it open doors to HDR/Bloom in the Bevy engine. Is there currently a place for the community server like discord to discuss about these topics? Would love to join them and be part of them! |
I've started a thread on Zulip for that question - https://xi.zulipchat.com/#narrow/stream/197075-gpu/topic/High.20Dynamic.20Range We use Zulip for our synchronous chat - I believe this should also be linked in the README |
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.
Works for me! Thanks
I did run into one issue, which is that this adds a dependency on alsa
, even just for rust-analyzer
. That was pre-existing, but I've since set up a new OS install.
Do you want to reduce the features for bevy to the minimum required set for the example? I think that's:
bevy = { version = "0.12", features = [
"bevy_winit",
"bevy_core_pipeline",
"bevy_pbr",
"bevy_render",
"multi-threaded",
"x11",
"tonemapping_luts",
], default-features = false }
If not, I'm happy to just land this
yeah sure thing, we can reduce it to reduce compile time. Are you going to make the changes or I do it from my side here? (sry am a bit new to open source contributions hahaha) |
I think you should do it - I want to make sure it works as expected on your machine as well with that change |
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.
Sorry I didn't catch this before, but I don't want to add more unsafe code where avoidable
examples/with_bevy/src/main.rs
Outdated
@@ -19,6 +19,8 @@ use bevy::{ | |||
#[derive(Resource)] | |||
struct VelloRenderer(Renderer); | |||
|
|||
unsafe impl Sync for VelloRenderer {} |
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.
We should change VelloRenderer
to use SyncCell instead of adding this implementation.
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.
thanks for the suggestion! I have no idea it can be done like 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.
Thanks! It's nice to get this working again, even if it's just for now
with_bevy
package demo as bevy 0.12 now supports wgpu 0.17with_bevy
demo to support bevy 0.12