Basis Compression Tool#1290
Conversation
erikwijmans
requested review from
Skylion007,
aclegg3,
bigbike,
eundersander,
jturner65 and
mosra
facebook-github-bot
added
the
CLA Signed
|
@erikwijmans Are we still against making Basis a submodule? Because now we are adding quite a few files. |
|
I don't have a strong preference against a submodule, but that repo does have a lot of extra stuff that we aren't using |
jturner65
left a comment
jturner65
left a comment
There was a problem hiding this comment.
I would think a submodule would be preferable for future support and bug fixes.
|
Thanks for packaging this up and publishing it here, @erikwijmans. Should be very useful for all of us and the community to have an easy way to do this conversion! I agree with other comments that it would be best to avoid pulling all of this code into Habitat-sim if we can find another way (dependency or submodule) to do so. |
|
The download for the basis git repo is ~200 MB, which is 100x larger than taking just what we need (I had forgotten just how big that repo's history is). I pushed the submodule so just an FYI |
|
200mb is not unreasonable for a submodule, and similar to the size of some other submodules we have like Bullet. I vote to keep it as submodule for easy upgrades, bugfixes, etc. |
|
If we are worried about this being cloned a lot even when it's a relatively rarely used submodule, we could make a CMAKE external project: https://cmake.org/cmake/help/git-stage/module/ExternalProject.html |
|
That's a new 3.20 feature and pretty buggy from what I've heard |
|
My vote remains keeping it in as a submodule. |
|
I vote for including just the files, as you did originally, especially since this dependency is not needed for 90% users (unlike Bullet or Assimp). Updating it would mean just replacing the files, I see no problem there (this is how I do it for Homebrew and Arch packages already anyway) -- it's actually more likely that I would need to apply manual patches to get rid of various silly warnings, and a submodule makes that harder. Basis has a "I hate Git"-style maintenance and that they don't care doesn't mean we should be careless too ;) Bundling it as a submodule would also act as a dangerous precedent for other dependencies ("hey, it's fine to bundle this gigabyte dataset, the other submodules are 500 MB already anyway") 😝 |
mosra
left a comment
mosra
left a comment
There was a problem hiding this comment.
About the submodule, see my comment above :)
|
@aclegg3 @jturner65 @Skylion007 -- Mosra made some good points wrt. keeping things as files instead of a submodule. It also doesn't seem like basis is keeping their API very stable (they are changing functions, args, etc) so upgrading likely isn't trivial. |
Yeah, one reason why Magnum is still stuck on the pre-UASTC version of Basis (and backporting various patches to it). The APIs changed significantly since, certain things that worked previously are crashing now and I didn't find the courage to look into the changes yet. |
Is there a way we could access this functionality through Magnum? |
|
What do you mean? The functionality is exposed and used through Magnum's BasisImporter and BasisImageConverter plugins, yes. (But not the glTF repacking part because that waits on the SceneData APIs and a glTF exporter being done, so there's a python script doing that instead.) |
eundersander
left a comment
eundersander
left a comment
There was a problem hiding this comment.
LGTM. I left some minor comments.
I don't have a strong opinion on submodule versus duplicated files. I do wish there was a workflow for "optional submodules". 90% of Hab users will never use this. Similarly, 90% of Hab users will never use our VHACD functionality. I wish a user could just init/clone specific submodules as needed.
|
|
||
| IMAGE_CONVERTER = osp.realpath( | ||
| osp.join( | ||
| osp.dirname(__file__), "..", "build/utils/imageconverter/magnum-imageconverter" |
There was a problem hiding this comment.
In my experience using this script, I have to locally modify here because I don't usually name my build directory "build". It would be nice to offer a friendly error message like "build directory not found" for users like me.
| def convert_image_to_basis(img: str) -> None: | ||
| basis_img = img_name_to_basis(img) | ||
|
|
||
| settings = "threads=4,mip_gen=true,compression_level=2" |
There was a problem hiding this comment.
What's compression_level? Can we document here? Is this an interesting choice for the user?
There was a problem hiding this comment.
Basis doesn't document it with anything more descriptive than "the overall effort the compressor uses on a 1-5 scale". I'll expose it, but there isn't a great rule of thumb for setting it when you can't manually verify if there are compression artifacts (higher may reduce them). I just put its as high as I could while still keeping things to a reasonable amount of time and checked a few meshes for artifacts.
There is a way, but as far as i know, it's not possible for the repo itself to specify that (would be great if it could be said in cd src/deps
git submodule init glog
git submodule init googletest
git submodule init imgui
git submodule init recastnavigation
git submodule init Sophus
git submodule init tinyobjloader
git submodule init tinyxml2
git submodule init v-hacd
git submodule update # clones / fetches only the submodules that were init'd |
|
I switched back to just including the files since upgrading basis is a more involved process than just changing a commit, so we don't get the biggest benefit of a submodule but get all its downsides. |
mosra
left a comment
mosra
left a comment
There was a problem hiding this comment.
The basisu_tool.cpp doesn't need to be included, as that's just the source of their basisu console tool, which the plugin doesn't use in any way. For the record, here's the set of cpp files Magnum uses, I think all other files besides this one are needed.
Besides that, this looks good to me :)
|
Ah darn. I found that source list and I thought I had removed everything that wasn't needed. Thanks for the catch! |
7 participants
Motivation and Context
pip install cmakethat way people can easily get a more recent version of cmake as compiling requires a newer versionTesting
Compressing various datasets!