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

bevy_crevice derive macro doesn't work properly in external crate #3436

Closed
alice-i-cecile opened this issue Dec 25, 2021 · 3 comments
Closed
Labels
A-Utils Utility functions and types C-Bug An unexpected or incorrect behavior S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@alice-i-cecile
Copy link
Member

Bevy version

main

Proplem

The bevy_crevice_path macro returns the render_resource path segment two times if one uses the bevy_derive macros in a local project.

Proposed patch

diff --git a/crates/bevy_crevice/bevy-crevice-derive/src/lib.rs b/crates/bevy_crevice/bevy-crevice-derive/src/lib.rs
index e0cc77a8..421c4aa9 100644
--- a/crates/bevy_crevice/bevy-crevice-derive/src/lib.rs
+++ b/crates/bevy_crevice/bevy-crevice-derive/src/lib.rs

@@ -47,14 +47,17 @@ fn bevy_crevice_path() -> Path {
                 segments,
             }
         })
-        .or_else(|| bevy_manifest.maybe_get_path(crate::BEVY_RENDER))
-        .map(|bevy_render_path| {
-            let mut segments = bevy_render_path.segments;
-            segments.push(BevyManifest::parse_str("render_resource"));
-            Path {
-                leading_colon: None,
-                segments,
-            }
+        .or_else(|| {
+            bevy_manifest
+                .maybe_get_path(crate::BEVY_RENDER)
+                .map(|bevy_render_path| {
+                    let mut segments = bevy_render_path.segments;
+                    segments.push(BevyManifest::parse_str("render_resource"));
+                    Path {
+                        leading_colon: None,
+                        segments,
+                    }
+                })
         })
         .unwrap_or_else(|| bevy_manifest.get_path(crate::BEVY_CREVICE))
 }

Additional information

Submitted on behalf of a user on Discord.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Utils Utility functions and types S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Dec 25, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Dec 25, 2021

@alice-i-cecile I know you're the ticketmain, but you are also allowed to create pull requests

That would make this proposed patch easier to review

Fwiw, I'm surprised by this patch; I'd expect the fix to be making the bevy::render::render_resources which the map is operating on be just bevy::render

But again, I can't review it easily on mobile, so it's challenging to check my assumptions there.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 25, 2021

As I mentioned on discord (in the linked thread) the correct fix is to just delete line 44 (I think), as explained above.

I'm not too bothered about attribution for that fix, but can create a pr if no-one else does before I get around to it.

@mockersf
Copy link
Member

oh I was testing that fix locally too @DJMcNab 👍

@bors bors bot closed this as completed in f3b053d Dec 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Bug An unexpected or incorrect behavior S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
3 participants