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

wwobjloader2 docs and code enhancements #11200

Merged
merged 3 commits into from
Apr 24, 2017
Merged

Conversation

kaisalmen
Copy link
Contributor

@kaisalmen kaisalmen commented Apr 19, 2017

Hello @mrdoob, I finally managed to add the missing documentation for OBJLoader2 and WWOBJLoader2. That is the first commit: 8e29b6f

Then, there are improvements to the code based on community enhancement request (including me) from repo WWOBJLoader: 4ebf28b
It seems to be more then it really is (changelog is below). Don't know when release 85 is due, but it'll be nice if this could make it as well. Just released wwobjloader2 npm compatible with R84. Docs on current code are separated, so you can merge it independently if you have any objections. Thanks! 😄

See the changelog:
Updated code to version V1.2.1 and updated docs accordingly.

Code related changes

THREE.OBJLoader2.WWOBJLoader2
  • Function _receiveWorkerMessage now uses a meshDescription that allows to override material or bufferGeometry or to completely disregard the mesh. THREE.OBJLoader2.WWOBJLoader2.LoadedMeshUserOverride was introduced for this.
  • Allow usage of multiple callbacks per callback type
  • THREE.OBJLoader2.WWOBJLoader2.PrepDataArrayBuffer and THREE.OBJLoader2.WWOBJLoader2.PrepDataFile require less mandatory parameters. Setters are introduced to handle optional things
THREE.OBJLoader2.WWOBJLoader2Director
  • Added per queue object callbacks
  • Global callbacks in prepareWorkers will be specified with new object OBJLoader2.WWOBJLoader2.PrepDataCallbacks. This object is also used in both PrepData objects for defining extra per model callbacks in addition to the global ones
  • Callbacks will be reset and reassigned for every run
All
  • Improve code quality and logging: Replaced != or == with Boolean() or ! Boolean() where applicable
  • Improve logging and comments

@@ -221,7 +220,7 @@ THREE.OBJLoader2 = (function () {
}

Parser.prototype.setDebug = function ( debug ) {
this.debug = ( debug == null ) ? this.debug : debug;
this.debug = Boolean( debug ) ? debug : this.debug;
Copy link
Owner

@mrdoob mrdoob Apr 19, 2017

Choose a reason for hiding this comment

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

I don't like this Boolean() stuff...

Consider this case:

> Boolean( "true" )
< true

I think this is more robust:

this.debug = debug === true ? debug : this.debug;

Copy link
Contributor Author

@kaisalmen kaisalmen Apr 20, 2017

Choose a reason for hiding this comment

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

Updated
You are correct. I wanted to make things more elegant, but I introduced potential problems that did not exist before.

I will kill all the Boolean stuff and replace it with correct helper functions inside OBJLoader2. These three functions will cover all cases. verifyBoolean handles non-boolean identification properly and returns forced-boolean defaultValue if for example input is null, undefined, "blah" or new Object(). Fully updated code will be available tonight (CEST):

var Helper = {
	/**
	 * Only evaluate input in case it really is boolean (true or false). Otherwise return defaultValue as forced boolean
	 * @param {Object} input If not true or false defaultValue is returned
	 * @param {boolean} defaultValue Will be forced to boolean if not boolean
	 * @returns {boolean}
	 */
	verifyBoolean: function( input, defaultValue ) {
		return ( input === true || input === false ) ? input : ( defaultValue === true );
	},
	/**
	 * If given input is null or undefined, false is returned otherwise true.
	 *
	 * @param input Anything
	 * @returns {boolean}
	 */
	isValid: function( input ) {
		return ( input !== null && input !== undefined );
	},
	/**
	 * If given input is null or undefined, the defaultValue is returned otherwise the given input.
	 *
	 * @param input Anything
	 * @param defaultValue Anything
	 * @returns {*}
	 */
	verifyInput: function( input, defaultValue ) {
		return ( input === null || input === undefined ) ? defaultValue : input;
	}
};

Then
this.debug = debug === true ? debug : this.debug;
could be expressed as follows:
this.debug = Helper.verifyBoolean( debug, this.debug );

Further examples:
this.streamMeshes = ( streamMeshes === null || streamMeshes === undefined ) ? true : streamMeshes;
will become:
this.streamMeshes = Helper.verifyBoolean( streamMeshes, true );

modelName: Boolean( modelName ) ? modelName : 'none'
will become:
modelName: Helper.verifyInput( modelName, 'none' )

if ( ! Boolean( this.worker ) )
will become:
if ( ! Helper.isValid( this.worker ) )

Copy link
Owner

Choose a reason for hiding this comment

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

In my opinion this is overcomplicating it. I would just do debug === true or debug === false. I think the code logic is easier to read that way.

Copy link
Contributor Author

@kaisalmen kaisalmen Apr 20, 2017

Choose a reason for hiding this comment

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

Yes, you are likely right. Do you agree with the rest (thumb up or down will do)?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, the rest is all good! 😊👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I can't finish work tonight...

Copy link
Owner

Choose a reason for hiding this comment

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

No worries!

Code related changes

THREE.OBJLoader2.WWOBJLoader2:
- Function _receiveWorkerMessage now uses a meshDescription that allows to override material or bufferGeometry or to completely disregard the mesh. THREE.OBJLoader2.WWOBJLoader2.LoadedMeshUserOverride was introduced for this.
- Allow usage of multiple callbacks per callback type
- THREE.OBJLoader2.WWOBJLoader2.PrepDataArrayBuffer and THREE.OBJLoader2.WWOBJLoader2.PrepDataFile require less mandatory parameters. Setters are introduced to handle optional things

THREE.OBJLoader2.WWOBJLoader2Director:
- Added per queue object callbacks
- Global callbacks in prepareWorkers will be specified with new object OBJLoader2.WWOBJLoader2.PrepDataCallbacks. This object is also used in both PrepData objects for defining extra per model callbacks in addition to the global ones
- Callbacks will be reset and reassigned for every run

All:
- Improve code quality, especially, replaced != or == with Boolean() or ! Boolean() where applicable
- Improve logging and comments
@kaisalmen kaisalmen force-pushed the wwobjloader2_docs branch 2 times, most recently from 97ccd6a to 052a2ec Compare April 22, 2017 12:36
kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Apr 22, 2017
Code related changes:
- Validator and its functions replace all Boolean calls. It is included in THREE.OBJLoader2.
- Versions are now defined inside OBJLoader2 and WWOBJLoader2.
- Static OBJLoader2._getValidator and OBJLoader2_buildWebWorkerCode are reached via prototype of OBJLoader2. Instance of OBJLoader2 is no longer created.

Bugfix in webgl_loader_obj2_ww_parallels:
- Fixed "Run Queue" started new run before first was completed.
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Apr 22, 2017

Ok, here we go. Boolean is gone and boolean checks are made directly inside the code (=no verifyBoolean). I only kept the two proposed functions and defined them inside Validator that is exposed via _getValidator() to WWOBJLoader2 and its sub-classes. THREE.OBJLoader2 defines the namespace, therefore the new functions can't be defined outside before. This was true for the version string all the time. It was just overridden and it is now moved inside.

Copy of commit message gives a summary:
Loader related changes:

  • Validator and its functions replace all Boolean calls. It is included in THREE.OBJLoader2.
  • Versions are now defined inside OBJLoader2 and WWOBJLoader2.
  • Static OBJLoader2._getValidator and OBJLoader2_buildWebWorkerCode are reached via prototype of OBJLoader2. Instance of OBJLoader2 is no longer created.

Bugfix in webgl_loader_obj2_ww_parallels:

  • Fixed "Run Queue" started new run before first was completed.
  • 2017-04-23: Replaced Boolean with Validator

Code related changes:
- Validator and its functions replace all Boolean calls. It is included in THREE.OBJLoader2.
- Versions are now defined inside OBJLoader2 and WWOBJLoader2.
- Static OBJLoader2._getValidator and OBJLoader2_buildWebWorkerCode are reached via prototype of OBJLoader2. Instance of OBJLoader2 is no longer created.

Bugfix in webgl_loader_obj2_ww_parallels:
- Fixed "Run Queue" started new run before first was completed.
- Replaced Boolean with Validator
@mrdoob mrdoob merged commit af446c3 into mrdoob:dev Apr 24, 2017
@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2017

Thanks!

@kaisalmen kaisalmen deleted the wwobjloader2_docs branch April 24, 2017 08:25
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