api: prototype using optional keyword/value params for ImageBufAlgo

[lgritz]

Many IBA functions take lots of optional parameters, which are from time to time augmented, leaving us with a clutter of semi-deprecated versions.

This PR prototypes a new approach, which is to make IBA functions have explicit parameters for their core/necessary controls, and the minor optional behavioral control parameters are passed as keyword/value argument lists. I think this has a few really big advantages over explicit parameter lists with ever-growing optional parameters:

1. It makes the call sites very readable, since the keywords document what the parameters do.

2. It allows us to add (or deprecate) optional parameters without ABI breaking changes, and without proliferation of many versions of each function.

A new class is added: ParamValueSpan, which as the name implies, is simply a span of ParamValue structs (actually, it's a subclass that inherits from span<const ParamValue>, and adds a bunch of helper methods). Within the IBA namespace, this is aliased to KWArgs, to make a short and self-documenting name to pass this collection of optional parameters. This leads to IBA declarations that look like this:

ImageBuf OIIO_API resize(const ImageBuf &src, KWArgs options = {},
                         ROI roi = {}, int nthreads = 0);

and calls that looks like this:

resize(dst, src, { { "filtername", "lanczos3" },
                   { "filterwidth", 6.0f } });

In this first stab, I have converted IBA::resize(), fit(), and warp(). That seems like enough to evaluate whether we like the direction this is going. If so, we'll tackle more IBA functions over time. I believe I've done this all in a way that preserves API and ABI compatibility for now

A few other odds and ends that come along for the ride:

• Filter1D and Filter2D have a new create_shared static method that returns a shared_ptr instead of raw pointer.

• Make all filter ctrs ok with 0 default filter width value, which means to use the natural default width for that filter.

• IBA::make_kernel is extended so that if you pass 0 for width or hight, it selects the a resolution using the default size for the named filter kernel. This way, the caller doesn't need to know the correct extents of the kernel, just the name.

[nrusch]

My impression is that the ParamValueSpan is really nice for the reasons you mentioned: easy to add/deprecate features (even behind build flags), better self-documentation at call sites, etc.

However, I do want to point out what I see as the main tradeoffs, just to put them out there:

1. Reduced discoverability/increased reliance on documentation

Because of the loss of argument visibility, there's some increased pressure on rigorous comment and documentation maintenance. If that breaks down, the only option is to find and read the OIIO source itself to understand how a function consumes options from the ParamValueSpan.

There are some echoes here of Python's **kwargs syntax for key-value argument passing and all of the readability/maintenance headaches that come along with it. However, this is obviously a very different scenario, and there are some very real benefits that would come along with this, so I don't want to lean too heavily on that comparison.

2. Loss of compile-time argument validation

This one is fairly self-evident.

As a related follow-up question, how do you imagine these changes affecting the Python bindings? I suspect it would be a much bigger deal to lose the individual arguments there, since the binding wrappers have no embedded docstrings to refer to...

[lgritz]

@nrusch To take your last question first: I don't see this changing the Python bindings at all, since Python already has the ability to specify optional arguments out-of-order with keyword identifiers:

// C++ proposed:
dst = resize(src, { { "filtername", "lanczos3" }, { "filterwidth", 6.0f } });

# Python, already:
dst = resize(src, filtername="lanczos3", filterwidth=6.0)

I'm not sure I see a need to alter the Python side. If anything, this moves the C++ side to appear a bit more like the Python in some nice ways (modulo some limitations of C++ syntax).

I do agree with your warning about how we're losing some compile-time checking. On the other hand, if there were previously several different optional params that take a float, it was already easy to screw up and pass the wrong value for the wrong one, without it being at all apparent at the call site and without any help from the compiler. So we're losing some compile-time checking, but at the same time we are gaining a different kind of checking and self-documentation and on the whole perhaps we are coming out ahead?

As for the discoverability, it's not discoverable from the raw function declaration, but I'm trying to be extra attentive to making the comments at each declaration be a really detailed description of the possible parameters (see the code in this PR for an example), and those get automatically incorporated into the web docs as well. So anybody who looks at the header or the docs, I'm confident, will get plenty of instruction about the options available.

What you didn't mention, and I am mildly concerned about, is simply that this is an unusual idiom to use in C++, so NEW people to OIIO might find it takes a little getting used to. It's not hard to get the hang of, but it's different. That isn't ideal, but again, it solves some other problems, especially with ABI stability, that I think make it worth it in the long run.

[nrusch]

To take your last question first: I don't see this changing the Python bindings at all [...]

Great! I just wasn't sure if you had additional ideas around refactoring those as well.

So we're losing some compile-time checking, but at the same time we are gaining a different kind of checking and self-documentation and on the whole perhaps we are coming out ahead?

Makes sense to me. I think that sounds like a reasonable tradeoff given your point about the potential to mix up multiple similarly-typed arguments.

[lgritz]

After thinking about this more today, I'll also add that this is similar in spirit to how many of our major classes have some kind of attribute() call that sets properties or behaviors by name + value. That has worked really well for us over the years, with several similar benefits and deficiences as described above for the present PR:

• Prevents proliferation of separate calls needing to be added every time we think of a new property people might want to set.
• Can add, change, or remove those named properties without public ABI breaks.
• As noted, this makes the possible properties "not very discoverable" by examining the function call prototypes, but we try to make up for it with extensive (and I hope clear) documentation.

So I guess there is a long-established precedent within OIIO APIs of controls using a keyword/value approach. We've used this on the major classes (ImageCache, TextureSystem, global OIIO behaviors via OIIO::attribute(), as well as setting metadata in an ImageSpec this way) for a long time, now we're extending the basic idea to IBA functions. It seems like a less exotic idiom when you draw this analogy.

[lgritz]

The reaction at last week's TSC meeting to this was positive.

Would anybody (especially from the TSC) care to give this PR an unequivocal approve, or express any remaining doubts, or make suggestions to improve the idioms I've chosen?

If approved, I'd be in favor of merging this and looking for other IBA functions to simplify in an analogous way.

[lgritz]

@sfriedmapixar on the PR for the earlier stab at this, you expressed preference for the cspan approach. Does this look roughly like you were assuming?

[ThiagoIze]

Here are a few comments for the small amount of the code that I have looked at. Some of my suggestions I'm not too invested in, so please feel free to ignore them if you think it better to do so.

[ThiagoIze]

comment says "case INsensitive search" but code says bool casesensitive = true. Does the comment need adjusting to say it does a search with optional case insensitivity? Same applies to the other functions below.

[lgritz]

Which way do you think is better in this case? I think for ImageSpec metadata, we search case-insensitively. I don't know if that's the "right" choice. But since it would be so confusing and unwise to have two separate parameters that differed only in case, I don't see an advantage to encouraging it. On the other hand, case-sensitive string compares are a lot faster. Opinions?

[ThiagoIze]

I agree with your points, but I don't have a strong preference for what to default to. My main concern is that the comment and the code are currently opposite.
If I do have to pick something, I would probably default to case insensitive so it gives the results users likely expect. If anyone really needs the speed, they can still request the case sensitive fast path.

[lgritz]

Yes, pushed an update where all the ParamValueSpan searches are case insensitive by default and not contradicted by the comments.

[lgritz]

A variety of improvements based on this feedback have been incorporated into an update to this PR.

[ThiagoIze]

This example code could be simplified now that there's a special case for a single pointer. Worth doing?

[lgritz]

yes, fixing

[ThiagoIze]

give simpler way for single item? make_pv?

[lgritz]

yes, done and pushed

[ThiagoIze]

I agree with your points, but I don't have a strong preference for what to default to. My main concern is that the comment and the code are currently opposite.
If I do have to pick something, I would probably default to case insensitive so it gives the results users likely expect. If anyone really needs the speed, they can still request the case sensitive fast path.

[ThiagoIze]

This looks good to me.