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: Make KHR_materials_unlit able to be overridden #20722

Closed
wants to merge 2 commits into from

Conversation

0b5vr
Copy link
Collaborator

@0b5vr 0b5vr commented Nov 20, 2020

Will close #20709

What I did

  • Replaced hardcoded behavior of KHR_materials_unlit with plugin system introduced in GLTFLoader plugin system, first round #19144 .
    • Current behavior completely ignores getMaterialType and extendMaterialParams of any plugin when KHR_materials_unlit exists in a material.
  • GLTFLoader.register now registers the given plugin callback to the top of its plugin callback list instead of last.
    • Which means, GLTFLoader's plugin system now prioritizes user defined plugins than builtin ones.

Motivation

As I mentioned in #20709, I want to load our own custom glTF material using GLTFLoader.
However, the glTF still have KHR_materials_unlit as a fallback, which is prioritized than any plugins registered.
With these changes, I can override the behavior of KHR_materials_unlit.

Points need review

  • GLTFMaterialsUnlitExtension.extendMaterialParams , does the "Removing material params" part look good?
  • There still are parts that says materialType !== MeshBasicMaterial . I don't know how I can resolve this

@mrdoob mrdoob added this to the r123 milestone Nov 20, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@0b5vr 0b5vr force-pushed the gltfloader-unlit branch 2 times, most recently from 23c6608 to 3713794 Compare February 10, 2021 04:40
@0b5vr
Copy link
Collaborator Author

0b5vr commented Feb 10, 2021

Redone the commits against the latest dev , plus applied the changes also to examples/js/

@0b5vr
Copy link
Collaborator Author

0b5vr commented Feb 10, 2021

ping @donmccurdy @takahirox , how do you think about it? See the "Points need review" section on the description.

@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@0b5vr
Copy link
Collaborator Author

0b5vr commented Mar 9, 2021

Update! Thanks to #21207 I'm now able to remove KHR_materials_unlit in beforeRoot of the plugin that want to override the unlit, my demand is achieved.
This PR still have a value I think though.

Ping @donmccurdy @takahirox

@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@mrdoob mrdoob modified the milestones: r128, r129 Apr 23, 2021
@mrdoob mrdoob modified the milestones: r129, r130 May 27, 2021
@mrdoob mrdoob modified the milestones: r130, r131 Jun 30, 2021
@mrdoob mrdoob modified the milestones: r131, r132 Jul 28, 2021
@mrdoob mrdoob added this to the r135 milestone Oct 28, 2021
@mrdoob mrdoob modified the milestones: r135, r136 Nov 26, 2021
@mrdoob mrdoob modified the milestones: r136, r137 Dec 24, 2021
@mrdoob mrdoob modified the milestones: r137, r138 Jan 26, 2022
@mrdoob mrdoob modified the milestones: r138, r139 Feb 23, 2022
@mrdoob mrdoob modified the milestones: r139, r140 Mar 24, 2022
@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob modified the milestones: r141, r142 May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@donmccurdy
Copy link
Collaborator

@0b5vr is this PR still something you're interested in having merged?

@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@Mugen87 Mugen87 removed this from the r147 milestone Nov 21, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 21, 2022

Closing. Seems the change is not required anymore.

@Mugen87 Mugen87 closed this Nov 21, 2022
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.

GLTFLoader: I cannot override KHR_materials_unlit
5 participants