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

IC and TS getattributetype() -- BREAKING CHANGE #3559

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Sep 18, 2022

ImageSpec and ParamValueList have had (since 2.1) a getattributetype() method, which simply returns the TypeDesc of the named attribute, or TypeUnknown if no such attribute exits. This goes along well with the related getattribute() and attribute() family of methods.

The getattribute() method was not present in ImageCache and TextureSystem, despite their having very similar
getattribute()/attribute() interfaces.

A recent issue made me realize that in the Python bindings, the IC and TS bindings for getattribute(name) did not work properly, always returning None, and required you to use the full getattribute(name,type) variety where you have to supply -- and already know -- the type. This isn't especially Pythonic, where you should just be able to ask by name and get a polymorphic result. And when I went to look at how to repair this, it was going to be very awkward without getattributetype(), but trivial with it.

So this patch sets up getattributetype() for both IC and TS, and exposes them to both C++ and Python APIs, and also fully fixes the getattribute() so that it works properly with the name only.

Unfortunately, because getattributename is a new virtual method, it changes the vtable, breaking the ABI, and thus cannot be backported to any supported release that makes ABI compatibility promises.

Now, it is worth noting that OIIO 2.4 is currently in release candidate form, but has not yet been released. It was scheduled for this week. We thus have a last-minute opportunity to squeeze this into 2.4 rather than having this enhancement only in 2.5+, but at the expense of delaying the release by a week or two and making an ABI change later than we would ordinarily have allowed (but still ok to do, no promises are final until we do a "final release").

So I ask interested parties now, should we:

(a) Put this in master only, i.e. for next year's 2.5 release, and go ahead and release 2.4 now as-is (and without any last-minute change, potential instabilities, or ABI changes since it went into beta).

(b) Delay 2.4 final release until Oct 1, have this very minor ABI break versus the beta, and squeeze this into 2.4 so everybody will have it this year.

What say you?

@p12tic
Copy link
Contributor

p12tic commented Sep 20, 2022

I'd vote for (b), but with a new release candidate and potentially more delay to have a chance for revert in case this accidentally breaks something serious.

@lgritz
Copy link
Collaborator Author

lgritz commented Sep 20, 2022

Yes, I would make a new RC with this change, and at least a week for people to try it out. I wouldn't just spring it on the final release.

ImageSpec and ParamValueList have had (since 2.1) a getattributetype()
method, which simply returns the TypeDesc of the named attribute, or
TypeUnknown if no such attribute exits. This goes along well with the
related getattribute() and attribute() family of methods.

The getattribute() method was not present in ImageCache and
TextureSystem, despite their having very similar
getattribute()/attribute() interfaces.

A recent issue made me realize that in the Python bindings, the IC and
TS bindings for `getattribute(name)` did not work properly, always
returning None, and *required* you to use the full
`getattribute(name,type)` variety where you have to supply -- and
already know -- the type. This isn't especially Pythonic, where you
should just be able to ask by name and get a polymorphic result. And
when I went to look at how to repair this, it was going to be very
awkward without getattributetype(), but trivial with it.

So this patch sets up getattributetype() for both IC and TS, and
exposes them to both C++ and Python APIs, and also fully fixes the
getattribute() so that it works properly with the name only.

Unfortunately, because getattributename is a new virtual method, it
changes the vtable, breaking the ABI, and thus cannot be backported to
any supported release that makes ABI compatibility promises.

Now, it is worth noting that OIIO 2.4 is currently in release
candidate form, but has not yet been released. It was scheduled for
this week.  We thus have a last-minute opportunity to squeeze this
into 2.4 rather than having this enhancement only in 2.5+, but at the
expense of delaying the release by a week or two and making an ABI
change later than we would ordinarily have allowed (but still ok to
do, no promises are final until we do a "final release").

So I ask interested parties now, should we:

(a) Put this in master only, i.e. for next year's 2.5 release, and go
    ahead and release 2.4 now as-is (and without any last-minute
    change, potential instabilities, or ABI changes since it went into
    beta).

(b) Delay 2.4 final release until Oct 1, have this very minor ABI
    break versus the beta, and squeeze this into 2.4 so everybody will
    have it this year.

What say you?
@lgritz lgritz merged commit 9b41418 into AcademySoftwareFoundation:master Sep 23, 2022
@lgritz lgritz deleted the lg-getattribtype branch September 23, 2022 05:58
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 23, 2022
ImageSpec and ParamValueList have had (since 2.1) a getattributetype()
method, which simply returns the TypeDesc of the named attribute, or
TypeUnknown if no such attribute exits. This goes along well with the
related getattribute() and attribute() family of methods.

The getattribute() method was not present in ImageCache and
TextureSystem, despite their having very similar
getattribute()/attribute() interfaces.

A recent issue made me realize that in the Python bindings, the IC and
TS bindings for `getattribute(name)` did not work properly, always
returning None, and *required* you to use the full
`getattribute(name,type)` variety where you have to supply -- and
already know -- the type. This isn't especially Pythonic, where you
should just be able to ask by name and get a polymorphic result. And
when I went to look at how to repair this, it was going to be very
awkward without getattributetype(), but trivial with it.

So this patch sets up getattributetype() for both IC and TS, and
exposes them to both C++ and Python APIs, and also fully fixes the
getattribute() so that it works properly with the name only.

Unfortunately, because getattributename is a new virtual method, it
changes the vtable, breaking the ABI, and thus cannot be backported to
any supported release that makes ABI compatibility promises.
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 23, 2022
Changes since RC1:

  - ImageCache/TextureSystem both have added a `getattributetype()`
    method, which retrieves just the type of a named attribute. AcademySoftwareFoundation#3559
    (2.4.4.0)  NOTE: THIS IS AN ABI BREAKING CHANGE
  - Python: Fix the ability to `getattribute()` of int64 and uint64
    metadata or attributes. AcademySoftwareFoundation#3555 (2.4.4.0)
  - Build: Improvements to the generated cmake config files when
    building static libraries. AcademySoftwareFoundation#3552 AcademySoftwareFoundation#3557 (2.4.4.0)
  - Support for gcc 12.1. AcademySoftwareFoundation#3480 (2.4.2.1) AcademySoftwareFoundation#3551 (2.4.4.0)
  - Support building with clang 15.0. AcademySoftwareFoundation#3563 (2.4.4.0)
  - Fix cross-compiling on Android failing due to `-latomic` check. AcademySoftwareFoundation#3560
    (2.4.4.0)
  - Fix building on iOS. AcademySoftwareFoundation#3562 (2.4.4.0)
lgritz added a commit to lgritz/OpenImageIO that referenced this pull request Sep 23, 2022
Changes since RC1:

  - ImageCache/TextureSystem both have added a `getattributetype()`
    method, which retrieves just the type of a named attribute. AcademySoftwareFoundation#3559
    (2.4.4.0)  NOTE: THIS IS AN ABI BREAKING CHANGE
  - Python: Fix the ability to `getattribute()` of int64 and uint64
    metadata or attributes. AcademySoftwareFoundation#3555 (2.4.4.0)
  - Build: Improvements to the generated cmake config files when
    building static libraries. AcademySoftwareFoundation#3552 AcademySoftwareFoundation#3557 (2.4.4.0)
  - Support for gcc 12.1. AcademySoftwareFoundation#3480 (2.4.2.1) AcademySoftwareFoundation#3551 (2.4.4.0)
  - Support building with clang 15.0. AcademySoftwareFoundation#3563 (2.4.4.0)
  - Fix cross-compiling on Android failing due to `-latomic` check. AcademySoftwareFoundation#3560
    (2.4.4.0)
  - Fix building on iOS. AcademySoftwareFoundation#3562 (2.4.4.0)

Note that we bumped the version to from 2.4.3.x 2.4.4.0 to reflect the
ABI break introduced by the last minute by the IC/TS addition of a new
virtual function. ABI will not change again for 2.4.
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.

3 participants