IC and TS getattributetype() -- BREAKING CHANGE#3559
Merged
lgritz merged 2 commits intoSep 23, 2022
Conversation
Contributor
|
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. |
Collaborator
Author
|
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. |
EvanAW
reviewed
Sep 22, 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?
178b610 to
c08f90b
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 fullgetattribute(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?