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

MaterialLoader:createMaterial() for extended Material class #22552

Closed
wants to merge 3 commits into from

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Sep 18, 2021

Description

The function MaterialLoader.createMaterial( type ) can be useful to create loader for extended Materials, like NodeMaterials.

Example:

class NodeMaterialLoader extends MaterialLoader {

	constructor( manager ) {

		super( manager );

	}

	createMaterial( type ) {

		// use an extended material if here is

		if ( NodeMaterial[ type ] !== undefined ) {

			return new NodeMaterial[ type ]();

		}

		return super.createMaterial( type );

	}

	parse( json ) {

		const material = super.parse( json );

		// stuff

		return material;

	}

}

This contribution is funded by Google via Igalia.

@mrdoob
Copy link
Owner

mrdoob commented Sep 20, 2021

How about adding this to Material.js as a static class instead?

const material = Material.fromType( type );

@mrdoob mrdoob added this to the r133 milestone Sep 20, 2021
@sunag
Copy link
Collaborator Author

sunag commented Sep 20, 2021

This look a bit more complicated to extended using only Material.fromType(). For example, without MaterialLoader.createMaterial() is possible to replace the prototype, something like:

Material.prototype._fromType = Material.prototype.fromType;

Material.prototype.fromType = ( type ) => {

	if ( NodeMaterialLib[ type ] !== undefined ) {

		return new NodeMaterialLib[ type ]();

	}

	return Material.prototype._fromType( type );

}

@sunag
Copy link
Collaborator Author

sunag commented Sep 22, 2021

@mrdoob I can do like this, if you to prefer:

const materialLoader = new MaterialLoader();

 // material lib
materialLoader.library = Materials;

materialLoader.parse( json );

@sunag
Copy link
Collaborator Author

sunag commented Sep 22, 2021

Current PR:

const materialLoader = new NodeMaterialLoader();
materialLoader.parse( json );

@mrdoob
Copy link
Owner

mrdoob commented Sep 22, 2021

This look a bit more complicated to extended using only Material.fromType(). For example, without MaterialLoader.createMaterial() is possible to replace the prototype, something like:

Material.prototype._fromType = Material.prototype.fromType;

Material.prototype.fromType = ( type ) => {

	if ( NodeMaterialLib[ type ] !== undefined ) {

		return new NodeMaterialLib[ type ]();

	}

	return Material.prototype._fromType( type );

}

That doesn't look too bad. How about creating a NodeMaterial class too?
Probably doesn't need to extend Material?

@sunag
Copy link
Collaborator Author

sunag commented Sep 22, 2021

Probably doesn't need to extend Material?

The problem is that the Materials lib is readonly inside parse():
Currently, It creates only the root materials and not allows being extended.

const material = new Materials[ json.type ]();

@sunag
Copy link
Collaborator Author

sunag commented Sep 22, 2021

I am developing a version using Material.fromType()...

@sunag
Copy link
Collaborator Author

sunag commented Sep 22, 2021

Seems that is not possible import Materials.js inside Material because has redundancy cycle of imports. So I created MaterialUtils.fromType(). What do you think?

@sunag
Copy link
Collaborator Author

sunag commented Sep 24, 2021

How about creating a NodeMaterial class too?

I will try to mature this then. If we use a NodeMaterial as root node materials, an extension of MaterialLoader for NodeMaterial like this PR lost sense.

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2021

I am thinking about close this PR because extending the current NodeMaterials for a NodeMaterial base made the things a bit different and better in my option. In this context extend the MaterialLoader is not so more necessary. Anyway, we can a wait a little until that I can publish the PR related to close this.

@mrdoob mrdoob modified the milestones: r133, r134 Sep 30, 2021
@mrdoob mrdoob modified the milestones: r134, r135 Oct 28, 2021
@mrdoob mrdoob modified the milestones: r135, r136 Nov 26, 2021
@sunag
Copy link
Collaborator Author

sunag commented Dec 14, 2021

@mrdoob I still have some issue with this, exist some possibilities here that I noticed:

  1. Add Material.fromType() in Material.js result in Circular depedencies I can add this in other file to fix it, for example: MateiralUtils.js.
  2. Use .fromType() in other class MaterialUtils.fromType().
  3. Use MaterialLoader.createMaterial().
  4. Use library variable in MaterialLoader: materialLoader.library = MaterialsLib

@mrdoob mrdoob modified the milestones: r136, r137 Dec 24, 2021
@sunag sunag closed this Jan 24, 2022
@sunag sunag removed this from the r137 milestone Jan 24, 2022
@sunag sunag deleted the dev-materialloader-extension branch January 27, 2022 05:26
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.

3 participants