Incorporating comments for code review (minecraft import)

interface/src/Application.cpp

@@ -1363,8 +1363,10 @@ void Application::exportVoxels() {
 
 void Application::importVoxels() {
     QString desktopLocation = QDesktopServices::storageLocation(QDesktopServices::DesktopLocation);
-    QString fileNameString = QFileDialog::getOpenFileName(_glWidget, tr("Import Voxels"), desktopLocation, 
+    QString fileNameString = QFileDialog::getOpenFileName(
+                _glWidget, tr("Import Voxels"), desktopLocation,
                 tr("Sparse Voxel Octree Files, Square PNG, Schematic Files (*.svo *.png *.schematic)"));
+
     QByteArray fileNameAscii = fileNameString.toAscii();
     const char* fileName = fileNameAscii.data();
     

libraries/voxels/src/Tags.cpp

@@ -1,3 +1,11 @@
+//
+//  Tags.h
+//  hifi
+//
+//  Created by Clement Brisset on 7/3/13.
+//  Copyright (c) 2013 High Fidelity, Inc. All rights reserved.
+//
+
 #include "Tags.h"
 #include <Log.h>
 
@@ -10,8 +18,7 @@ Tag::Tag(int tagId, std::stringstream &ss) : _tagId(tagId) {
     int size = ss.get() << 8 | ss.get();
 
     _name.clear();
-    for (int i = 0; i < size; ++i) {
+    for (int i = 0; i < size; ++i) {        _name += ss.get();
-        _name += ss.get();
     }
 }
 
@@ -96,7 +103,8 @@ TagList::TagList(std::stringstream &ss) :
     _size  = ss.get() << 24 | ss.get() << 16 | ss.get() << 8 | ss.get();
 
     for (int i = 0; i < _size; ++i) {
-    ss.putback(0); ss.putback(0);
+        ss.putback(0);
+        ss.putback(0);
         _data.push_back(readTag(_tagId, ss));
     }
 }
@@ -151,12 +159,12 @@ int retrieveData(std::string filename, std::stringstream &ss) {
     std::ifstream file(filename.c_str(), std::ios::binary);
 
     int type = file.peek();
-  if (0x0A == type) {
+    if (type == 0x0A) {
         ss.flush();
         ss << file;
         return 0;
     }
-  if (0x1F == type) {
+    if (type == 0x1F) {
         return ungzip(file, ss);
     }
 
@@ -173,7 +181,7 @@ int ungzip(std::ifstream &file, std::stringstream &ss) {
     }
     file.close();
 
-  if ( gzipedBytes.size() == 0 ) {
+    if (gzipedBytes.size() == 0) {
         ss << gzipedBytes;
         return 0;
     }
@@ -214,8 +222,11 @@ int ungzip(std::ifstream &file, std::stringstream &ss) {
 
         // Inflate another chunk.
         int err = inflate (&strm, Z_SYNC_FLUSH);
-    if (err == Z_STREAM_END) done = true;
+        if (err == Z_STREAM_END) {
-    else if (err != Z_OK)    break;
+            done = true;
+        } else if (err != Z_OK) {
+            break;
+        }
     }
 
     if (inflateEnd (&strm) != Z_OK) {

libraries/voxels/src/Tags.h

@@ -1,3 +1,14 @@
+//
+//  Tags.h
+//  hifi
+//
+//  Created by Clement Brisset on 7/3/13.
+//  Copyright (c) 2013 High Fidelity, Inc. All rights reserved.
+//
+
+#ifndef __hifi__Tags__
+#define __hifi__Tags__
+
 #include <cstdlib>
 #include <cstring>
 
@@ -22,120 +33,109 @@ int retrieveData(std::string filename, std::stringstream &ss);
 int ungzip(std::ifstream &file, std::stringstream &ss);
 
 class Tag {
- protected:
+public:
-  int         _tagId;
-  std::string _name;
-  
- public:
     Tag(int tagId, std::stringstream &ss);
 
     int         getTagId() const {return _tagId;}
     std::string getName () const {return _name; }
 
     static Tag* readTag(int tagId, std::stringstream &ss);
+
+protected:
+    int         _tagId;
+    std::string _name;
 };
 
 class TagByte : public Tag  {
- private:
+public:
-  int8_t _data;
-  
- public:
     TagByte(std::stringstream &ss);
 
     int8_t getData() const {return _data;}
+
+private:
+    int8_t _data;
 };
 
 class TagShort : public Tag  {
- private:
+public:
-  int16_t _data;
-  
- public:
     TagShort(std::stringstream &ss);
 
     int16_t getData() const {return _data;}
+
+private:
+    int16_t _data;
 };
 
 class TagInt : public Tag  {
- private:
+public:
-  int32_t _data;
-  
- public:
     TagInt(std::stringstream &ss);
 
     int32_t getData() const {return _data;}
+
+private:
+    int32_t _data;
 };
 
 class TagLong : public Tag  {
- private:
+public:
-  int64_t _data;
-  
- public:
     TagLong(std::stringstream &ss);
 
     int64_t getData() const {return _data;}
+
+private:
+    int64_t _data;
 };
 
 class TagFloat : public Tag  {
- public:
+public:
     TagFloat(std::stringstream &ss);
 };
 
 class TagDouble : public Tag  {
- public:
+public:
     TagDouble(std::stringstream &ss);
 };
 
 class TagByteArray : public Tag {
- private:
+public:
-  int   _size;
-  char* _data;
-  
- public:
     TagByteArray(std::stringstream &ss);
 
     int   getSize() const {return _size;}
     char* getData() const {return _data;}
+
+private:
+    int   _size;
+    char* _data;
 };
 
 class TagString : public Tag {
- private:
+public:
-  int         _size;
-  std::string _data;
-
- public:
     TagString(std::stringstream &ss);
 
     int         getSize() const {return _size;}
     std::string getData() const {return _data;}
+
+private:
+    int         _size;
+    std::string _data;
 };
 
 class TagList : public Tag {
- private:
+public:
-  int             _tagId;
-  int             _size;
-  std::list<Tag*> _data;
-  
- public:
     TagList(std::stringstream &ss);
 
     int             getTagId() const {return _tagId;}
     int             getSize () const {return _size; }
     std::list<Tag*> getData () const {return _data; }
+
+private:
+    int             _tagId;
+    int             _size;
+    std::list<Tag*> _data;
 };
 
 class TagCompound : public Tag {
- private:
+public:
-  int             _size;
-  std::list<Tag*> _data;
-  
-  // Specific to schematics file
-  int   _width;
-  int   _length;
-  int   _height;
-  char* _blocksData;
-  char* _blocksId;
-
- public:
     TagCompound(std::stringstream &ss);
 
     int             getSize      () const {return _size;      }
@@ -146,17 +146,30 @@ class TagCompound : public Tag {
     int             getHeight    () const {return _height;    }
     char*           getBlocksId  () const {return _blocksId;  }
     char*           getBlocksData() const {return _blocksData;}
+
+private:
+    int             _size;
+    std::list<Tag*> _data;
+
+    // Specific to schematics file
+    int   _width;
+    int   _length;
+    int   _height;
+    char* _blocksData;
+    char* _blocksId;
 };
 
 class TagIntArray : public Tag {
- private:
+public:
-  int  _size;
-  int* _data;
-
- public:
     TagIntArray(std::stringstream &ss);
     ~TagIntArray() {delete _data;}
 
     int  getSize() const {return _size;}
     int* getData() const {return _data;}
+
+private:
+    int  _size;
+    int* _data;
 };
+
+#endif /* defined(__hifi__Tags__) */

libraries/voxels/src/VoxelTree.cpp

@@ -1460,7 +1460,7 @@ bool VoxelTree::readFromSquareARGB32Pixels(const uint32_t* pixels, int dimension
 bool VoxelTree::readFromSchematicsFile(const char *fileName) {
     std::stringstream ss;
     int err = retrieveData(fileName, ss);
-    if (err && TAG_Compound != ss.get()) {
+    if (err && ss.get() != TAG_Compound) {
         printLog("[ERROR] Invalid schematic file.\n");
         return false;
     }
@@ -1476,7 +1476,7 @@ bool VoxelTree::readFromSchematicsFile(const char *fileName) {
     max = (max > schematics.getHeight()) ? max : schematics.getHeight();
 
     int scale = 1;
-    while (max > scale) scale *= 2;
+    while (max > scale) {scale *= 2;}
     float size = 1.0f / scale;
 
     int create = 1;
@@ -1491,7 +1491,7 @@ bool VoxelTree::readFromSchematicsFile(const char *fileName) {
                 int data = schematics.getBlocksData()[pos];
 
                 create = 1;
-                computeBlockColor(id, data, &red, &green, &blue, &create);
+                computeBlockColor(id, data, red, green, blue, create);
 
                 switch (create) {
                     case 1:
@@ -1620,8 +1620,7 @@ void VoxelTree::copyFromTreeIntoSubTree(VoxelTree* sourceTree, VoxelNode* destin
     }
 }
 
-void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int* create) {
+void VoxelTree::computeBlockColor(int id, int data, int& red, int& green, int& blue, int& create) {
-    int red = *r, green = *g, blue = *b;
 
     switch (id) {
         case   1:
@@ -1681,7 +1680,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
                 case 14: red = 165; green =  58; blue =  53; break;
                 case 15: red =  24; green =  24; blue =  24; break;
                 default:
-            *create = 0;
+                    create = 0;
                     break;
             }
             break;
@@ -1690,7 +1689,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
         case  43:
         case  98: red = 161; green = 161; blue = 161; break;
         case  44:
-        *create = 3;
+            create = 3;
 
             switch (data) {
                 case 0: red = 161; green = 161; blue = 161; break;
@@ -1702,7 +1701,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
                 case 6: red =  45; green =  22; blue =  26; break;
                 case 7: red = 195; green = 192; blue = 185; break;
                 default:
-            *create = 0;
+                    create = 0;
                     break;
             }
             break;
@@ -1722,7 +1721,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
         case 135:
         case 136:
         case 156:
-        *create = 2;
+            create = 2;
 
             switch (id) {
                 case  53:
@@ -1736,7 +1735,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
                 case 128: red = 209; green = 202; blue = 156; break;
                 case 156: red = 195; green = 192; blue = 185; break;
                 default:
-            *create = 0;
+                    create = 0;
                     break;
             }
             break;
@@ -1769,11 +1768,7 @@ void VoxelTree::computeBlockColor(int id, int data, int* r, int* g, int* b, int*
         case 172: red = 140; green =  86; blue =  61; break;
         case 173: red =   9; green =   9; blue =   9; break;
         default:
-        *create = 0;
+            create = 0;
             break;
     }
-
-    *r = red;
-    *g = green;
-    *b = blue;
 }

libraries/voxels/src/VoxelTree.h

@@ -136,7 +136,7 @@ public:
     // reads voxels from square image with alpha as a Y-axis
     bool readFromSquareARGB32Pixels(const uint32_t* pixels, int dimension);
     bool readFromSchematicsFile(const char* filename);
-    void computeBlockColor(int id, int data, int* r, int* g, int* b, int* create);
+    void computeBlockColor(int id, int data, int& r, int& g, int& b, int& create);
 
     unsigned long getVoxelCount();