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.

\$\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.

\$\endgroup\$

Your Answer

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