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

It seems it is required to import vulkano in order to declare shaders in v0.9 #312

Closed
mitchmindtree opened this issue May 13, 2019 · 8 comments · Fixed by #452
Closed

It seems it is required to import vulkano in order to declare shaders in v0.9 #312

mitchmindtree opened this issue May 13, 2019 · 8 comments · Fixed by #452
Assignees
Labels
v0.9 Specific to v0.9

Comments

@mitchmindtree
Copy link
Member

cc @freesig

Would be nice to confirm and if necessary fix this before v0.9.

@mitchmindtree mitchmindtree added the v0.9 Specific to v0.9 label May 13, 2019
@freesig freesig self-assigned this May 14, 2019
@freesig
Copy link
Collaborator

freesig commented May 21, 2019

Yeh this is essentially the problem:

mod vs {
    #[allow(unused_imports)]
    use std::sync::Arc;
    #[allow(unused_imports)]
    use std::vec::IntoIter as VecIntoIter;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorBufferDesc;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorDesc;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorDescTy;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorImageDesc;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorImageDescArray;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::DescriptorImageDescDimensions;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor::ShaderStages;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor_set::DescriptorSet;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor_set::UnsafeDescriptorSet;
    #[allow(unused_imports)]
    use vulkano::descriptor::descriptor_set::UnsafeDescriptorSetLayout;
    #[allow(unused_imports)]
    use vulkano::descriptor::pipeline_layout::PipelineLayout;
    #[allow(unused_imports)]
    use vulkano::descriptor::pipeline_layout::PipelineLayoutDesc;
    #[allow(unused_imports)]
    use vulkano::descriptor::pipeline_layout::PipelineLayoutDescPcRange;
    #[allow(unused_imports)]
    use vulkano::device::Device;
    #[allow(unused_imports)]
    use vulkano::pipeline::shader::SpecializationConstants as SpecConstsTrait;
    #[allow(unused_imports)]
    use vulkano::pipeline::shader::SpecializationMapEntry;
    pub struct Shader {
        shader: ::std::sync::Arc<::vulkano::pipeline::shader::ShaderModule>,
    }

I think we might need to change this macro to crate::vulkano then do use nannou::vulkano.
I can't think of a way to do it without changing the macro because I think use vulkano only searches for external crates.

@freesig
Copy link
Collaborator

freesig commented May 21, 2019

Yep I got this working by adding crate in front all ::vulkano and vulkano.
However you need to add pub use vulkano to the nannou prelude so that this works for all current nannou code.
If you're happy with this solution I can chuck a PR onto vulkano.

@mitchmindtree
Copy link
Member Author

Ahh, is that code above within the body of a macro_rules!? If so, we might be able to change it to use $crate:: instead of vulkano::? I think $crate used to be a shorthand for "this crate, wherever it may be when this macros is invoked". I can't seem to find any reference to it in recent versions of the book though so maybe it was made reduntant with all the module changes in 2018? I'm pretty sure it should be possible to do this in a way that doesn't require any extra imports in nannou.

@freesig
Copy link
Collaborator

freesig commented May 21, 2019

It's a proc_macro. It's actually hard coded into the codegen.rs file in vulkano-shadees.
I just put crate in front of all the use vulkano and it works fine. I think this was just missed by cargo fix because it's a macro

@freesig
Copy link
Collaborator

freesig commented May 21, 2019

As in use crate::vulkano

@mitchmindtree
Copy link
Member Author

Ahhhh of course, my bad, I forgot that it is a proc_macro. Yeah that sounds fine to me!

@freesig
Copy link
Collaborator

freesig commented May 21, 2019

Ok the problem with this solution is that it's a breaking change. So every vulkano project will now need to add use vulkano to their lib.rs or main.rs if it's not already there.
I'm not sure they are going to go for this.
I really wish there was a way to say. use vulkano either from the crate or external.
Can you think of anyway to solve this without forcing everyone to add that line?

@freesig
Copy link
Collaborator

freesig commented May 22, 2019

People are suggesting this but I don't see how that helps. Struggling to find a good solution to this. Pretty much need $crate but for proc macro.

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

Successfully merging a pull request may close this issue.

2 participants