-
Notifications
You must be signed in to change notification settings - Fork 596
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
lgritz
merged 2 commits into
AcademySoftwareFoundation:master
from
lgritz:lg-getattribtype
Sep 23, 2022
Merged
IC and TS getattributetype() -- BREAKING CHANGE #3559
lgritz
merged 2 commits into
AcademySoftwareFoundation:master
from
lgritz:lg-getattribtype
Sep 23, 2022
Conversation
This file contains 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
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. |
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?
lgritz
force-pushed
the
lg-getattribtype
branch
from
September 22, 2022 23:51
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.
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?