search

castano / nvidia-texture-tools

Texture processing tools with support for Direct3D 10 and 11 formats.
https://github.com/castano/nvidia-texture-tools/wiki
Other
578 stars 216 forks source link

API/ABI changes review for NVTT #259

Open lvc opened 7 years ago

lvc commented 7 years ago

The review of API/ABI changes for NVTT since 2.0.0 version: https://abi-laboratory.pro/tracker/timeline/nvidia-texture-tools/

Created with the help of open-source abi-tracker tool: https://github.com/lvc/abi-tracker. Hope it will be helpful for users and maintainers of the library.

nvtt-1 nvtt-2

Environment

castano commented 7 years ago

This is neat. Is this tracking all exported symbols or only the ones declared in the public headers? In my case the nvtt library exports many private symbols, but applications are only supposed to use the ones declared in the public header 'nvtt.h'.

lvc commented 7 years ago

Hi,

It checks all headers in the source directory (to check ABI in all shared objects) for now.

I've fixed it to check only one header nvtt.h:

nvtt-3 nvtt-4

Thank you.

StefanBruens commented 6 years ago

Just some idea: Lets call 2.1.0/2.1.1 "development snapshots". Then provide a "stable" version 2.2.0, meant for everyone upgrading from 2.0.8 or earlier.

The change in 2.1.1 is just a correction of class added in 2.1.0 (https://github.com/castano/nvidia-texture-tools/commit/78054e977b71cf984c9434d3ad53ba329cd8386b), so no issue for anyone coming from 2.0.8 or earlier.

The new APIs in 2.1.0 are fine for anyone coming from 2.0.8.

Remaining issues: removed API in 2.1.0, namely:

  1. InputOptions::setColorTransform ( enum ColorTransform t )
  2. InputOptions::setLinearTransform ( int channel, float w0, float w1, float w2, float w3 )
  3. InputOptions::setTextureLayout ( enum TextureType type, int width, int height, int depth )

(3) is trivial, as it has been replaced by a new function with an additional (defaulting) parameter: NVTT_API void setTextureLayout(TextureType type, int w, int h, int d = 1, int arraySize = 1); Just readd NVTT_API void setTextureLayout(TextureType type, int w, int h, int d = 1) and make the old function a wrapper to the new function. As InputOptions has no vtable, order of declaration does not matter.

The first two look a little bit more complicated (commit https://github.com/castano/nvidia-texture-tools/commit/1e2567e4a37f4b0b21369c7c5dcf4cb8b616990a#diff-48db6077e2f51b018674558733ee4460), but are trivial as well. setLinearTransform actually never had any effect, so it is fine to replace this one with a stub. setColorTransform only had an effect with ColorTransform_YCoCg. This can be restored.

Biggest issue: new virtual method in OutputHandler::endImage() If an application compiled with the old OutputHandler sets its own outputhandler, two things can happen:

  1. The class adds no virtual methods - a call to endImage() is not backed by any implementation, so we end up with calling (*0)();
  2. The class adds a virtual methods - this ends up in the vtable slot where endImage() is expected. Everytime endImage() is called, a completely different method is executed.

For this issue I see no trivial solution, but to rename the current OutputHandler base class and restore the original one.

castano commented 6 years ago

I was hoping 2.1.0 would be an opportunity to do some breaking changes. My plan was to maintain binary compatibility through the revisions of minor version, but allow changes between minor versions. This may be a little unusual, but I'm reserving the major version changes for major rewrites with little to none source code compatibility. If this makes life too hard for distributions, I'm willing to reconsider. Patches like this that improve compatibility at little cost are certainly welcome.

StefanBruens commented 4 years ago

Its somewhat sad this has not been merged prior to the 2.1.2 release.