patternjavascriptMinor
WebGL (Three.js) simple application
Viewed 0 times
threesimplewebglapplication
Problem
I have done the source code for my lesson, which I shall publish later on my website:
You can see the working results on JSFiddle.
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 You can see the working results on JSFiddle.
Solution
This code is good for tutorial use.
Some suggestion:
You are switching over 'x','y' and 'z' :
You could simply use
You could explain some of your magical constants, like
You should probably extract your rgb codes into well named variables like cubeColor, pointLightColor etc.
This line could use some comment:
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.
is less readable than
You could merge the treatment of
you can change the above to
Though I will admit that might be too Golfic for a tutorial, you could turn the ternary into a regular
Furtermore, from a style perspective, your
should be
and
should be either
or
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;
Finally, this :
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.
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 badif ( 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.
Code Snippets
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 rotateCube( axis ) {
mesh.rotation[axis] += 0.02;
}function renderScene() {
renderer.render( scene, camera );
}
function animateScene() {
requestAnimationFrame( animateScene );
renderScene();
rotateCube( axisRotation );
}function animateScene() {
requestAnimationFrame( animateScene );
//Render scene
renderer.render( scene, camera );
rotateCube( axisRotation );
}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 );
}Context
StackExchange Code Review Q#33375, answer score: 5
Revisions (0)
No revisions yet.