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

Game Lab: S3-backed animation library #10685

Merged
merged 22 commits into from Sep 21, 2016

Conversation

Projects
None yet
4 participants
@islemaster
Member

islemaster commented Sep 16, 2016

Instead of our hard-coded library of eight animations, dedicate an S3 bucket to the animation library and use a generated library manifest to search and import those animations.

See the Animation Library Tech Spec (requires login).

This PR is a first iteration. I'd like to get the basic details in here, and return to address a number of TODOs and usability items in the future. Eventually it should be easy for any engineer or PM to add a set of animations to the library.

How to add an animation to the library

  1. Generate a metadata file for your animation.

    For example, bunny1_walk2.png needs an associated bunny1_walk2.json that looks like this:

    {
     "name": "bunny1_walk2",
     "aliases": ["jumper"],
     "frameSize": {"x":120, "y": 207},
     "frameCount": 1,
     "looping": false,
     "frameDelay": 2
    }

    These metadata files can be created by hand, or we can automate their creation with better tools. I'm currently working on a script to help generate these for one-frame images.

  2. Upload the animation and metadata to the cdo-animation-library S3 bucket.

    You'll need the appropriate AWS permissions to do this. In the future we might build a nice frontend for levelbuilders to do this; for now just use the S3 web interface.

  3. Run [cdo]/tools/scripts/rebuildAnimationLibraryManifest.rb

    Again, you'll need appropriate AWS permissions for this, this time in your locals.yml. This will regenerate [cdo]/apps/src/gamelab/animationLibrary.json - more on that process later.

  4. Commit the new animationLibrary.json and merge it!

    Changes to the manifest (and consequently, the animations that get loaded by the library) have nice diffs, and go through our full test/deploy pipeline.

The script

Here's normal script output right now. It's slowish, even with only 16 animations in the library - I have some thoughts on speeding it up, commented inline.

out

There's also --verbose output in case you really want to see everything it's doing.

What's in the manifest

In addition to concatenating all of the animation metadata JSON files, the manifest:

  • Adds the following to the supplied metadata:
    • The source image dimensions
    • The HEAD S3 version key of the source image at the time the manifest is generated
    • A sourceUrl for the source image pointing to the new animation library API
  • Generates a keyword => result map for searching (may be replaced with a search tree later)

There's no reason we can't extend or change the manifest format, either - since it's checked in with source code, we just have to change the manifest (and the generator script) at the same time we change the code that reads it.

The new animation library API

The cdo-animation-library bucket isn't public, so we provide a route /api/v1/animation-library/<version>/<filename> for read-only access to the library assets (and hopefully so our varnish layer will cache them). It's intentionally designed to require you to know the version key of the asset you are looking for, because the manifest will always provide one.

Library animations are always versioned so that we have freedom to edit and remove library animations in the future. We make a couple of important guarantees:

  • Normal changes to the S3 bucket don't affect production (or any environment) until that environment gets an updated manifest referencing the new asset versions. We can make changes to the library and they are effectively only 'seen' by each environment as the updated manifest reaches that environment - even in the case of deleted animations.
  • Projects keep referencing whichever version of a library asset they originally imported, even if we edit or remove that asset and the new manifest hits production. This works because a project clones the asset's sourceUrl (which includes its version) on import, so it will always point to that version regardless of what the 'latest' library contains.

Updates

islemaster added some commits Sep 15, 2016

Initial attempt at animation library manifest script
Also includes initial animation library manifest file.
Add /api/v1/animation-library/... endpoint
Provide limited access to contents of the cdo-animation-library bucket, so that we require knowledge of the particular version ID to retrieve the asset.
Add sourceSize to manifest
Make manifest generation script read source size for images and include it in the manifest.
Animation library loads manifest, is searchable
Instead of loading the static animationLibrary.js file, the animation library dialog now loads animationLibrary.json, uses its alias mapping to be searchable, loads animationProps from its metadata map, and consequently fetches library images from the new animation library endpoint (and by proxy S3) instead of from dashboard/public/blockly.
Add bunny1 to animation library
Adds bunny1 from the Kenney.nl "Jumper" pack to the animation library.
import ScrollableList from '../AnimationTab/ScrollableList.jsx';
import styles from './styles';
import AnimationPickerListItem from './AnimationPickerListItem.jsx';
import AnimationPickerSearchBar from './AnimationPickerSearchBar.jsx';
const MAX_SEARCH_RESULTS = 18;

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

We are limiting the number of results to a query to avoid triggering a ton of asset loading while-you-type. Long-term I'd like to add pagination to search results so you can keep stepping through animations if you'd like.

@islemaster

islemaster Sep 16, 2016

Member

We are limiting the number of results to a query to avoid triggering a ton of asset loading while-you-type. Long-term I'd like to add pagination to search results so you can keep stepping through animations if you'd like.

.filter(alias => searchRegExp.test(alias))
.reduce((resultSet, nextAlias) => {
return resultSet.union(animationLibrary.aliases[nextAlias]);
}, Immutable.Set());

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

I'm trying out Immutable.JS Set here, so I don't have to think about removing duplicates. Not really sure if there's a performance benefit.

@islemaster

islemaster Sep 16, 2016

Member

I'm trying out Immutable.JS Set here, so I don't have to think about removing duplicates. Not really sure if there's a performance benefit.

}
}
},
"aliases": {

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

Here's the alias map that we use for searching - we search the keys, and the results are all the values of all the matched keys.

Once the library gets large, we might benefit from generating a Trie instead, at least to two or three characters deep.

@islemaster

islemaster Sep 16, 2016

Member

Here's the alias map that we use for searching - we search the keys, and the results are all the values of all the matched keys.

Once the library gets large, we might benefit from generating a Trie instead, at least to two or three characters deep.

// Example: searchQuery "bar"
// Will match: "barbell", "foo-bar", "foo_bar" or "foo bar"
// Will not match: "foobar", "ubar"
const searchRegExp = new RegExp('(?:\\b|_)' + searchQuery, 'i');

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

TODO: I suppose I should escape searchQuery somehow? I mean, it's pretty cool to support regex in our search, but probably not helpful to students.

@islemaster

islemaster Sep 16, 2016

Member

TODO: I suppose I should escape searchQuery somehow? I mean, it's pretty cool to support regex in our search, but probably not helpful to students.

rescue
not_found
end
end

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

For now this API is really simple, and totally public, so it shouldn't need any special Varnish configuration. If we add routes that write to S3 in the future (say, for a nice levelbuilder UI to the library) we may need to do something like #8113 and whitelist cookies on those routes so we can check permissions.

@islemaster

islemaster Sep 16, 2016

Member

For now this API is really simple, and totally public, so it shouldn't need any special Varnish configuration. If we add routes that write to S3 in the future (say, for a nice levelbuilder UI to the library) we may need to do something like #8113 and whitelist cookies on those routes so we can check permissions.

# https://docs.google.com/document/d/18-LVuvKd0jKTUiGo5GYReUWM5oFWCyKRyEQURJ5HCOM/edit
#
# TODO: Optimize: Read existing manifest and don't do full object reads for
# entries whose modify date hasn't changed.

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

There are lots of TODOs in this script, and I think many of them can wait until after this PR. I've tried to make the script user-friendly (it has a --help flag!) but writing shell scripts is an art and I don't have a lot of practice. Feedback welcome!

@islemaster

islemaster Sep 16, 2016

Member

There are lots of TODOs in this script, and I think many of them can wait until after this PR. I've tried to make the script user-friendly (it has a --help flag!) but writing shell scripts is an art and I don't have a lot of practice. Feedback welcome!

This comment has been minimized.

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Don't underrate yourself, I learned quite a bit reading through this script. :)

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Don't underrate yourself, I learned quite a bit reading through this script. :)

animations_by_name[animation_name][extension] = object_summary
end
# TODO: Validate that keys are unique

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

There's a good number of validation TODOs in here that I'd like to tackle in a separate PR as prep for setting @poorvasingal loose with this script and the Kenney.nl assets.

@islemaster

islemaster Sep 16, 2016

Member

There's a good number of validation TODOs in here that I'd like to tackle in a separate PR as prep for setting @poorvasingal loose with this script and the Kenney.nl assets.

# Populate sourceSize if not already present
unless metadata.key?('sourceSize')
png_body = objects['png'].object.get.body.read
metadata['sourceSize'] = dimensions_from_png(png_body)

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

This part of the script can get slow - getting the object versions is a HEAD Object operation, I think, which is bad enough, but to read the source sizes we actually have to download the PNGs themselves. While I don't think this traffic is going to hurt us for an occasional script run, I'd like to do a little optimization and use the existing manifest to skip any images that haven't changed.

@islemaster

islemaster Sep 16, 2016

Member

This part of the script can get slow - getting the object versions is a HEAD Object operation, I think, which is bad enough, but to read the source sizes we actually have to download the PNGs themselves. While I don't think this traffic is going to hurt us for an occasional script run, I'd like to do a little optimization and use the existing manifest to skip any images that haven't changed.

EOS
# Report any issues while talking to S3 and suggest most likely steps for fixing it.
rescue Aws::Errors::ServiceError => service_error

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

This catches any S3-related error, but the most common cause will be missing credentials so I make sure to print a hint about that if it occurs.

@islemaster

islemaster Sep 16, 2016

Member

This catches any S3-related error, but the most common cause will be missing credentials so I make sure to print a hint about that if it occurs.

# IHDR Chunk Layout
# http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.IHDR
dimensions = png_body[0x10..0x18].unpack('NN')
{'x': dimensions[0], 'y': dimensions[1]}

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

There are whole libraries dedicated to this... but it turns out PNG is a really easy format to read directly, especially in Ruby.

@islemaster

islemaster Sep 16, 2016

Member

There are whole libraries dedicated to this... but it turns out PNG is a really easy format to read directly, especially in Ruby.

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Note for (way) future reference, may want to eventually run assets through pngquant (though those transformations could do with a visual inspection as sometimes it doesn't work with all images and there end up being small artifacts—may be better to just do as periodic batch one-off operations down the road)

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Note for (way) future reference, may want to eventually run assets through pngquant (though those transformations could do with a visual inspection as sometimes it doesn't work with all images and there end up being small artifacts—may be better to just do as periodic batch one-off operations down the road)

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

👍 That's a good idea - maybe I'll integrate that into the batch script I'm going to write next for importing Kenney libraries.

@islemaster

islemaster Sep 21, 2016

Member

👍 That's a good idea - maybe I'll integrate that into the batch script I'm going to write next for importing Kenney libraries.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Sep 16, 2016

Member

Circle failures look legitimate, will investigate tomorrow.

Member

islemaster commented Sep 16, 2016

Circle failures look legitimate, will investigate tomorrow.

@caleybrock

This is so cool - I can't wait to get more animations in!! Students are going to make so many awesome things with this. 👍 🎉 🏫 💯 💃

render() {
var pageOfResults = searchAnimations(this.state.searchQuery);

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

why do we do this in the render function and not componentWillRecieveProps?

@caleybrock

caleybrock Sep 16, 2016

Collaborator

why do we do this in the render function and not componentWillRecieveProps?

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

In this case, we are actually modifying the component's local state as the user types in the search box, which will not cause componentWillReceiveProps to occur. Conversely, we could call this from onSearchQueryChange but that wouldn't handle the initial render case so we'd need to do it in componentWillMount too... in the end, I think it's best to trust React to only call render() when it's needed (it's usually good at that) and optimize later if it becomes an issue. If you look at this component's props there's really nothing other than the search query that should trigger a re-render, so I suspect this will be fine.

@islemaster

islemaster Sep 16, 2016

Member

In this case, we are actually modifying the component's local state as the user types in the search box, which will not cause componentWillReceiveProps to occur. Conversely, we could call this from onSearchQueryChange but that wouldn't handle the initial render case so we'd need to do it in componentWillMount too... in the end, I think it's best to trust React to only call render() when it's needed (it's usually good at that) and optimize later if it becomes an issue. If you look at this component's props there's really nothing other than the search query that should trigger a re-render, so I suspect this will be fine.

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

coolio thanks for explaining!

@caleybrock

caleybrock Sep 16, 2016

Collaborator

coolio thanks for explaining!

// Match any word boundary or underscore followed by the search query.
// Example: searchQuery "bar"
// Will match: "barbell", "foo-bar", "foo_bar" or "foo bar"
// Will not match: "foobar", "ubar"

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

This will be interesting to test with the animation names we will use to see if we need a different type of search. It'll also be interesting to test if kids have ideas of what to search or if we'll need to add promps or suggestions. (I just read the spec said 1,000 - 10,000 images which is a ton)

@caleybrock

caleybrock Sep 16, 2016

Collaborator

This will be interesting to test with the animation names we will use to see if we need a different type of search. It'll also be interesting to test if kids have ideas of what to search or if we'll need to add promps or suggestions. (I just read the spec said 1,000 - 10,000 images which is a ton)

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

The 1000-10000 was my own ballpark, I'm not sure what the content team has in mind. I think kids will know what to search for but there is a discoverability concern - I was actually wondering how we should special-case the results when there is no search query. Right now we always display the first n animations (alphabetical) but I'm wondering if we should instead pick a random set, or highlight the newest content.

@islemaster

islemaster Sep 16, 2016

Member

The 1000-10000 was my own ballpark, I'm not sure what the content team has in mind. I think kids will know what to search for but there is a discoverability concern - I was actually wondering how we should special-case the results when there is no search query. Right now we always display the first n animations (alphabetical) but I'm wondering if we should instead pick a random set, or highlight the newest content.

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

I think this is what Caley was getting at but just in case not — it would be awesome to somehow save out failed searches (once user backtracks when no results?) to see what graphics students wish we had (& potential mis-searches). I know of at least one coding website that has done that as a way to source graphic additions and kids loved it (really makes it feel like the website offers something for everything).

@bcjordan

bcjordan Sep 21, 2016

Collaborator

I think this is what Caley was getting at but just in case not — it would be awesome to somehow save out failed searches (once user backtracks when no results?) to see what graphics students wish we had (& potential mis-searches). I know of at least one coding website that has done that as a way to source graphic additions and kids loved it (really makes it feel like the website offers something for everything).

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

Wow! That's an awesome idea, but maybe another pull request. We're doing similar logging when we strip unexpected markup from applab design mode HTML to see what we might be mis-categorizing. I'll take a look at this.

@islemaster

islemaster Sep 21, 2016

Member

Wow! That's an awesome idea, but maybe another pull request. We're doing similar logging when we strip unexpected markup from applab design mode HTML to see what we might be mis-categorizing. I'll take a look at this.

@@ -1,75 +0,0 @@
/** @file Metadata for stock animation library animations */

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

yay deleted!

@caleybrock

caleybrock Sep 16, 2016

Collaborator

yay deleted!

"asterisk_circle": {
"name": "asterisk_circle",
"aliases": [
"p5.play"

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

I really like that we have aliasing, because some of these names might be hard to match in a search. (ex - I can see a student searching character or something very general)

@caleybrock

caleybrock Sep 16, 2016

Collaborator

I really like that we have aliasing, because some of these names might be hard to match in a search. (ex - I can see a student searching character or something very general)

# Actually download the JSON from S3
json_response = objects['json'].get
metadata = JSON.parse(json_response.body.read)
# TODO: Validate metadata, ensure it has everything it needs

This comment has been minimized.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

It'd be nice to print out the animation names that don't have properly formatted metadata (once you validate) so we don't end up with missing animations.

@caleybrock

caleybrock Sep 16, 2016

Collaborator

It'd be nice to print out the animation names that don't have properly formatted metadata (once you validate) so we don't end up with missing animations.

This comment has been minimized.

@islemaster

islemaster Sep 16, 2016

Member

I was thinking that too. When I do a usability pass on this script I want it to leave any invalid animations out of the manifest (so we're not blocked on generating a valid manifest) but also write out warnings for each animation we skipped with useful reasons.

@islemaster

islemaster Sep 16, 2016

Member

I was thinking that too. When I do a usability pass on this script I want it to leave any invalid animations out of the manifest (so we're not blocked on generating a valid manifest) but also write out warnings for each animation we skipped with useful reasons.

islemaster added some commits Sep 16, 2016

Parallelize metadata generation
This provides a significant speedup because requests to S3 for object versions and image dimensions get parallelized.  The effect should be more obvious once there's more content on S3.
@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Sep 16, 2016

Member

Adding @ashercodeorg for API and general Ruby feedback - not my strong suits.

Member

islemaster commented Sep 16, 2016

Adding @ashercodeorg for API and general Ruby feedback - not my strong suits.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Sep 20, 2016

Member

bump @bcjordan @ashercodeorg Please review when you can. Thanks!

Member

islemaster commented Sep 20, 2016

bump @bcjordan @ashercodeorg Please review when you can. Thanks!

@ashercodeorg

LGTM for ruby changes.

Show outdated Hide outdated lib/cdo/cdo_cli.rb Outdated
Show outdated Hide outdated shared/middleware/animation_library_api.rb Outdated
Show outdated Hide outdated tools/scripts/rebuildAnimationLibraryManifest.rb Outdated
Show outdated Hide outdated tools/scripts/rebuildAnimationLibraryManifest.rb Outdated
# https://docs.google.com/document/d/18-LVuvKd0jKTUiGo5GYReUWM5oFWCyKRyEQURJ5HCOM/edit
#
# TODO: Optimize: Read existing manifest and don't do full object reads for
# entries whose modify date hasn't changed.

This comment has been minimized.

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Don't underrate yourself, I learned quite a bit reading through this script. :)

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Don't underrate yourself, I learned quite a bit reading through this script. :)

cli_parser = OptionParser.new do |opts|
opts.banner = "Usage: ./rebuildAnimationLibraryManifest.rb [options]"
opts.separator ""
opts.separator "Options:"

This comment has been minimized.

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Mind explaining what these consecutive calls do? The documentation isn't helpful.

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

Mind explaining what these consecutive calls do? The documentation isn't helpful.

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

Lifted from bin/dotd so here's my best understanding: One of OptionParser's features is to generate an "option summary" (read: help text) from the options you define in the order you define them. The separator method lets you inject text into that summary, wherever you call it relative to where the options are defined. Here we are inserting a blank line and an "Options:" line. The result looks like this:

╰○ ./tools/scripts/rebuildAnimationLibraryManifest.rb --help
Usage: ./rebuildAnimationLibraryManifest.rb [options]

Options:
    -q, --quiet                      Only log warnings and errors
    -v, --verbose                    Use verbose log output
    -h, --help                       Show this message

@islemaster

islemaster Sep 21, 2016

Member

Lifted from bin/dotd so here's my best understanding: One of OptionParser's features is to generate an "option summary" (read: help text) from the options you define in the order you define them. The separator method lets you inject text into that summary, wherever you call it relative to where the options are defined. Here we are inserting a blank line and an "Options:" line. The result looks like this:

╰○ ./tools/scripts/rebuildAnimationLibraryManifest.rb --help
Usage: ./rebuildAnimationLibraryManifest.rb [options]

Options:
    -q, --quiet                      Only log warnings and errors
    -v, --verbose                    Use verbose log output
    -h, --help                       Show this message

@bcjordan

LGTM! Esp like the extraction of CLI utils. This is going to be very cool.

render() {
var pageOfResults = searchAnimations(this.state.searchQuery);

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Checking my understanding — page here just means the result set is limited, not that there's pagination involved?

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Checking my understanding — page here just means the result set is limited, not that there's pagination involved?

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

Yes. It was optimistic naming on my part, since I want pagination in the future and could picture searchAnimations taking a 'start at index' argument... but I can rename if this is confusing.

@islemaster

islemaster Sep 21, 2016

Member

Yes. It was optimistic naming on my part, since I want pagination in the future and could picture searchAnimations taking a 'start at index' argument... but I can rename if this is confusing.

Show outdated Hide outdated apps/src/gamelab/AnimationPicker/AnimationPickerBody.jsx Outdated
// Match any word boundary or underscore followed by the search query.
// Example: searchQuery "bar"
// Will match: "barbell", "foo-bar", "foo_bar" or "foo bar"
// Will not match: "foobar", "ubar"

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

I think this is what Caley was getting at but just in case not — it would be awesome to somehow save out failed searches (once user backtracks when no results?) to see what graphics students wish we had (& potential mis-searches). I know of at least one coding website that has done that as a way to source graphic additions and kids loved it (really makes it feel like the website offers something for everything).

@bcjordan

bcjordan Sep 21, 2016

Collaborator

I think this is what Caley was getting at but just in case not — it would be awesome to somehow save out failed searches (once user backtracks when no results?) to see what graphics students wish we had (& potential mis-searches). I know of at least one coding website that has done that as a way to source graphic additions and kids loved it (really makes it feel like the website offers something for everything).

Show outdated Hide outdated apps/src/gamelab/AnimationPicker/AnimationPickerBody.jsx Outdated
.sort()
.slice(0, MAX_SEARCH_RESULTS)
.map(result => animationLibrary.metadata[result])
.toArray();

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

image

@bcjordan

bcjordan Sep 21, 2016

Collaborator

image

end
end
require_relative '../lib/cdo/cdo_cli'
include CdoCli

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Nice!

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Nice!

This comment has been minimized.

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

At some point, I thought there was a wish to make this script not depend on the rest of our codebase. This refactor violates this wish. Do we care?

@ashercodeorg

ashercodeorg Sep 21, 2016

Contributor

At some point, I thought there was a wish to make this script not depend on the rest of our codebase. This refactor violates this wish. Do we care?

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

My understanding was that there were two motivations for that decision.

  1. To not depend on loading our whole environment, which is slow.

    This change doesn't violate that principle, since CdoCli is itself very slim. I'll add a comment in that file suggesting we keep it that way though!

  2. To allow one to copy the dotd script anywhere and still run it.

    I used to do this and have found that I don't anymore (it doesn't really matter once it's running). I will take a quick Slack poll and see if anyone else is doing this.

@islemaster

islemaster Sep 21, 2016

Member

My understanding was that there were two motivations for that decision.

  1. To not depend on loading our whole environment, which is slow.

    This change doesn't violate that principle, since CdoCli is itself very slim. I'll add a comment in that file suggesting we keep it that way though!

  2. To allow one to copy the dotd script anywhere and still run it.

    I used to do this and have found that I don't anymore (it doesn't really matter once it's running). I will take a quick Slack poll and see if anyone else is doing this.

This comment has been minimized.

@islemaster

islemaster Sep 21, 2016

Member

Seven results (ignore the first in each category) say this is fine to change.

image

@islemaster

islemaster Sep 21, 2016

Member

Seven results (ignore the first in each category) say this is fine to change.

image

info <<-EOS.unindent
Manifest written to #{DEFAULT_OUTPUT_FILE}.
#{dim 'd[ o_0 ]b'}

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

👍 👍 👍 👍

@bcjordan

bcjordan Sep 21, 2016

Collaborator

👍 👍 👍 👍

# Report any issues while talking to S3 and suggest most likely steps for fixing it.
rescue Aws::Errors::ServiceError => service_error
warn service_error.inspect
warn <<-EOS.unindent

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Neat!

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Neat!

# IHDR Chunk Layout
# http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.IHDR
dimensions = png_body[0x10..0x18].unpack('NN')
{'x': dimensions[0], 'y': dimensions[1]}

This comment has been minimized.

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Note for (way) future reference, may want to eventually run assets through pngquant (though those transformations could do with a visual inspection as sometimes it doesn't work with all images and there end up being small artifacts—may be better to just do as periodic batch one-off operations down the road)

@bcjordan

bcjordan Sep 21, 2016

Collaborator

Note for (way) future reference, may want to eventually run assets through pngquant (though those transformations could do with a visual inspection as sometimes it doesn't work with all images and there end up being small artifacts—may be better to just do as periodic batch one-off operations down the road)

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Sep 21, 2016

Member

Awesome, thanks for the detailed reviews everyone! I'll clean this up and let you know if anything needs another look.

Member

islemaster commented Sep 21, 2016

Awesome, thanks for the detailed reviews everyone! I'll clean this up and let you know if anything needs another look.

@islemaster islemaster merged commit fa0aad3 into staging Sep 21, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!

@islemaster islemaster deleted the animation-library branch Sep 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment