{"action":"created","issue":{"url":"https://api.github.com/repos/cessen/openexr-rs/issues/8","repository_url":"https://api.github.com/repos/cessen/openexr-rs","labels_url":"https://api.github.com/repos/cessen/openexr-rs/issues/8/labels{/name}","comments_url":"https://api.github.com/repos/cessen/openexr-rs/issues/8/comments","events_url":"https://api.github.com/repos/cessen/openexr-rs/issues/8/events","html_url":"https://github.com/cessen/openexr-rs/issues/8","id":229866870,"number":8,"title":"Figure out a decent first draft API.","user":{"login":"cessen","id":1579058,"avatar_url":"https://avatars0.githubusercontent.com/u/1579058?v=3","gravatar_id":"","url":"https://api.github.com/users/cessen","html_url":"https://github.com/cessen","followers_url":"https://api.github.com/users/cessen/followers","following_url":"https://api.github.com/users/cessen/following{/other_user}","gists_url":"https://api.github.com/users/cessen/gists{/gist_id}","starred_url":"https://api.github.com/users/cessen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cessen/subscriptions","organizations_url":"https://api.github.com/users/cessen/orgs","repos_url":"https://api.github.com/users/cessen/repos","events_url":"https://api.github.com/users/cessen/events{/privacy}","received_events_url":"https://api.github.com/users/cessen/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2017-05-19T04:17:02Z","updated_at":"2017-05-21T00:12:35Z","closed_at":null,"body":"I'd like to take some time to play with API ideas, to see if we can come up with something better than we currently have, and also account for supporting the various features of OpenEXR.\r\n\r\nI don't expect to figure out a final 1.0 API or anything, but rather a first draft that we can implement and use for a while to get more experience for the next iteration.\r\n\r\nSome notes of my own below.\r\n\r\n\r\n## Using the builder pattern, pros and cons\r\n\r\nFor `OutputFile` I'm currently using a builder pattern, so creating a file and writing to it looks something like this:\r\n``` Rust\r\nlet mut fb = {\r\n    // Create the frame buffer ...\r\n};\r\n\r\nOutputFile::new()\r\n    .resolution(256, 256)\r\n    .channel(\"R\", PixelType::FLOAT)\r\n    .channel(\"G\", PixelType::FLOAT)\r\n    .channel(\"B\", PixelType::FLOAT)\r\n    .open(Path::new(\"/path/to/file.exr\"))\r\n    .unwrap();\r\n    .write_pixels(&mut fb).unwrap();\r\n```\r\n\r\nThis seems to make sense, because there are a lot of things that can potentially be specified about the file (but which mostly have obvious defaults), and due to OpenEXR's C++ API they all need to be specified before actually opening the file for writing.  It's also convenient for creating an `OutputFile` inline somewhere.\r\n\r\nHowever, a weakness of this is, for example, that if you have an array of channels already, the builder pattern actually makes it a bit more awkward.  Instead of something like this:\r\n\r\n``` Rust\r\nlet mut out = OutputFile::new().resolution(256, 256);\r\nfor (name, pixel_type) in channel_vec.iter() {\r\n    out.add_channel(name, pixel_type);\r\n}\r\n// ...\r\n```\r\n\r\nYou have to do this:\r\n\r\n``` Rust\r\nlet mut out = OutputFile::new().resolution(256, 256);\r\nfor (name, pixel_type) in channel_vec.iter() {\r\n    out = out.channel(name, pixel_type);\r\n}\r\n// ...\r\n```\r\nOutputFile::new()\r\n    .resolution(256, 256)\r\n\r\nThat isn't awful, but it does IMO read a bit strange with the `out = out.whatever()` in the loop.\r\n\r\n\r\n## Supporting different InputFile and OutputFile types\r\n\r\nOpenEXR supports scanline files, tiled files, multi-part files, and deep files and all the permutations thereof.  For simple input/output this isn't too much of a concern, and I would like to make sure there are simple API's for cases where people just don't care.  But first I want to make sure we wrap all of the lower-level functionality well, and then we can build on top of that later for simpler use-cases.\r\n\r\nFor output, my current idea is to have multiple different `*Writer` types that you can get from `*_open()` calls.  So, for example, if you want a create a tiled file you would do:\r\n\r\n``` Rust\r\nlet mut out: TiledWriter = OutputFile::new()\r\n    .resolution(256, 256)\r\n    // ...\r\n    .open_tiled(Path::new(\"/path/to/file.exr\"));\r\n```\r\n\r\n And for a tiled and multi-part file:\r\n\r\n\r\n``` Rust\r\nlet mut out: TiledMultiPartWriter = OutputFile::new()\r\n    .resolution(256, 256)\r\n    // ...\r\n    .open_tiled_multipart(Path::new(\"/path/to/file.exr\"));\r\n```\r\n\r\nEtc.\r\n\r\nThis would correspond closely to the various `*OutputFile` types in the C++ API, and would allow us to present a tailer-made API for each output type (e.g. writing tiles to a tiled file).\r\n\r\nFor InputFile I'm thinking we could do something similar by returning an `enum` of possible `*Reader` types.\r\n\r\n\r\n## Accessing InputFile's channels\r\n\r\nTo properly read an input file, it should either be verified that the expected channels exist in it and are of the right type, or the channels that do exist should be used to build the frame buffer.  This requires accessing the channel descriptors.\r\n\r\nSo far all I've implemented for that is an iterator API:\r\n\r\n``` Rust\r\nlet input = // Get input file\r\nfor (name, channel) in input.channels() {\r\n    println!(\"{} {:#?}\", name, channel);\r\n}\r\n```\r\n\r\nBut that is unlikely to cut it on its own.  Some kind of direct access would probably be good:\r\n\r\n``` Rust\r\nlet channel_descriptor = input.get_channel(\"R\").unwrap();\r\n```\r\n\r\n\r\n## Custom Attributes\r\n\r\nOpenEXR files support custom attributes to be written to and read from exr files.  I haven't investigated the API closely enough to know exactly how they work (e.g. what types are supported, if it's extensible, etc.).  But at first blush, I think using an API similar to what we have for channels would be good:\r\n\r\n``` Rust\r\nOutputFile::new()\r\n    .resolution(256, 256)\r\n    .attribute(\"attribute_name_1\", /* some way of specifying an attribute value */)\r\n    .attribute(\"attribute_name_2\", /* some way of specifying an attribute value */)\r\n    // ...\r\n\r\nlet input = // Get input file\r\nfor (name, attribute) in input.attributes() {\r\n    println!(\"{} {:#?}\", name, attribute);\r\n}\r\n\r\nlet attribute_value = input.get_attribute(\"attribute_name_1\").unwrap();\r\n```"},"comment":{"url":"https://api.github.com/repos/cessen/openexr-rs/issues/comments/302906648","html_url":"https://github.com/cessen/openexr-rs/issues/8#issuecomment-302906648","issue_url":"https://api.github.com/repos/cessen/openexr-rs/issues/8","id":302906648,"user":{"login":"Ralith","id":104558,"avatar_url":"https://avatars1.githubusercontent.com/u/104558?v=3","gravatar_id":"","url":"https://api.github.com/users/Ralith","html_url":"https://github.com/Ralith","followers_url":"https://api.github.com/users/Ralith/followers","following_url":"https://api.github.com/users/Ralith/following{/other_user}","gists_url":"https://api.github.com/users/Ralith/gists{/gist_id}","starred_url":"https://api.github.com/users/Ralith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Ralith/subscriptions","organizations_url":"https://api.github.com/users/Ralith/orgs","repos_url":"https://api.github.com/users/Ralith/repos","events_url":"https://api.github.com/users/Ralith/events{/privacy}","received_events_url":"https://api.github.com/users/Ralith/received_events","type":"User","site_admin":false},"created_at":"2017-05-21T00:12:35Z","updated_at":"2017-05-21T00:12:35Z","body":"> I was assuming we'd write our own simplifications for that, rather than wrapping theirs, because it's that much less unsafe FFI surface area and should be straightforward.\r\n\r\nThat makes sense to me.\r\n\r\n> The only downside is that you can't do this:\r\n\r\nIt looks to me like the upstream `OutputFile` implementation takes a reference and copies the `Header` rather than consuming it. If we respect that, I think we still get `OutputFile::new(Header::new().set_size(256, 256).add_channel(\"R\". PixelType::FLOAT)...`, which is probably the main ergonomic case of interest.\r\n"}}
{"actor":{"display_login":"Ralith"}}