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

GLTFLoader: add getDependency( type, index ) implementation #24252

Merged
merged 3 commits into from
Nov 3, 2022

Conversation

hybridherbst
Copy link
Contributor

@hybridherbst hybridherbst commented Jun 20, 2022

Description

@takahirox requested that the getDependency('light') implementation should be a separate PR, so here it is.
This is in preparation for supporting the KHR_animation_pointer glTF extension in three.js.

This PR adds a new way for extensions to implement getDependency( type, index ) for custom types; targets for implementation are right now light and audio.

Related:

@takahirox
Copy link
Collaborator

takahirox commented Jun 21, 2022

No conlusion yet but let me share what I'm worried about so far. (It would be better than no response for long.)

I'm thinking whether it is good to let getDependency() in the core glTF loader know the light extension handler. Especially, ._loadLight() of GLTFLightsExtension is expected to be a private method. Ideally the core glTF loader doesn't need to know extensions and their handlers for simplicity.

Perhaps the root issue may be the current plugin system doesn't expect that an extension has dependency with other extensions....

@hybridherbst
Copy link
Contributor Author

hybridherbst commented Jun 21, 2022

Yes, with KHR_animation_pointer an extension certainly has dependencies to other extensions.

I think _loadLight being private (having that underscore) doesn't match how the other extensions do that anyways: the extension methods loadAnimation, loadTexture, loadMaterial etc. all are public.

I'm happy to change _loadLight to loadLight so that these are more consistent to each other.

@robertlong
Copy link
Contributor

robertlong commented Jul 25, 2022

I think it might be better to make the glTF plugins expose an optional getDependency(type, index) function which can be used in the default case of the switch statement in getDependency. This would mean any extension could expose its own resources to be used by other extensions or getDependency can be invoked by the user directly.

For example in KHR_audio you could call

getDependency("audioEmitter", 0)

or

getDependency("audioSource", 0)

And the default loader doesn't need to know anything about audioEmitters or audioSources.

@takahirox
Copy link
Collaborator

takahirox commented Jul 26, 2022

Like this?

// GLTFParser
getDependency( type, index ) {
  const cacheKey = type + ':' + index;
  let dependency = this.cache.get( cacheKey );
  if ( ! dependency ) {
    switch ( type ) {
      ...
      default:
        dependency = this._invokeOne( function ( ext ) {
          return ext !== this && ext.getDependency && ext.getDependency( type, index );
        } );
        if ( ! dependency ) {
          throw new Error( 'Unknown type: ' + type );
        }
        break;
    }
    ...
  }
  ...
}

@robertlong
Copy link
Contributor

Yeah, exactly 👍

@takahirox
Copy link
Collaborator

Maybe that sounds good.

Nit: parser.getDependency(type, index) calls .loadXXX(index) that returns the dependency as promise, caches it, and returns it. For consistency, perhaps plugin should have .loadXXX(index) (or .load(type, index)) methods.

// GLTFParser
getDependency( type, index ) {
  const cacheKey = type + ':' + index;
  let dependency = this.cache.get( cacheKey );
  if ( ! dependency ) {
    switch ( type ) {
      ...
      default:
        const functionName = 'load' + type[0].toUpperCase() + type.slice(1); // ugh...
        dependency = this._invokeOne( function ( ext ) {
          return ext[functionName] && ext.[functionName]( index );
          // or return ext.load && ext.load( type, index ); ?
        } );
        if ( ! dependency ) {
          throw new Error( 'Unknown type: ' + type );
        }
        break;
    }
    ...
  }
  ...
}

@hybridherbst hybridherbst force-pushed the gltfloader-lights-dependency branch from b081e45 to b309ca2 Compare September 6, 2022 21:39
@hybridherbst
Copy link
Contributor Author

hybridherbst commented Sep 6, 2022

I rebased this on dev and implemented the suggestions from here – let me know if that works, thanks @takahirox!

Regarding the .loadXXX approach, not sure if the potential added clarity due to that offsets that weird uppercasing-and-splicing...

@hybridherbst hybridherbst changed the title GLTFLoader: add getDependency('light') implementation GLTFLoader: add getDependency( type, index ) implementation Sep 6, 2022
@Mugen87 Mugen87 added this to the r147 milestone Nov 3, 2022
@Mugen87 Mugen87 merged commit cd017c6 into mrdoob:dev Nov 3, 2022
@hybridherbst hybridherbst deleted the gltfloader-lights-dependency branch November 24, 2022 23:23
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.

6 participants