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.

Sign up for GitHub

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

Jump to bottom

Add support for importing palette based on Gimp .gpl files #18

Merged
merged 4 commits into from
Jun 3, 2020
Merged

Add support for importing palette based on Gimp .gpl files #18

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b24855
Support importing palette from Gimp *.gpl file
CJLove May 25, 2020
ef809cf
Merge branch 'master' into gpl_support
CJLove May 25, 2020
1b28362
Fix error handling in palette_import()
CJLove May 27, 2020
0af4127
Refactoring to address PR comments, eliminate gimp_palette crate depe…
CJLove May 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 19 additions & 6 deletions src/cmd/palette/command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,26 @@ pub struct PaletteImportArgs {

/// Palette import command
pub fn palette_import(g_args: &GlobalArgs, args: &PaletteImportArgs) -> Result<(), Error> {
let png_bytes = common::read_file_bin(&args.input_file)?;
let pal_config = VeraPaletteLoadConfig {
direct_load: true,
include_defaults: false,
sort: false,
let is_gpl = args.input_file.ends_with("gpl") || args.input_file.ends_with("GPL");
Copy link
Owner

Choose a reason for hiding this comment

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

Just wondering if there's a better way to do this rather than relying on file extension. I believe PNGs have magic bytes that can be used to identify them from raw data (first 8 bytes might be the same). A better workflow might be:

  • Create an enum with PNG and GPL variants
  • Load file as bytes
  • Check first few bytes for png identifier or Gimp Palette (if that's consistent)
  • Call a factored out derive function that takes the bytes and enum type and decides where to go from there

I might be getting ahead of myself a bit here, this is an easy enough refactor to do at a later date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per my first comment, rolling a replacement for the gimp_palette crate is a first step toward enabling this sort of refactoring.
PNG files have an 8 byte header that could be used for identification (https://en.wikipedia.org/wiki/Portable_Network_Graphics#File_header).

let palette = match is_gpl {
true => {
let pal_config = VeraPaletteLoadConfig {
direct_load: true,
include_defaults: false,
sort: false,
};
VeraPalette::derive_from_gpl(&args.id, &args.input_file, &pal_config)?
}
false => {
let png_bytes = common::read_file_bin(&args.input_file)?;
let pal_config = VeraPaletteLoadConfig {
direct_load: true,
include_defaults: false,
sort: false,
};
VeraPalette::derive_from_png(&args.id, png_bytes, &pal_config)?
}
};
let palette = VeraPalette::derive_from_png(&args.id, png_bytes, &pal_config)?;
// load up the project json
let project_file = match &g_args.project_file {
Some(f) => f,
Expand Down
1 change: 1 addition & 0 deletions vera/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ serde_derive = "1"
png = "0.15"
permutate = "0.3"
aloevera_util = { path = "../util", version = "0.2.2" }
gimp_palette = { version = "0.1.1" }
51 changes: 51 additions & 0 deletions vera/src/palette.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@

//! Vera Palette definition

extern crate gimp_palette;

use crate::{Assemblable, AssembledPrimitive};
use crate::{Error, ErrorKind};
use gimp_palette::{NewPaletteError, Palette};
use std::fmt;

const PALETTE_SIZE: usize = 256;
Expand Down Expand Up @@ -164,6 +167,54 @@ impl VeraPalette {
self.entries.len() * 2
}

/// Derives a palette from the given Gimp gpl file
pub fn derive_from_gpl(
id: &str,
gpl_file: &str,
Copy link
Owner

Choose a reason for hiding this comment

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

For the sake of consistency with other similar functions, it might be better to accept a vec here similar to the derive_from_png function, even if it's a little more fiddly to read a text file as bytes, send it to this function than reinterpret as text. To better support multiple file types (which I think would need to happen as soon as we add a third type), we'd ultimately want this function to be factored out into a trait which accepts data and an enum denoting what kind of input it represents and outputs a palette. So keeping the calls consistent would help toward this.

Copy link
Contributor Author

@CJLove CJLove May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In its current form the gimp_palette crate doesn't support parsing the palette from a string or array of bytes. I was a little torn between using this crate and rolling my own; since the crate hasn't been updated since 2017 the latter is probably a better choice anyway and would allow for the approach you're describing here and below. If I were to roll my own would it be better to do as a separate crate or as a lib project on the same level as the util and library projects?

Copy link
Owner

Choose a reason for hiding this comment

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

It's such a simple file format that I think creating a function to read the bytes directly and return a vec of hex tuples right in the util crate would be a good way to start, then if you think other people would get use out of it, break it out into its own crate later (at least that's the approach I'd take)

config: &VeraPaletteLoadConfig,
) -> Result<Self, Error> {
let gimp_palette = match Palette::read_from_file(&gpl_file) {
Ok(p) => p,
Err(e) => match e {
NewPaletteError::NoColors => {
return Err(ErrorKind::GenericError(format!("No colors loaded")).into())
}
NewPaletteError::InvalidData { line_num, val } => {
return Err(ErrorKind::GenericError(format!(
"Line {} has invalid data: {}",
line_num, val
))
.into())
}
NewPaletteError::IoErr(io_err) => {
return Err(ErrorKind::GenericError(format!("I/O error {}", io_err)).into())
}
},
};
debug!(
"Palette load: Gimp palette {} with {} colors",
gimp_palette.get_name(),
gimp_palette.get_colors().len()
);
let mut palette = match config.include_defaults {
true => VeraPalette::blank_with_defaults(id),
false => VeraPalette::blank(id),
};
for color in gimp_palette.get_colors() {
palette.add_entry(
config.direct_load,
color.r as u8,
color.g as u8,
color.b as u8,
)?;
}
if config.sort {
palette.sort();
}
info!("Palette creation successful");
Ok(palette)
}

/// Derives a palette from the given png image
/// this will fail if the image > 254 distinct RGB or index values
pub fn derive_from_png(
Expand Down
21 changes: 21 additions & 0 deletions vera/tests/data/palette/palette-gimp.gpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
GIMP Palette
Name: TestPalette
Columns: 0
#
0 0 0 Untitled
71 71 71 Untitled
255 255 255 Untitled
34 68 17 Untitled
136 102 17 Untitled
79 16 16 Untitled
79 16 176 Untitled
34 34 34 Untitled
255 255 128 Untitled
144 255 80 Untitled
153 153 153 Untitled
0 0 112 Untitled
69 36 19 Untitled
246 25 25 Untitled
25 246 246 Untitled
233 25 246 Untitled

39 changes: 39 additions & 0 deletions vera/tests/palette.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,42 @@ fn palette_8bpp_indexed() -> Result<(), Error> {

Ok(())
}

#[test]
fn palette_gimp() -> Result<(), Error> {
init_test_logger();
// Unclear why the path needs to be different for this test compared to
Copy link
Owner

Choose a reason for hiding this comment

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

include_bytes interprets the data in a file as a byte array at compilation time, with the path would be relative to the source file. Here you seem to be loading the file during runtime, so the path is relative to the executable. Tests can be run from different directories, so loading files at runtime can lead to more configuration needs. If you change the derive_from_gpl to accept a byte array and do the actual loading outside, this can be made consistent with the rest of the tests.

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 clarification!

// the other tests which use include_bytes().
let test_gpl = "tests/data/palette/palette-gimp.gpl";
let pal_config = VeraPaletteLoadConfig {
direct_load: true,
include_defaults: false,
sort: false,
};
let palette = VeraPalette::derive_from_gpl("pal", &test_gpl, &pal_config)?;
assert_eq!(
palette.value_at_index(0)?,
VeraPaletteEntry { r: 0, g: 0, b: 0 }
);
assert_eq!(
palette.value_at_index(2)?,
VeraPaletteEntry {
r: 15,
g: 15,
b: 15
}
);
assert_eq!(
palette.value_at_index(4)?,
VeraPaletteEntry { r: 8, g: 6, b: 1 }
);
assert_eq!(
palette.value_at_index(10)?,
VeraPaletteEntry { r: 9, g: 9, b: 9 }
);
println!("{}", palette);

assert_eq!(palette.len(), 16);

Ok(())
}