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

Upgrade Dependencies #212

Merged
merged 3 commits into from
Apr 7, 2019
Merged

Upgrade Dependencies #212

merged 3 commits into from
Apr 7, 2019

Conversation

nuew
Copy link
Contributor

@nuew nuew commented Mar 31, 2019

This PR updates the following outdated dependencies, and fixes the resulting incompatibilities:

  • approx, 0.1.1 -> 0.3
  • base64, 0.6 -> 0.10
  • cgmath, 0.15 -> 0.17
  • image, 0.19 -> 0.21
  • lazy_static, 0.2 -> ^1
  • quote, 0.3 -> 0.6
  • syn, 0.11 -> 0.15

Updating quote requires adding a dependency on proc_macro2 to gltf-derive, but gltf-json already transitively requires that anyways.

Updating image is slightly complicated by their adding of BGR pixel formats to the image::DynamicImage enum. The current PR just adds the new formats to gltf::image::Format, though you could, to preserve backwards compatibility, alternatively not add them and report some sort of error when encountered; or, to ensure forwards compatibility, add a hidden __Nonexhaustive variant (see RFC 2008 to prevent exhaustive matching.

Additionally, this only asks for image features required by the glTF spec (that is, support for PNG and JPEG images), so as to decrease unused code size if someone is not using image except through gltf. images jpeg_rayon parallel JPEG decoding feature is also enabled by default for convenience. Alternatively, of course, we could not do this.

.map(|f| f.ident.as_ref().unwrap())
.map(|ident| {
use inflections::Inflect;
let field = ident.as_ref().to_camel_case();
let field = ident.to_string().to_camel_case();
Copy link
Member

Choose a reason for hiding this comment

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

Why is to_string needed?

Copy link
Contributor Author

@nuew nuew Apr 1, 2019

Choose a reason for hiding this comment

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

Using .as_ref() to get a str was removed in syn 0.13, because proc_macro2 0.4 removed their Ident AsStr impl, as the normal proc_macro crate removed their AsStr impl, the rationale for which is explained here.

Using .to_string() is now the only supported way to get the name of the identifier.


[dependencies.image]
default-features = false
features = ["jpeg", "png_codec"]
Copy link
Member

Choose a reason for hiding this comment

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

This is better.

@alteous
Copy link
Member

alteous commented Apr 1, 2019

Thanks for your PR!

The changes to the image crate are good. Adding #[non_exhaustive] won't be necessary.

The parallel JPEG decoding feature is OK but I would prefer it to be opt-in, that is, not enabled by default.

nuew added 2 commits April 1, 2019 14:48
This causes a minor break in backwards compatibility, due to images
own break, involving the Format enum.

Also, only require features necessary to support standard-required
formats (jpg+png, see specification/2.0/schema/image.schema.json).
A convenience feature to enable parallel JPEG decoding is also added.
@nuew
Copy link
Contributor Author

nuew commented Apr 1, 2019

Parallel JPEG decoding was only enabled by default because it had been in image, though I think all of their features are enabled by default.

I also fixed the alphabetization on the third commit message.

@alteous alteous merged commit acf33a9 into gltf-rs:master Apr 7, 2019
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