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

Consider warning on redundant definitions #163

Open
donmccurdy opened this issue May 12, 2021 · 4 comments
Open

Consider warning on redundant definitions #163

donmccurdy opened this issue May 12, 2021 · 4 comments

Comments

@donmccurdy
Copy link
Contributor

Recently we had a couple user-facing bugs in three.js (mrdoob/three.js#21559, mrdoob/three.js#21819) which came down to redundant definitions in a glTF file. Both bugs led to duplicate instances of the THREE.Texture class and redundant GPU texture allocation:

This output came from Blender:

"textures": [
  {
    "sampler": 0,
    "source": 0
  },
  {
    "sampler": 0,
    "source": 0
  }
],
"images": [
  {
    "mimeType": "image/jpeg",
    "name": "REDgrad",
    "uri": "REDgrad.jpg"
  }
],

And this output came from the Babylon.js 3DS Max Exporter:

"texCoord": 0,
"extensions": {
  "KHR_texture_transform": {
    "texCoord": 0
  }
}

Both pass the validator, and aren't "wrong", but — especially in the first example — it's difficult for our loader to do deep comparisons and figure out how many things to allocate. Equivalently, the textures could have (but didn't) referred to different but identical samplers, and we'll (still) end up allocating extra texture memory in that case.

The second case, with the texture transform, partially our fault because of limitations in our KHR_texture_transform support, but basically the extension isn't doing anything here and would ideally not be included.

tl;dr – it'd be helpful for the ecosystem if the validator could flag redundant definitions, like identical samplers and textures.

@bghgary
Copy link

bghgary commented May 12, 2021

it's difficult for our loader to do deep comparisons and figure out how many things to allocate

This is interesting. Is the three.js loader deduping identical textures? I would not expect loaders to do this.

it'd be helpful for the ecosystem if the validator could flag redundant definitions, like identical samplers and textures.

+1

@lexaknyazev
Copy link
Member

How should extras and extensions affect redundancy detection?

@donmccurdy
Copy link
Contributor Author

donmccurdy commented May 12, 2021

Is the three.js loader deduping identical textures?

Currently not, but we may start de-duping textures entries with the same source and sampler indices to work around this issue. Duplicate images entries would not be deduped. We do some caching at the network layer but that isn't enough prevent duplicate GPU texture allocation.

How should extras and extensions affect redundancy detection?

Ideally some type of "deep equality", but even skipping redundancy detection on anything with extensions or extras would probably catch a lot.

@donmccurdy
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants