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

Assorted fixes for more robust handling of asset corner cases #1915

Merged
merged 4 commits into from
Oct 26, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Oct 26, 2022

Motivation and Context

  • Enables shader texture transform only if it's actually textured. If it's not (for example if a texture failed to load or if the material data are strange in some way), then the shader construction would assert with

    Shaders::PhongGL: texture transformation enabled but the shader is not textured
    

    This change doesn't fix any root causes of a texture missing or failing to load, it only makes it not abort anymore. Also removed a now-irrelevant TODO and a branch in flat material data setup -- that check should have been done in the shader setup code in the first place anyway, which is how it's done now. The TODO was about a different thing, reverted that back.

  • Fixed a crash if the BasisImporter plugin wouldn't be present for whatever reason. (I disabled it in order to be able to test the above assert, and discovered this crash.)

  • And fixed a case where files without a default scene would be treated as having no scene at all. This is the case for example with glTF files produced by gltfpack.

How Has This Been Tested

With the BasisImporter plugin disabled, loading gltfpack-produced files shared by @mukulkhanna no longer dies with the above shader assertion.

Originally the assertion was a consequence of this error:

Trade::AnyImageImporter::openData(): cannot load the KtxImporter plugin

... which as far as I can tell isn't reproducible on current main anymore. It was fixed by mosra/magnum@569ae5a (which delegates to BasisImporter if KtxImporter isn't found), which was merged into Habitat with #1853.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@mosra mosra requested a review from aclegg3 October 26, 2022 11:15
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 26, 2022
@0mdc 0mdc self-requested a review October 26, 2022 14:32
If it's not (for example if a texture failed to load or if the
material data are strange in some way), then the shader construction
would assert with

    Shaders::PhongGL: texture transformation enabled but the shader is not textured

This change *doesn't* fix any root causes of a texture missing or
failing to load, it only makes it not abort anymore.
Magnum's GltfImporter used to return 0 if the default scene field wasn't
present, but now it returns -1 to indicate no default scene is
specified, consistently with all other importers. The code mistakenly
relied on the previous behavior.
@mosra mosra force-pushed the texture-transform-only-if-textured branch from 805e893 to 8b033b7 Compare October 26, 2022 15:53
@mosra mosra merged commit 7a5cd14 into main Oct 26, 2022
@mosra mosra deleted the texture-transform-only-if-textured branch October 26, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants