Developmentnow it's rendring #37

mahesh0537 · 2022-07-27T20:21:41Z

No description provided.

prateekgargX · 2022-07-27T23:57:18Z

utilities/include/vec4.hpp

            return std::acos(dot(v1,v2)/(v1.norm()*v2.norm()));
        }
-        CONSTEXPR_CMATH vec4 unit() const{
+        constexpr vec4 unit() const{
            return *this/this->norm();
        }



Isn't that supposed to be taken care of by the std library we are using?

wermos · 2022-07-27T23:41:29Z

Rendera/include/cube.hpp

@@ -41,7 +58,8 @@ class Cube
    vec3 m_position;
    vec3 m_a, m_b, m_c;
    Material m_material;
-    static const double m_epsilon = 1e-5;
+    vec3 int_pt;
+    const double m_epsilon = 1e-5;


Why did you make m_epsilon a non-static member?

wermos · 2022-07-27T23:41:47Z

Rendera/include/cube.hpp

@@ -7,6 +7,23 @@
 #include "ray.hpp"
 #include "material.hpp"

+
+//debug


What does this comment mean?

wermos · 2022-07-27T23:42:06Z

Rendera/include/cube.hpp

@@ -7,6 +7,23 @@
 #include "ray.hpp"
 #include "material.hpp"

+
+//debug
+class HitInfo{


The HitInfo struct should be in its own header file.

Why is this a class? It should be a struct.

wermos · 2022-07-27T23:42:56Z

Rendera/include/cube.hpp

+    int depth;
+
+    public:
+        HitInfo(bool if_hit, vec3 v, Material m, ray r, int d):if_hit{if_hit}, dist_v{v}, mat{m}, r{r}, depth{d}{};


Members of structs are public by default. Also, this constructor is not needed. The compiler will auto-generate it for you.

wermos · 2022-07-27T23:43:08Z

Rendera/include/cube.hpp

+        HitInfo(bool if_hit, vec3 v, Material m, ray r, int d):if_hit{if_hit}, dist_v{v}, mat{m}, r{r}, depth{d}{};
+
+};
+


Why are there so many blank spaces?

wermos · 2022-07-27T23:54:12Z

Rendera/src/main.cpp


-    sc.render();
-
    int max_col = 255;
    std::ofstream image("sphere.ppm");


Why did you add the ppm code back in, and remove the png/jpg code?

wermos · 2022-07-27T23:54:40Z

utilities/include/vec3.hpp

@@ -35,6 +35,8 @@ class alignas(ALIGN_WIDTH) vec3 : public vec4{
        constexpr explicit vec3(Utype x) : vec4{x,x,x,0} {}
        constexpr vec3(Utype x,Utype y,Utype z) : vec4{x,y,z,0} {}
        explicit vec3(xsimd::batch<Utype,UArch> x) : vec4{x} {}
+        constexpr vec3(vec4 a): vec4{a.x(), a.y(), a.z(), a.w()}{}
+        vec3() = default;


Why is this constructor necessary?

wermos · 2022-07-27T23:55:01Z

utilities/include/vec3.hpp

@@ -35,6 +35,8 @@ class alignas(ALIGN_WIDTH) vec3 : public vec4{
        constexpr explicit vec3(Utype x) : vec4{x,x,x,0} {}
        constexpr vec3(Utype x,Utype y,Utype z) : vec4{x,y,z,0} {}
        explicit vec3(xsimd::batch<Utype,UArch> x) : vec4{x} {}
+        constexpr vec3(vec4 a): vec4{a.x(), a.y(), a.z(), a.w()}{}


What are you trying to do here?

wermos · 2022-07-27T23:55:36Z

utilities/include/vec4.hpp

@@ -7,7 +7,6 @@
 #include <type_traits> // for std::is_constant_evaluated

 #include <xsimd/xsimd.hpp>
-


Why did you remove this blank line? It exists to provide a little separation between a library include and our own project's include.

wermos · 2022-07-27T23:57:42Z

utilities/include/vec4.hpp

-        CONSTEXPR_CMATH Utype norm() const{
+        constexpr Utype norm() const{
            return std::sqrt(dot(*this,*this));
        }

-        CONSTEXPR_CMATH Utype angle(const vec4& v1,const vec4& v2){
+        constexpr Utype angle(const vec4& v1,const vec4& v2){
            return std::acos(dot(v1,v2)/(v1.norm()*v2.norm()));
        }
-        CONSTEXPR_CMATH vec4 unit() const{
+        constexpr vec4 unit() const{


Why did you change these functions from the CONSTEXPR_CMATH macro to constexpr?

As of C++20, the <cmath> functions are not constexpr and hence cannot be used in a constant expression context. However, GCC implements the <cmath> functions as constexpr in their implementation of the standard library as a non-standard extension. That's why the CONSTEXPR_CMATH macro is necessary.

wermos · 2022-07-28T12:33:30Z

Rendera/include/cube.hpp

@@ -7,6 +7,23 @@
 #include "ray.hpp"
 #include "material.hpp"

+
+//debug
+class HitInfo{


Why is this a class? It should be a struct.

wermos · 2022-07-28T12:35:25Z

Rendera/include/cube.hpp

@@ -41,7 +58,8 @@ class Cube
    vec3 m_position;
    vec3 m_a, m_b, m_c;
    Material m_material;
-    static const double m_epsilon = 1e-5;
+    vec3 int_pt;
+    const double m_epsilon = 1e-5;


Why did you make this a non-static member? Now every Cube object will have its own copy of m_epsilon.

Make it static constexpr double m_epsilon = 1e-5;

wermos · 2022-07-28T12:37:45Z

Rendera/include/scene.hpp

@@ -4,7 +4,7 @@
 #include "sphere.hpp"
 #include "intersection.hpp"
 #include "camera.hpp"
-
+#include "cube.hpp"
 class Hitinfo{


This is a One Definition Rule (ODR) violation. You can only have definition for a class. Why did you define HitInfo multiple times?

wermos · 2022-07-28T14:47:13Z

Rendera/include/cube.hpp

@@ -41,7 +58,8 @@ class Cube
    vec3 m_position;
    vec3 m_a, m_b, m_c;
    Material m_material;
-    static const double m_epsilon = 1e-5;
+    vec3 int_pt;
+    const double m_epsilon = 1e-5;


Make it static constexpr double m_epsilon = 1e-5;

wermos · 2022-07-28T14:58:13Z

Rendera/include/scene.hpp

@@ -54,21 +55,25 @@ color lighting(Material mat, ray cam_ray, ray normal, vec3 point, vec3 light_src

 class Scene{
    public:
-        int n_sphere;
+        int n_sphere = 1;
+        int n_cube = 0;
        uint16_t img_width;


Make this a 32 bit int (std::uint32_t). To use these types, you should also include <cstddef>.

Same for img_height.

wermos · 2022-07-28T14:58:50Z

Rendera/include/scene.hpp

@@ -54,21 +55,25 @@ color lighting(Material mat, ray cam_ray, ray normal, vec3 point, vec3 light_src

 class Scene{
    public:
-        int n_sphere;
+        int n_sphere = 1;
+        int n_cube = 0;
        uint16_t img_width;
        uint16_t img_height;
        Hitinfo *hitinfo = new Hitinfo[img_height*img_width];


Remove this line because we are going depth-first.

wermos · 2022-07-28T15:00:00Z

Rendera/include/scene.hpp

@@ -85,29 +90,34 @@ class Scene{

        }
        ~Scene(){


We don't a manually defined destructor because we are removing the array of HitInfo structs.

mahesh0537 and others added 6 commits July 27, 2022 23:04

scene update

d3d4221

Update README.md

8aa70dc

Update README.md

4a78312

debug done

1f39274

debugged

e4318ad

cude added in scene

9a675af

prateekgargX reviewed Jul 27, 2022

View reviewed changes

wermos requested changes Jul 27, 2022

View reviewed changes

wermos requested changes Jul 28, 2022

View reviewed changes

mahesh0537 added 2 commits July 30, 2022 17:11

Merge branch 'wermos:development' into development

030513c

Update README.md

c47f76e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Developmentnow it's rendring #37

Developmentnow it's rendring #37

mahesh0537 commented Jul 27, 2022

prateekgargX Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 28, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 27, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

wermos Jul 28, 2022

		HitInfo(bool if_hit, vec3 v, Material m, ray r, int d):if_hit{if_hit}, dist_v{v}, mat{m}, r{r}, depth{d}{};

		};

		@@ -7,7 +7,6 @@
		#include <type_traits> // for std::is_constant_evaluated

		#include <xsimd/xsimd.hpp>

Developmentnow it's rendring #37

Are you sure you want to change the base?

Developmentnow it's rendring #37

Conversation

mahesh0537 commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment