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

PVRTexToolCLI #319

Closed
wants to merge 3 commits into from
Closed

PVRTexToolCLI #319

wants to merge 3 commits into from

Conversation

eugeneloza
Copy link
Member

@eugeneloza eugeneloza commented Jul 14, 2021

Use new version of PVRTexToolCLI (PVRTexTool v5.1.0) in Castle Game Engine.

Tested and working. There are some glitches with loading the generated textures, but they may be specific to ASTC_4x4 format, as not all textures are affected. yes, investigating more - it's ASTC issue, which doesn't have relation to this PR, however, maybe we'll need to address it from our side so that users will be able to use it without hassle.

We still need to comply with:

If object code is bundled with a product, all branding should be kept as it was originally, and the following acknowledgement should be displayed clearly in any associated documentation or other collateral in printed or electronic form distributed with the product incorporating the Software: “This product includes components of the PowerVR Tools Software from Imagination Technologies Limited”

I believe "associated documentation" here can simply mean putting this notice at https://castle-engine.io/creating_data_auto_generated_textures.php is enough.

@eugeneloza
Copy link
Member Author

Hmm... I wonder if there is any need in this PR anymore? PVRTexToolCLI may be useful, but it's glitchy for ASTC format. And the version we're currently using in Docker image seems enough for all its uses.

I believe we should use ASTCENC as in #321 - also FOSS compatible, included in Debian repository - it seems to work perfectly for our usecase and without any glitches of PVRTexToolCLI and NvidiaTextTools.

@michaliskambi
Copy link
Member

I agree with your last comment @eugeneloza . I mean I still want this PR, because I want this "infrastructure" -- I believe we should package some texture tools with CGE, as we want https://castle-engine.io/creating_data_auto_generated_textures.php to work more "out of the box" (whether someone uses our Docker or not).

But the PVRTexToolCLI may indeed be the wrong candidate for this integration. As it's not critical, and not open-source.

I say to remake this PR to include open-source texture compression tools, that we can distribute without hassle, so

  1. ASTCENC (license: Apache 2.0, https://github.com/ARM-software/astc-encoder )
  2. nvcompress (license: MIT)
  3. AMD Compressonator (license: combination of MIT, BSD-3, other -- https://github.com/GPUOpen-Tools/compressonator/tree/master/license )

From what I see, they are all open-source and thus we could "bundle" them without getting entangled in any additional legalese. The licenses listed above (MIT, BSD-3, Apache 2.0) are very permissive.

I would say to go with nvcompress for the start. It creates S3TC compressed textures, thus is most useful for desktop apps. This will allow to see the benefits of the system "tools are bundled in CGE" quickly.

Copy link
Member

@michaliskambi michaliskambi left a comment

Choose a reason for hiding this comment

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

See my above comments. In short, I'd say we should bundle with "nvcompress" for start.

@eugeneloza eugeneloza mentioned this pull request Dec 11, 2021
@eugeneloza
Copy link
Member Author

I've implemented the changes from this PR into #351 with PVRTexToolCLI binaries never touching CGE commit history - I believe we may close this PR.

@michaliskambi
Copy link
Member

I've implemented the changes from this PR into #351 with PVRTexToolCLI binaries never touching CGE commit history - I believe we may close this PR.

Thank you, and thanks for thinking about this important detail ("PVRTexToolCLI binaries never touching CGE commit history", good point). Closing this PR then, I merged #351 already.

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.

None yet

2 participants