Mostly formatting changes, added a few suggestions #4
Conversation
| return p.y; | ||
| case Axis::Z_AXIS: | ||
| return p.z; | ||
| case Axis::X_AXIS: return p.x; |
There was a problem hiding this comment.
Intend one tab less to align with switch
| @@ -0,0 +1 @@ | |||
| Did I miss the performance tests? | |||
| std::unique_ptr<BBNode> BBAccelerator::split( | ||
| size_t startIndex, size_t endIndex, | ||
| std::vector<std::pair<BoundingBox, size_t>>& boxes) | ||
| { |
There was a problem hiding this comment.
All brackets go after the function
| bool hit = false; | ||
| const Vector3 invrDir = ONE_VECTOR / ray.direction; | ||
| stack[0] = m_root->left.get(); | ||
| stack[1] = m_root->right.get(); | ||
| stackBack = 2; | ||
| stackBack = 2; // General Suggestion of writting in snake_case |
| const BBNode* stack[128]; | ||
| int stackBack; | ||
| const BBNode* current; | ||
| const BBNode* stack[128]; // Move closer to assigment |
There was a problem hiding this comment.
We just want to allocate the array here
| gloss = std::make_unique<SpecularShader>( | ||
| point.shadingNormal, point.tangent, | ||
| point.bitangent, color); | ||
| // return std::... |
There was a problem hiding this comment.
More readable using if/else
| @@ -1,6 +1,7 @@ | |||
| #include "meshes/cube.h" | |||
|
|
|||
| Cube::Cube() : TriangleMesh() { | |||
| // Extract | |||
There was a problem hiding this comment.
This just makes a mesh, we just hardcode declare the mesh here
| @@ -20,6 +20,8 @@ Plane::Plane() { | |||
| m_vertices.push_back(Vertex(p2, uv2, n)); | |||
| m_vertices.push_back(Vertex(p3, uv3, n)); | |||
|
|
|||
| // At this point I know you are formating at about 100 characters, | |||
| // which I would say is the max, though I prefer 80 :P | |||
There was a problem hiding this comment.
It's random, the code has really bad formating tbh
|
|
||
|
|
||
| // if parameters align with the procedures's body, | ||
| // make it easier to separate them |
There was a problem hiding this comment.
Yes, I usually use two tabs here but you are right, it looks bad
| @@ -37,12 +40,23 @@ int main() { | |||
| std::shared_ptr<Material> whiteMatte(new MatteMaterial(whiteTexture)); | |||
| std::shared_ptr<EmissionMaterial> emissionMat(new EmissionMaterial(lightTexture, 1.3f)); | |||
|
|
|||
| // Suggestion: Assign all parameters in single variable | |||
| // Eg what does the first 500 mean for options? | |||
There was a problem hiding this comment.
That's kinda fine. We get these problems all the time. Every function has values that you usually don't know what they refer to. Of course this can be done better in this specific situation but overall it's usually fine
Feel free to accept any change you want, or none. I myself would remove a few "separate by new line" formatting changes I've made.