add TrOneIsOpaque option to MTLLoader #12862

wtlyu · 2017-12-12T04:52:21Z

mrdoob · 2017-12-12T06:24:06Z

I don't know...

wtlyu · 2017-12-12T09:30:13Z

@mrdoob I have re-committed this modification. This modification won't change the default behavior of MTLLoader. Only add a new option to give us capability to switch the parsing of keyword Ts (Ts = 1 - d or Ts = d).

Related documents is also updated.

Mugen87 · 2017-12-12T10:30:59Z

examples/js/loaders/MTLLoader.js

 					if ( n > 0 ) {

-						params.opacity = 1 - n;
+						if ( this.options && this.options.equalizeTransparency )


Just a code style issue. Please write it like this

if ( this.options && this.options.equalizeTransparency ) { params.opacity = n; } else { params.opacity = 1 - n; }

ok, I will update soon

WestLangley · 2017-12-12T12:41:54Z

Did you test this code?

WestLangley · 2017-12-12T12:47:22Z

Maybe

options.TrOneIsOpaque = true; // default is false

WestLangley · 2017-12-12T19:49:33Z

examples/js/loaders/MTLLoader.js

-						params.opacity = 1 - n;
+						if ( this.options && this.options.TrOneIsOpaque ) {
+
+							params.opacity = n;


params.transparent is set to true when options.TrOneIsOpaque = true and Tr = 1. ???

Emmmm... good comment. I didn't notice it. fix soon

mrdoob · 2017-12-18T21:35:08Z

docs/examples/loaders/MTLLoader.html

 				<li>wrap: What type of wrapping to apply for textures. THREE.RepeatWrapping (default), THREE.ClampToEdgeWrapping, THREE.MirroredRepeatWrapping</li>
 				<li>normalizeRGB: RGBs need to be normalized to 0-1 from 0-255. Default: false, assumed to be already normalized</li>
 				<li>ignoreZeroRGBs: Ignore values of RGBs (Ka,Kd,Ks) that are all 0's. Default: false</li>
+				<li>TrOneIsOpaque: Use values 1 of Tr field for fully opaque. This option is useful for obj exported from 3ds MAX, vcglib or meshlab. Default: false</li>


How about calling it invertTr or invertTrProperty?

mrdoob · 2017-12-18T21:39:37Z

if ( this.options && this.options.invertTr ) n = 1 - n;

if ( n < 1 ) {

	params.opacity = n;
	params.transparent = true;

}

Mugen87 · 2018-01-04T13:51:34Z

@eastpiger Any news on this PR?

wtlyu · 2018-01-16T09:48:04Z

Yep.

mrdoob · 2018-01-18T01:05:19Z

Thanks!

RSpace · 2018-03-22T10:49:07Z

Merging this PR has made any obj model material with Tr 0.0000 specified in the mtl file completely transparent, where before it would be completely opaque. It's breaking quite a lot stuff out there.

The problem looks fairly obvious to me. It was fine to add

if ( this.options && this.options.invertTrProperty ) n = 1 - n;

but

					if ( n > 0 ) {

						params.opacity = 1 - n;
						params.transparent = true;

					}

should never have been changed to

					if ( n < 1 ) {

						params.opacity = n;
						params.transparent = true;

					}

which completely reverse the logic and makes invertTrProperty work reverse of what it should have.

I'd be happy to submit a PR to fix this, unless I'm missing something?

mrdoob · 2018-03-24T17:37:49Z

Yes please. A PR would be great 👌

WestLangley · 2018-03-24T20:58:56Z

which completely reverse the logic and makes invertTrProperty work reverse of what it should have.

#12861 (comment) implies "what it should have" is not well-defined. (Are the docs correct?)

Consequently, this.options.invertTrProperty is not well-defined, either.

RSpace · 2018-03-24T22:02:00Z

@WestLangley Agreed. I have no need for this.options.invertTrProperty personally, and as such would be just a happy to submit a PR that simply reverts this PR and gets rid of this.options.invertTrProperty, as I would be to submit a PR that changes logic as described above, but still leaves the question of whether the logic behind this.options.invertTrProperty is correct open.

I assume this PR was merged for a good reason, but I'd prefer reverting it unless @eastpiger or someone else can chime in with a good explanation of what's going on here?

WestLangley · 2018-03-25T01:43:30Z

As implemented, Tr and d are, by default, interpreted the same: 0 is transparent, and 1 is opaque. However, as written, the documentation is not correct.

wtlyu · 2018-03-25T18:04:37Z

Hi all. I'm glad to make more explanation about this problem.

as I described in issue #12861, the definition of Tr option in .MTL file is confusing since it actually hasn't never been defined officially. There is a big controversy about it between academic and engineering application. I tried to ask Dr. John Burkardt, which holds the earliest webpage I can find given another different definition, and he didn't give a clear reason and answer about that. Since these two conflicting definition is both widely used in some big projects, (for example, d = Kr = 1 for opaque material is used in 3DS MAX and vcglib, which built meshLab, a good cloud point processing tool project), I give this PR to three.js hoping given a nondiscriminatory treat to them.

On the other hand. I do totally agree that this change is not supposed to break the back compatibility. I feel very very sorry to bother your work these days. Because that we had some discussion about how to introduce this feature properly and I made reset-and-recommit three times before it was merged. I used the suggestion given by mrdoob in the final version. Unfortunately, because of my carelessness, I didn't re-check the code here and it do make an inverse effect. Yes, @RSpace , you are right, that code treats the Tr option in the inverse way.

I will open a new PR fixing this soon.

and I do want to express my apologize of my careless, and I'm truly very very sorry to @mrdoob @WestLangley @RSpace as well as anyone who was confused by my mistake.

RSpace · 2018-03-31T14:20:14Z

@eastpiger Thanks for the clarification. We all make mistakes, and I'm thankful to everyone who contributes to three.js, making it the fantastic library it is today. Your PR #13685 fixes the issue for me :)

mrdoob · 2018-04-03T23:37:07Z

and I do want to express my apologize of my careless, and I'm truly very very sorry to @mrdoob @WestLangley @RSpace as well as anyone who was confused by my mistake.

No worries!! 🙌

wtlyu mentioned this pull request Dec 12, 2017

add TrOneIsOpaque option to MTLLoader #12861

Closed

14 tasks

wtlyu force-pushed the eastpiger-patch-MTLLoader-equalizeTransparency branch from fff1a5f to e140664 Compare December 12, 2017 09:24

Mugen87 requested changes Dec 12, 2017

View reviewed changes

wtlyu force-pushed the eastpiger-patch-MTLLoader-equalizeTransparency branch 2 times, most recently from b4f9e56 to 9007c0b Compare December 12, 2017 16:46

wtlyu changed the title ~~add equalizeTransparency option to MTLLoader~~ add TrOneIsOpaque option to MTLLoader Dec 12, 2017

WestLangley reviewed Dec 12, 2017

View reviewed changes

wtlyu force-pushed the eastpiger-patch-MTLLoader-equalizeTransparency branch from 9007c0b to 64ecc25 Compare December 13, 2017 16:58

mrdoob reviewed Dec 18, 2017

View reviewed changes

add invertTrProperty option to MTLLoader

ad90938

wtlyu force-pushed the eastpiger-patch-MTLLoader-equalizeTransparency branch from 64ecc25 to ad90938 Compare January 16, 2018 09:21

mrdoob merged commit b2fc3b4 into mrdoob:dev Jan 18, 2018

mrdoob added this to the r90 milestone Jan 18, 2018

kaisalmen mentioned this pull request Jan 24, 2018

OBJLoader2 V2.3.1: Parser Verification, Point and Line support, moved generic functions to LoaderBase #13156

Merged

RSpace mentioned this pull request Mar 22, 2018

Invisible controllers aframevr/aframe#3441

Closed

wtlyu mentioned this pull request Mar 25, 2018

fix wrong parser behavior to Tr option with parameter invertTrProperty in MTLLoader.js #13685

Merged

Uh oh!

add TrOneIsOpaque option to MTLLoader #12862

add TrOneIsOpaque option to MTLLoader #12862

Uh oh!

Conversation

wtlyu commented Dec 12, 2017 • edited by mrdoob Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Dec 12, 2017

Uh oh!

wtlyu commented Dec 12, 2017

Uh oh!

Mugen87 Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

wtlyu Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

WestLangley commented Dec 12, 2017

Uh oh!

WestLangley commented Dec 12, 2017

Uh oh!

WestLangley Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtlyu Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

wtlyu Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

mrdoob Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

mrdoob commented Dec 18, 2017

Uh oh!

Mugen87 commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtlyu commented Jan 16, 2018

Uh oh!

mrdoob commented Jan 18, 2018

Uh oh!

RSpace commented Mar 22, 2018

Uh oh!

mrdoob commented Mar 24, 2018

Uh oh!

WestLangley commented Mar 24, 2018

Uh oh!

RSpace commented Mar 24, 2018

Uh oh!

WestLangley commented Mar 25, 2018

Uh oh!

wtlyu commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RSpace commented Mar 31, 2018

Uh oh!

mrdoob commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wtlyu commented Dec 12, 2017 •

edited by mrdoob

Loading

WestLangley Dec 12, 2017 •

edited

Loading

Mugen87 commented Jan 4, 2018 •

edited

Loading

wtlyu commented Mar 25, 2018 •

edited

Loading