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

KTX read-only library #324

Merged
merged 7 commits into from
Oct 13, 2020
Merged

KTX read-only library #324

merged 7 commits into from
Oct 13, 2020

Conversation

atteneder
Copy link
Contributor

Added an additional library target ktx_read for users who only want to read KTX files. Especially useful if binary size is a concern.

  • Had to restructure the CMake config a bit to avoid redundancies.
  • Added an option to force creation of static libraries. I needed this to create a .bundle library for Unity macOS.

At the moment this additional lib is built by default. We could make options to en-/disable it (also for the default lib).

Thanks!

Copy link
Collaborator

@MarkCallow MarkCallow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if the additional library was only built when an option is set to build the read-only library. However it should always be built by CI so CI should set the option.

lib/texture1.c Outdated Show resolved Hide resolved
lib/texture2.c Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
lib/texture1.c Outdated Show resolved Hide resolved
lib/texture2.c Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/texture1.c Show resolved Hide resolved
@atteneder
Copy link
Contributor Author

The tools ktx2check and ktxinfo should also work if linking against ktx_read (instead of ktx). I tried and the tests did not break.

@MarkCallow Would you approve that?

@MarkCallow
Copy link
Collaborator

CI should certainly build ktx_read and link ktxinfo and ktx2check against it. However during development I don't want to be building 2 libraries so I'd like the default build to be the ktx with the tools linking against its libktx.

Do you think we should include the read only library in the installers? Even if we do, these 2 tools should be linked against the full libktx to avoid having 2 libraries resident in memory.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Oct 11, 2020

I've changed my mind about not having KTX_FEATURE_STATIC_LIBRARY appearing for iOS and Emscripten. Leave it the way it is now but please add an error should it be set false on these 2 platforms. After that I'll merge this PR.

@atteneder
Copy link
Contributor Author

CI should certainly build ktx_read and link ktxinfo and ktx2check against it. However during development I don't want to be building 2 libraries so I'd like the default build to be the ktx with the tools linking against its libktx.

Do you think we should include the read only library in the installers? Even if we do, these 2 tools should be linked against the full libktx to avoid having 2 libraries resident in memory.

My thought was saving memory by using the smaller ktx_read, but in that case this does not make sense.

…figuration yields error if type is invalid for current platform
@atteneder
Copy link
Contributor Author

Alright, KTX_FEATURE_STATIC_LIBRARY is now always present, but a proper error is shown in case it is misconfigured.

I quickly tested if a read-only WebAssembly variant makes sense, but the current version already offers no write capability (thus it's already very small).

@MarkCallow
Copy link
Collaborator

I quickly tested if a read-only WebAssembly variant makes sense, but the current version already offers no write capability (thus it's already very small).

I do expect to add write capabilities to the WebAssembly build in the future so people can write web tools to encode files. Please check that the Emscripten build disables writing in the same way as ktx_read, i.e. using the same compile options. However do not change the name of the Emscripten library at this time as I do not want to confuse existing users. We'll need to provide warning of the change.

…ad` (instead of `ktx`). Those are read-only and this ensures it.

If creating KTX is required, we'd need to make additional targets and link against `ktx` again.
@atteneder
Copy link
Contributor Author

I now link ktx_js and msc_basis_transcoder_js against ktx_read.

Once write functionality in web is required, we should create additional targets and (optionally) set the KTX_FEATURE_WRITE compile flag on those.

@MarkCallow
Copy link
Collaborator

MarkCallow commented Oct 13, 2020

I cannot find any compile option being set to omit write from the Emscripten build. I think this functionality is missing from the .wasm module just because the wrapper does not have any write functions so they're never linked from libktx. Basically an example of why a static read-only library is not needed. When we make a .wasm module that supports write, at that time we must use the same ktx_read mechanisms in CMakeLists.txt to enable building of a read-only wrapper.

For now I think this PR is good to go.

@MarkCallow MarkCallow merged commit 58c8e98 into KhronosGroup:master Oct 13, 2020
@atteneder atteneder deleted the ktx_read branch October 19, 2020 21:52
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 21, 2024
* feat: ktx_read, an experimental read-only library

* feat: Allow forcing libraries to be static. Useful for flexible integration.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
* feat: ktx_read, an experimental read-only library

* feat: Allow forcing libraries to be static. Useful for flexible integration.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
* feat: ktx_read, an experimental read-only library

* feat: Allow forcing libraries to be static. Useful for flexible integration.
KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024
* feat: ktx_read, an experimental read-only library

* feat: Allow forcing libraries to be static. Useful for flexible integration.
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.

2 participants