[API CHANGE SUGGESION] Make `TextureOpt::missingcolor` a `cspan`

[virtualritz]

I heard most pointers + length would be replaced with spans of some sort going forward.

In that spirit I would like to suggest a (breaking) change to TextureOpt. The title kinda sums it up already. :grin:

The change would be that the length of the missingcolor does not have to match nchannels any more. It would simply be using the last channel value as fill if the cspan is shorter than nchannels or ignore channels at the end, if it is longer.

On that note: the OIIO API has quite afew naming inconistencies with its use of underscores. For example, the set prefix in some some methods is sometimes postfixed with an underscore and sometimes not.

In TextureOpt itself the obvious example is conservative_filter (uses an underscore) vs missingcolor, firstchannel, subimage, subimagename, etc. (don't). It should be conservative_filter, missing_color, first_channel, sub_image, sub_image_name, etc. Or neither should have an underscore.

While this is more changes than just removing the underscore from conservative_filter: as a non-native speaker I very much prefer the underscore version. One of the reasons people struggle with German is that we string arbitray number of nouns together to create new ones. :grin: When you read these it is very difficult for non-native speakers to understand where one noun ends and the next one starts. So if you think this is a silly request, ask someone in your circle of friends who know German as a second language how they feel about that. They can probably reason about it better than I. :wink:

TLDR; if the missingcolor as cspan change happens that may be an opportunity to clean up naming in this struct too.

In the Rust version I btw. also postfixed any s & t settings, i.e. s_blur etc.

[lgritz]

This is a very late request. I don't feel good about changing names of fields at this time. But you make a good point about missingcolor. And looking at the struct, there are other things I'd like to change, like there's no reason to use 4 bytes for the wrap or mip mode fields, etc. I'm not sure why this didn't get an overhaul on the road to 3.0.

I fear it's too late now, we are long overdue to get 3.0 out the door.

But if the TSC thinks this is an important overhaul to squeeze in, I'll try to do it somehow. What do TSC members say? Opinions?

[virtualritz]

No wuckers. I wasn't expecting this to make 3.0 anyway.

I track all renaming suggestions I have in a Google sheet. I figured a while ago that there are too many for any chance to make it into 3.0. :grin: And I haven't shared this sheet as I like to take the time to write a rationale for each.

But I would hope the Rust wrapper could be used drive the discussion as to what/if sth. gets renamed how & why. Some of the suggestions are already reflected there. Not least because there is no method overloading in Rust.

On that note: the wrapper now builds w/o running babble to generate the bindings (bbl-build-rs is still needed though but it will pull that in from GH).

You can have a peek at the docs by doing:

git clone https://github.com/phyrondev/openimageio
cd openimageio
cargo doc --no-deps --open

No need to have a C++ toolchain & OIIO installed at all. Issue weldome if that doesn't work as expected.

[ThiagoIze]

How long do you think we'd need to wait for the next chance to add this in?

For the cspan, that sounds nice, but this would I suspect require users to modify their code to integrate this, which does sound like an imposition at this stage for a small benefit.

For naming cleanups, I think we can survive waiting longer, though I would love to see that later.

If the memory savings can translate into real noticeable gains (memory or time) then I'd feel more greedy about sneaking these in. Plus, I suspect this might be doable in a way that won't require clients to rewrite their code (the compiler should handle the conversion between say a 4B int to a 3-bit bitfield).

[lgritz]

I have a half-way solution I'm working on right now: Some very small changes that are almost completely back-compatible API, but also establishing a versioning system to the struct so we can make bigger changes later (but not for 3.0) without needing to wait 6 years for 4.0.

[virtualritz]

For naming changes in e.g. method names it is also not so bad, you keep the old name, add a deprecation warning and give clients a sensible grace period before the old API bits get really ripped out.

As for the cspan stuff: I'd never consider inconvenience for API consumers if the change is about safety. In case of TextureOpt::missingcolor the fact that the resp. nchannels (that dictates the min allocation of the former) isn't even part of that struct makes it less than a "small benefit" IMHO. :grin:

[lgritz]

Here's what I feel comfortable squeezing in before 3.0. I think the most important thing here is the versioning that will allow us to make further changes without waiting years for 4.0.

https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/4485