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

Expand $OUT_DIR in include_aseprite and include_background_gfx #545

Merged
merged 5 commits into from
Feb 3, 2024

Conversation

screenshakes
Copy link
Contributor

@screenshakes screenshakes commented Jan 24, 2024

As discussed in #544, this expand $OUT_DIR when used in the include_aseprite and include_background_gfx macros in order to support including from the out directory.

Example usage:
const SPRITE_0: &Graphics = include_aseprite!("$OUT_DIR/sprite_0.aseprite");

  • Changelog updated / no changelog update needed

Copy link
Member

@gwilymk gwilymk left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. Was on holiday over the past week :).

Would you mind also adding an entry to the changelog and mentioning this in the documentation for the macros in the agb crate itself?

@@ -310,6 +314,7 @@ pub fn include_aseprite_inner(input: TokenStream) -> TokenStream {
let filenames: Vec<PathBuf> = parsed
.iter()
.map(|s| s.value())
.map(|s| s.replace(OUT_DIR_TOKEN, &out_dir_path))
.map(|s| Path::new(&root).join(&*s))
Copy link
Member

Choose a reason for hiding this comment

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

is $OUT_DIR relative to the cargo manifest dir or is it absolute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value you get from std::env::var("OUT_DIR") is absolute.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, missed this bit in the docs:

If path is absolute, it replaces the current path.

@@ -663,6 +668,16 @@ fn valid_sprite_size(width: u32, height: u32) -> bool {
}
}

const OUT_DIR_TOKEN: &str = "$OUT_DIR";

fn get_out_dir(raw_input: &str) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

given the usage here, should this function just do the replacement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split it to get then replace in order not to call std::env::var for every file when importing multiple sprites.

@@ -171,6 +171,15 @@ impl config::Config for IncludeBackgroundGfxInput {
}
}

/// Including from the out directory is supported through the `$OUT_DIR` token.
///
/// ```rust,ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to use ignore here because ci would fail since we don't have any build script to create the file in the out directory.
If necessary I could add a build script that copy an example file to the directory instead.

@@ -91,6 +91,17 @@ macro_rules! align_bytes {
/// name in code. You should ensure tags are unique as this is not enforced by
/// aseprite.
///
/// Including from the out directory is supported through the `$OUT_DIR` token.
///
/// ```rust,ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same

@gwilymk gwilymk enabled auto-merge February 3, 2024 20:23
@gwilymk
Copy link
Member

gwilymk commented Feb 3, 2024

Thanks! We'll probably do a release on Tuesday :D

@gwilymk gwilymk merged commit ac8e7d8 into agbrs:master Feb 3, 2024
1 check passed
@gwilymk
Copy link
Member

gwilymk commented Feb 6, 2024

@screenshakes We've just released 0.18.1 :)

@screenshakes
Copy link
Contributor Author

👍

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