4
\$\begingroup\$

I have done the source code for my lesson, which I shall publish later on my website:

<script src="three.min.js"></script>
<script>
var renderer, scene, camera, geometry, material, mesh, light, axisRotation;
function createScene() {
 renderer = new THREE.WebGLRenderer();
 renderer.setSize( window.innerWidth, window.innerHeight );
 document.body.appendChild( renderer.domElement );
 scene = new THREE.Scene();
 camera = new THREE.PerspectiveCamera( 45, window.innerWidth / window.innerHeight, 1, 10000 );
 camera.position.set( 3, 3, 3 );
 camera.lookAt( scene.position );
 scene.add( camera );
 geometry = new THREE.CubeGeometry( 1, 1, 1 );
 material = new THREE.MeshLambertMaterial( { color: 0xff0000 } );
 mesh = new THREE.Mesh( geometry, material );
 scene.add( mesh );
 light = new THREE.PointLight( 0xffff00 );
 light.position.set( 10, 10, 10 );
 scene.add( light );
}
function rotateCube( axis ) {
 switch ( axis ) {
 case 'x':
 mesh.rotation.x += 0.02;
 break;
 case 'y':
 mesh.rotation.y += 0.02;
 break;
 case 'z':
 mesh.rotation.z += 0.02;
 break;
 }
}
function toggleObjectWireframe() {
 material.wireframe = !material.wireframe;
}
function renderScene() {
 renderer.render( scene, camera );
}
function animateScene() {
 requestAnimationFrame( animateScene );
 renderScene();
 rotateCube( axisRotation );
}
function getChar( event ) {
 var inputChar = null;
 if ( event.which === null ) {
 if ( event.keyCode < 32 ) inputChar = null;
 else inputChar = String.fromCharCode( event.keyCode );
 }
 if ( event.which != 0 && event.charCode != 0 ) {
 if ( event.which < 32 ) inputChar = null;
 else inputChar = String.fromCharCode( event.which );
 }
 if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
 axisRotation = inputChar;
 }
 else if ( inputChar == 'w' ) toggleObjectWireframe();
 else inputChar = null;
}
function initWebGLApp() {
 axisRotation = 'x';
 createScene();
 animateScene();
}
function setCssStyle() {
 document.body.style.width = "100%";
 document.body.style.height = "100%";
 document.body.style.margin = 0;
 document.body.style.padding = 0;
 document.querySelector('canvas').style.display = 'block';
}
window.onload = function() {
 initWebGLApp();
 setCssStyle();
 document.onkeypress = getChar;
}
</script>

You can see the working results on JSFiddle.

glampert
17.3k4 gold badges31 silver badges89 bronze badges
asked Oct 28, 2013 at 8:29
\$\endgroup\$
0

1 Answer 1

5
\$\begingroup\$

This code is good for tutorial use.

Some suggestion:

You are switching over 'x','y' and 'z' :

function rotateCube( axis ) {
 switch ( axis ) {
 case 'x':
 mesh.rotation.x += 0.02;
 break;
 case 'y':
 mesh.rotation.y += 0.02;
 break;
 case 'z':
 mesh.rotation.z += 0.02;
 break;
 }
}

You could simply use axis straight.

function rotateCube( axis ) {
 mesh.rotation[axis] += 0.02;
}

You could explain some of your magical constants, like 10000, it is those values that always throw me off when I experiment with this 3d library.

You should probably extract your rgb codes into well named variables like cubeColor, pointLightColor etc.

This line could use some comment: requestAnimationFrame( animateScene );

One liner functions that are called only once are wrong, I'd much rather you keep those lines in-line with a comment above them as to what is going on.

function renderScene() {
 renderer.render( scene, camera );
}
function animateScene() {
 requestAnimationFrame( animateScene );
 renderScene();
 rotateCube( axisRotation );
}

is less readable than

function animateScene() {
 requestAnimationFrame( animateScene );
 //Render scene
 renderer.render( scene, camera );
 rotateCube( axisRotation );
}

You could merge the treatment of keyCode and which.

var inputChar = null;
if ( event.which === null ) {
 if ( event.keyCode < 32 ) inputChar = null;
 else inputChar = String.fromCharCode( event.keyCode );
}
if ( event.which != 0 && event.charCode != 0 ) {
 if ( event.which < 32 ) 
 inputChar = null;
 else 
 inputChar = String.fromCharCode( event.which );
}

you can change the above to

var keyCode = event.keyCode || event.which,
 inputChar = keyCode < 32 ? null : String.fromCharCode( keyCode );

Though I will admit that might be too Golfic for a tutorial, you could turn the ternary into a regular if statement to keep things easy.

Furtermore, from a style perspective, your if's look bad

 if ( event.keyCode < 32 ) inputChar = null;
 else inputChar = String.fromCharCode( event.keyCode );

should be

 if ( event.keyCode < 32 ) 
 inputChar = null;
 else 
 inputChar = String.fromCharCode( event.keyCode );

and

if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
 axisRotation = inputChar;
}
else if ( inputChar == 'w' ) toggleObjectWireframe();
else inputChar = null;

should be either

if ( ~['x', 'y', 'z'].indexOf( inputChar ) ) {
 axisRotation = inputChar;
}
else if {
 ( inputChar == 'w' ) toggleObjectWireframe();
}
else { 
 inputChar = null;
}

or

if ( ~['x', 'y', 'z'].indexOf( inputChar ) )
 axisRotation = inputChar;
else if ( inputChar == 'w' ) 
 toggleObjectWireframe();
else 
 inputChar = null;

I would advocate the second approach.

Furthermore, the following 2 lines seem pointless, I took them out in the fiddle, and it works just fine;

document.body.style.width = "100%";
document.body.style.height = "100%";

Finally, this :

document.onkeypress = getChar;

is old skool, and should be avoided in general, I understand it is easy for a tutorial, but you should at least put a line of comment that this is not ideal.

All in all, I like your code, even though this review got a bit lengthy.

answered Jan 10, 2014 at 13:42
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.