trjhtr

Currently I have working code which shows two NRRD planes on the web:

I would like some help to improve my existing code.

First I will show it file by file, and then I will tell you how I think it could be improved.

We have a class InitCanvas.js which has the responsability to inicialize a Threejs scene, with its NRRD model, into a HTML div:

// this class handles the load and the canva for a nrrd
// Using programming based on prototype: https://javascript.info/class
// This class should be improved:
// - Canvas Width and height

InitCanvas = function (IdDiv, Filename) 


 this.IdDiv = IdDiv;
 this.Filename = Filename


InitCanvas.prototype = 

 constructor: InitCanvas,

 init: function () 

 this.container = document.getElementById(this.IdDiv);

 // this should be changed.
 debugger;
 this.container.innerHeight = 600;
 this.container.innerWidth = 800;

 //These statenments should be changed to improve the image position
 this.camera = new THREE.PerspectiveCamera(60, this.container.innerWidth / this.container.innerHeight, 0.01, 1e10);
 this.camera.position.z = 300;

 let scene = new THREE.Scene();
 scene.add(this.camera);

 // light

 let dirLight = new THREE.DirectionalLight(0xffffff);
 dirLight.position.set(200, 200, 1000).normalize();

 this.camera.add(dirLight);
 this.camera.add(dirLight.target);


 // read file

 let loader = new THREE.NRRDLoader();
 loader.load(this.Filename, function (volume) 

 //z plane
 let sliceZ = volume.extractSlice('z', Math.floor(volume.RASDimensions[2] / 4));

 debugger;
 this.container.innerWidth = sliceZ.iLength;
 this.container.innerHeight = sliceZ.jLength;

 sliceZ.mesh.material.color.setRGB(0,1,1);


 console.log('Our slice is: ', sliceZ);

 scene.add(sliceZ.mesh);
 .bind(this));


 this.scene = scene;

 // renderer

 this.renderer = new THREE.WebGLRenderer(alpha: true);
 this.renderer.setPixelRatio(this.container.devicePixelRatio);
 debugger;
 this.renderer.setSize(this.container.innerWidth, this.container.innerHeight);

 // add canvas in container
 this.container.appendChild(this.renderer.domElement);

 ,

 animate: function () 

 this.renderer.render(this.scene, this.camera);
 

I think there should be getters and setters for class' properties, to avoid accessing them with dot notation directly.

It could be better to put the constructor function directly as:

constructor: function()

instead of defining it first, and then calling it through the InitCanvas alias.

It would be better to put the div and canvas size in the CSS if it is static, as it currently is.

It could be better to name the parameters which PerspectiveCamera receives as:

let fieldOfView = 60, aspectRatio = this.container.innerWidth / this.container.innerHeight, near = 0.01, far = 1e10;

Maybe it could be better to put the asynchronous function loader.load() at file's ending part, because as it is could confuse those who read it and think that it is being a synchronous call. I mean, despite the fact that it is before renderer, renderer will execute before, because of loader.load() takes time to resolve the file loading.

Could be better if we encapsulate Math.floor(volume.RASDimensions [2] / 4)into a variable that explains why it is used, however I took that line from the official threejs github repo and I still not understanding why it is needed.

Maybe we should handle the case when the NRRD model does not load. Would be better with an asynchronous call or with a Promise, the error handling?

Then, we have logic.js whose responsability is to bootstrap both canvas and make them to animate and respond to user clicks:

if (!Detector.webgl) Detector.addGetWebGLMessage();

// global variables for this scripts
let OriginalImg,
 SegmentImg;

var mouse = new THREE.Vector2();
var raycaster = new THREE.Raycaster();
var mousePressed = false;
var clickCount = 0;


init();
animate();


// initilize the page
function init() 
 let filename = "models/nrrd/columna01.nrrd"; // change your nrrd file
 let idDiv = 'original';
 OriginalImg = new InitCanvas(idDiv, filename);
 OriginalImg.init();
 console.log(OriginalImg);

 filename = "models/nrrd/columnasegmentado01.nrrd"; // change your nrrd file
 idDiv = 'segment';
 SegmentImg = new InitCanvas(idDiv, filename);
 SegmentImg.init();


let originalCanvas = document.getElementById('original');
originalCanvas.addEventListener('mousedown', onDocumentMouseDown, false);
originalCanvas.addEventListener('mouseup', onDocumentMouseUp, false);


function onDocumentMouseDown(event) 
 mousePressed = true;

 clickCount++;

 mouse.x = ( ( event.clientX - OriginalImg.renderer.domElement.offsetLeft ) / OriginalImg.renderer.domElement.clientWidth ) * 2 - 1;
 mouse.y = - ( ( event.clientY - OriginalImg.renderer.domElement.offsetTop ) / OriginalImg.renderer.domElement.clientHeight ) * 2 + 1

 console.log('Mouse x position is: ', mouse.x, 'the click number was: ', clickCount);
 console.log('Mouse Y position is: ', mouse.y);

 raycaster.setFromCamera(mouse.clone(), OriginalImg.camera);
 var objects = raycaster.intersectObjects(OriginalImg.scene.children);

 console.log(objects);


function onDocumentMouseUp(event) 
 mousePressed = false



function animate() 


 requestAnimationFrame(animate);
 OriginalImg.animate();
 SegmentImg.animate();


 

Maybe we should declare all variables as let, in a line.

It could be better if we rename originalCanvas with originalDiv, because of it gets a HTML div. In fact it does contains the threejs canvas, but itself is a div.

I would let the addEventListeners as they are, I mean grouped before the functions' definitions because it is easier to read.

It could be better to encapsulate in onMouseDocumentDown() the expression OriginalImg.renderer.donElement in a variable called originalRendererDOM.

mousePressed should be removed because it is not used, as well as onDocumentMouseUp is neither used.

I do like the symetry which has both previous files, because both have init() and animate() functions.

Then, we have index.html which has the divs to contains the threejs canvas:

<!DOCTYPE html>
<html lang="en">
 <head>
 <title>Prototype: three.js without react.js</title>
 <meta charset="utf-8">
 <meta name="viewport" content="width=device-width, user-scalable=no, minimum-scale=1.0, maximum-scale=1.0">
 <link rel="stylesheet" href="css/styles.css">

 <!-- load the libraries and js -->
 <script src="js/libs/three.js"></script>

 <script src="js/Volume.js"></script>
 <script src="js/VolumeSlice.js"></script>
 <script src="js/loaders/NRRDLoader.js"></script>

 <script src="js/Detector.js"></script>
 <script src="js/libs/stats.min.js"></script>
 <script src="js/libs/gunzip.min.js"></script>
 <script src="js/libs/dat.gui.min.js"></script>

 <script src="js/InitCanvas.js"></script>


 </head>

 <body>
 <div id="info">
 <h1>Prototype: three.js without react.js</h1> 
 </div>

 <!-- two canvas -->
 <div class="row">
 <div class="column" id="original">
 </div>

 <div class="column" id="segment">

 </div>
 </div> 

 <script src="js/logic.js"></script>

 </body>
</html>

I think it is better to aisolate libraries scripts and those in the project, so then I put the first in head and the second in body, is it correct?

In addition we have styles.css

body 
 font-family: Monospace;
 margin: 0px;
 overflow: hidden;


#info 
 color: rgb(137, 145, 192);
 position: absolute;
 top: 10px;
 width: 100%;
 text-align: center;
 z-index: 5;
 display:block;


.column 
 float: left;
 width: 50%;


/* Clear floats after the columns */
.row:after 
 content: "";
 display: table;
 clear: both;


canvas 
 width: 200px;
 height: 200px;
 margin: 100px;
 padding: 0px;
 position: static; /* fixed or static */
 top: 100px;
 left: 100px;

It could be improved if we group the HTML tags, body and canvas on the top part, then the css classes and finally the css ids.

Finally, having a look to the architecture:

I think it could be better to aisolate project's source javascript and put it into a different folder than the external javascript.

What do you think?