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

Update with_bevy demo package #406

Merged
merged 12 commits into from
Nov 5, 2023
Merged

Update with_bevy demo package #406

merged 12 commits into from
Nov 5, 2023

Conversation

nixon-voxell
Copy link
Contributor

  • reenable with_bevy package demo as bevy 0.12 now supports wgpu 0.17
  • update with_bevy demo to support bevy 0.12

@nixon-voxell nixon-voxell marked this pull request as ready for review November 5, 2023 05:22
@DJMcNab
Copy link
Member

DJMcNab commented Nov 5, 2023

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

@DJMcNab
Copy link
Member

DJMcNab commented Nov 5, 2023

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.

@nixon-voxell
Copy link
Contributor Author

Hey no worries! I guess we can close it for now. I'll follow closely with your updates in my fork.

@nixon-voxell
Copy link
Contributor Author

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!

@DJMcNab
Copy link
Member

DJMcNab commented Nov 5, 2023

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

Copy link
Member

@DJMcNab DJMcNab left a 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

@nixon-voxell
Copy link
Contributor Author

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)

@DJMcNab
Copy link
Member

DJMcNab commented Nov 5, 2023

I think you should do it - I want to make sure it works as expected on your machine as well with that change

Copy link
Member

@DJMcNab DJMcNab left a 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

@@ -19,6 +19,8 @@ use bevy::{
#[derive(Resource)]
struct VelloRenderer(Renderer);

unsafe impl Sync for VelloRenderer {}
Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Member

@DJMcNab DJMcNab left a 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

@DJMcNab DJMcNab merged commit 78aa9e8 into linebender:main Nov 5, 2023
4 checks passed
@nixon-voxell nixon-voxell deleted the bevy branch November 6, 2023 13:01
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.

2 participants