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

GLTFExporter: Plugin system for extensibility #20842

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Dec 5, 2020

Description

This PR proposes a extensibility mechanism for GLTFExporter similar to the GLTFLoader plugin system.

The idea is originally proposed by @garyo at #11682 (comment). If you @garyo have been also working on the exporter extensibility mechanism and my work is conflicted with yours, please let me know.

API

const exporter = new GLTFExporter().register(parser => {
  return new GLTFMaterialsUnlitExtension(parser);
});
exporter.parse(scene, gltf => {
  console.log(gltf);
});

class GLTFMaterialsUnlitExtension(parser) {
  constructor(parser) {
    this.parser = parser;
    this.name = 'KHR_materials_unlit';
  }

  // Hookpoint for processMaterial().
  // Called at the end of processMaterial().
  // @param {THREE.Material} material
  // @param {Object} materialDef
  parseMaterial(material, materialDef) {
    if (!material.isMeshBasicMaterial) return;
    const parser = this.parser;
    const extensionsUsed = parser.extensionsUsed;
    // Refer to the GLTFParser constructor for other parser properties

    // You can add extension definition.
    materialDef.extensions = materialDef.extensions || {};
    materialDef.extensions[this.name] = {};

    // You can add to extensionsUsed
    extensionsUsed[this.name] = true;

    // You can override core spec gltf definition
    materialDef.pbrMetallicRoughness.metallicFactor = 0.0;
    materialDef.pbrMetallicRoughness.roughnessFactor = 0.9;

    // If you have async logic, you can add to pending.
    // The parser outputs the result glTF if all parser.pending are resolved.
    // See GLTFParser.parse().
    // const pending = parser.pending;
    // pending.push(new Promise((resolve, reject) => { ... }));
  }
}

Advantages

Similar to GLTFLoader plugin system,

  • Core GLTFExporter code will be simpler and clearer because we can separate the extension handler logics from the core code
  • We can easily add upcoming glTF extensions handlers to the exporter
  • Users will be able to add their custom extensions handlers without changing the exporter core code

Expected use cases

  • We implement new glTF extensions handlers as the plugin while we keep the core code simple
  • Users implement their custom glTF extensions handlers as the plugin without making their forks
  • The exporter exports glTF very simply so if users want optimized glTF they need to use post-process workflow tools. With this change, if they want to directly export optimized glTF, they may do by writing optimization plugins. (I haven't deeply thought yet if the current proposed API is powerful enough to do that tho.)

Change

Similar to GLTFLoader

  • Add .register(callback) and .unregister(callback) to GLTFExporter
  • Make GLTFParser (English question, is this name good in the exporter?) in GLTFExporter for internal and plugin use. Parser instance is passed to plugins constructors. The plugins can use the parser to handle their logics
  • Add parseMesh/Texture/Material/Node (English question, are these names good?) hook points so far. They are called at the end of processMesh/Texture/Material/Node(). The hook points take the two arguments, input Three.js object and glTF core spec definition made by the core parser. The plugins add or override glTF definition depending on the Three.js object. We may add other hook points later.
  • Add before/afterParse (are they good name?) hook points. It would be used for the root level extensions or processing something at the beginning/end of the export.
  • Add _invokeAll() method to GLTFExporter to call plugins' methods.
  • Move the lights, unlit, and specular-glossiness extension handlers from the core parser but keep them in GLTFExporter.js file. They are registered by default.
  • Naming clean up for the consistency with GLTFLoader.
  • A little bit clean up

Ask for help

  • Any additional hook points/features/functionalities you want?
  • Any feedback is welcome
  • Would you folks please help to check if this change doesn't break the exporter? I didn't change the exporter core logic at all but did a lot of copy-and-paste and editted the indentations. I confirmed misc_exporter_gltf.html and the example unit tests keep working fine but I would test more. If you use the existing exporter, would you please try to locally merge this change and check if your apps keep working?

/cc @donmccurdy @zeux @fernandojsg @robertlong @garyo

@takahirox takahirox marked this pull request as draft December 5, 2020 22:58
@takahirox takahirox force-pushed the GLTFExporterPlugin branch 2 times, most recently from 502f10b to b743970 Compare December 6, 2020 00:10
@garyo
Copy link
Contributor

garyo commented Dec 6, 2020

Hi; I ended up doing my processing by duplicating the geometry and making my tweaks there before exporting, so no conflict with my work at all. It would be great to use this instead of my hack when it's ready though! (And I would really love to optimize the resulting glTF.)

@takahirox
Copy link
Collaborator Author

Any thoughts on this change? @donmccurdy

@takahirox takahirox marked this pull request as ready for review January 28, 2021 05:47
@takahirox
Copy link
Collaborator Author

No opposite opinion so far so I marked the PR as ready for review.

utils/modularize.js Outdated Show resolved Hide resolved
@mrdoob mrdoob added this to the r126 milestone Jan 28, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 28, 2021

@elalish does this help with the variants issue?

@elalish
Copy link
Contributor

elalish commented Jan 28, 2021

@mrdoob Yes, this is necessary for variants export. I didn't notice any red flags when I read through it. If you merge this I'll start building on it in model-viewer and let you know if I run into anything.

@mrdoob
Copy link
Owner

mrdoob commented Jan 30, 2021

@donmccurdy How do you feel about this one?

@donmccurdy
Copy link
Collaborator

I won't be able to do a detailed review or testing on the code changes here, but I like the intention of making this extensible and agree with the design in this PR. It seems like a good structure for the exporter's code moving forward. Once @takahirox or others feel this is ready to merge, that is fine with me. 👍


On naming — I would suggest renaming GLTFParser, parseMesh, parseTexture, etc. to something like GLTFWriter, writeMesh, writeTexture. The word "parse" usually refers to reading and not writing.

@mrdoob
Copy link
Owner

mrdoob commented Feb 3, 2021

On naming — I would suggest renaming GLTFParser, parseMesh, parseTexture, etc. to something like GLTFWriter, writeMesh, writeTexture. The word "parse" usually refers to reading and not writing.

Sounds good to me 👍

@takahirox
Copy link
Collaborator Author

Thanks for the comments. I renamed them.

And I would be happy if you folks would help this

Would you folks please help to check if this change doesn't break the exporter? I didn't change the exporter core logic at all but did a lot of copy-and-paste and editted the indentations. I confirmed misc_exporter_gltf.html and the example unit tests keep working fine but I would test more. If you use the existing exporter, would you please try to locally merge this change and check if your apps keep working?

@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2021

I'll merge so people have time to test before the next release.

@mrdoob mrdoob merged commit ef6a691 into mrdoob:dev Feb 4, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2021

Thanks!

@takahirox takahirox deleted the GLTFExporterPlugin branch February 4, 2021 09:55
@mrdoob
Copy link
Owner

mrdoob commented Feb 11, 2021

Found a variable that we missed in this refactoring: 9d8ed27

@takahirox
Copy link
Collaborator Author

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2021

Found another issue in this refactoring: 1435180

@takahirox
Copy link
Collaborator Author

Thanks again!

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.

5 participants