Skip to content

Conversation

@kaisalmen
Copy link
Contributor

@kaisalmen kaisalmen commented Jan 22, 2018

These are the enhancements and bug fixes:

  • Org Repo Issue 28: Parser Validation. Parser now is one single class. Simplified code, fixed bugs missing 'g' statement, etc. and introduced obj_obj2 verification examples with gulpfile to re-create verify.obj.
  • Org Repo Issue 31: Worker code still works when mangling is used during minification. Needed to remove Consts.
  • Org Repo Issue 32: THREE.LoaderSupport.ConsoleLogger: Allow to pass additional arguments to error, warn, info and debug
  • Org Repo Issue 33: Added support for Points and fixed Lines along
  • Fixed CRLF issues. Now all files has unix line-endings.

Hi @mrdoob, here comes an important update. I have finished a Parser review. It is now one single object. The code should be easier to understand for others, I think.
I have introduced verify.obj which can be rebuilt with gulp instructions. It is is used by new example obj_obj2_verify. This should be used and expanded in the future to highlight & fix Parser issues. OBJLoader has some (Omission of 'g' and usage of different face descriptions). OBJLoader2 is now able to handle those cases.

@kaisalmen
Copy link
Contributor Author

Do you have any idea why the checks were unable to run?

@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jan 23, 2018

I amended the commit and push it again. Putting the Parser into its own namespace is no longer required It was a work-around to overcome minification problems that is no longer required.

"webgl_loader_nodes",
"webgl_loader_obj",
"webgl_loader_obj_mtl",
"webgl_loader_obj_obj2_verify",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about webgl_loader_obj2_verify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses both OBJLoader and OBJLoader2 that's why I chose that name. You decide!


return workerCode;
};
var libs2Load = [ 'node_modules/three/build/three.js' ];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node_modules/three/build/three.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be build/three.min.js. Thanks.
There are other path issues in other examples. I will double check now and push an update.

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Jan 23, 2018
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jan 23, 2018

I fixed the path issues, but the verify.obj is not loaded properly. Did someone break MTLLoader? When I don't assign materials the cubes are displayed.

Solution: I removed the Tr statements from the mtl file as it is not needed. It is now working properly. Latest commit on MTLLoader changed the Tr statement handling.

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Jan 23, 2018
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jan 24, 2018

Some background info on the mtl problem. I manually derived the mtl file and I think I have started of one that was created by 3dsmax. Tr was set to 0. There is no need for transparency in my current test case (not yet), but after now checking #12862 I wonder why the behaviour changes if you have not set the option in MTLLoader.

Correction: The behaviour is correct. If Tr 0.000 then an object is fully transparent. Removal of Trfrom verify.mtl is anyway correct as it is not in the spec. In the former version of MTLLoader the value "0.0" was not considered. This explains the change of behaviour.

Edit: I added a param to pass material options to OBJLaoder2.loadMtl helper functions, so behaviour can be adjusted as required. See commit below.

kaisalmen pushed a commit to kaisalmen/three.js that referenced this pull request Jan 24, 2018
kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Jan 26, 2018
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jan 26, 2018

@mrdoob I pushed another update that adds Point support p support and it fixes Line creation. Now, OBJLoader2 has Parsing feature parity again (#13060)
Line and Point Support are available in the global THREE.LoaderSupport.Builder. User just needs to ensure proper Material are used which is now ensured by the Builder.

From my point of view this is ready. 👍
Do you know why the code checks fail? This seems to be not code related.

@kaisalmen kaisalmen changed the title OBJLoader2 V2.3.0: Parser Verification & review, Minification Support, etc. OBJLoader2 V2.3.0: Parser Verification, Point, Line and Minification Support Jan 26, 2018
kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Jan 29, 2018
…from functions list (fixed singleton typo along)

Builder defines the default materials and renamed 'vertexColorMaterial' to 'defaultVertexColorMaterial' for consistency.
Fixed Builder.updateMaterials clone instructions
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jan 29, 2018

Ok, here are some more improvements to the WorkerSupport resulting from work on worker based PCDLoader. Force push (below) was required, because I forgot a single brace. 😅
Do you see this in R90 by any chance?

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Jan 29, 2018
…from functions list (fixed singleton typo along)

Builder defines the default materials and renamed 'vertexColorMaterial' to 'defaultVertexColorMaterial' for consistency.
Fixed Builder.updateMaterials clone instructions
@kaisalmen
Copy link
Contributor Author

V2.3.0 is now available via npm:
https://www.npmjs.com/package/wwobjloader2

@kaisalmen kaisalmen changed the title OBJLoader2 V2.3.0: Parser Verification, Point, Line and Minification Support OBJLoader2 V2.3.1: Parser Verification, Point and Line support, moved generic functions to LoaderBase Feb 5, 2018
@kaisalmen
Copy link
Contributor Author

In preparation of PCDLoader PR (add parseAsync and run) which will come tomorrow (need to update docs as well and ran out of time) I have pushed another update bumping the minor version up.
Basically, generic functions load and checkResourceDescriptorFiles extracted from OBJLoader2 are now available in THREE.LoaderSupport.LoaderBase.

@mrdoob do you see this whole PR merged before R90? Thanks

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Feb 9, 2018
OrgRepoIssue 28: Parser Validation. Parser now is one single class: Simplified code, fixed bugs missing 'g' statements and introduced obj_obj2 verification with gulpfile to re-create verify.obj.
OrgRepoIssue 31: Worker code still works when mangling is used during minification. Needed to remove Consts.
OrgRepoIssue 32: THREE.LoaderSupport.ConsoleLogger: Allow to pass additional arguments to error, warn, info and debug
OrgRepoIssue 33: Added support for Points and fixed Lines along
- Removed Tr instruction from verify.mtl
- WorkerSupport.buildSingelton allows to use constructors from functions list (fixed singleton typo along)
- Builder defines the default materials and renamed 'vertexColorMaterial' to 'defaultVertexColorMaterial' for consistency.
- Fixed Builder.updateMaterials clone instructions
- Extracted load and checkResourceDescriptorFiles to LoaderBase.
- loadMtl now allows to pass material options to MTLLoader
- Updated documentation
OrgRepoIssue 28: Parser Validation. Parser now is one single class: Simplified code, fixed bugs missing 'g' statements and introduced obj_obj2 verification with gulpfile to re-create verify.obj.
OrgRepoIssue 31: Worker code still works when mangling is used during minification. Needed to remove Consts.
OrgRepoIssue 32: THREE.LoaderSupport.ConsoleLogger: Allow to pass additional arguments to error, warn, info and debug
OrgRepoIssue 33: Added support for Points and fixed Lines along
- Removed Tr instruction from verify.mtl
- WorkerSupport.buildSingelton allows to use constructors from functions list (fixed singleton typo along)
- Builder defines the default materials and renamed 'vertexColorMaterial' to 'defaultVertexColorMaterial' for consistency.
- Fixed Builder.updateMaterials clone instructions
- Extracted load and checkResourceDescriptorFiles to LoaderBase.
- loadMtl now allows to pass material options to MTLLoader
- Updated documentation
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Feb 9, 2018

@mrdoob I squashed down all commits to one and put it at the head of dev (sorry, needed to force push, again as I was behind). Now, it is in-line with your CRLF commit.

I moved verify.html alongside the verify.obj to examples/obj/verify and removed it from the example. This really is more a test than an example.

@mrdoob
Copy link
Owner

mrdoob commented Feb 9, 2018

I moved verify.html alongside the verify.obj to examples/obj/verify and removed it from the example.

Do you mind moving these to examples/models/obj/verify instead? Ideally we'll kill that examples/obj folder soon.

@mrdoob mrdoob added this to the r90 milestone Feb 9, 2018
@kaisalmen
Copy link
Contributor Author

Done, separated point and line cubes in verify.obj in addition.

@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2018

Sweet!

@mrdoob mrdoob merged commit 827e3e1 into mrdoob:dev Feb 13, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2018

Thanks!

@kaisalmen kaisalmen deleted the OBJLoader2_V230 branch February 14, 2018 04:32
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.

2 participants