From fdbe0f4c74b73f2f3ea7edeedaae0ed6af39bca4 Mon Sep 17 00:00:00 2001 From: Jules Fouchy Date: Thu, 23 May 2024 19:30:59 +0200 Subject: [PATCH] Remove comments --- content/bob.py | 26 +++++++ content/evaluation/Adelie F.md | 58 --------------- content/evaluation/Alexandre M.md | 76 -------------------- content/evaluation/Baptiste J.md | 81 --------------------- content/evaluation/Camille L.md | 34 --------- content/evaluation/Coralie B.md | 97 ------------------------- content/evaluation/Corentin P.md | 52 -------------- content/evaluation/Elies B.md | 70 ------------------ content/evaluation/Eliott D.md | 81 --------------------- content/evaluation/Enora C.md | 87 ----------------------- content/evaluation/Enzo B.md | 51 ------------- content/evaluation/Erwan T.md | 44 ------------ content/evaluation/Florian T.md | 21 ------ content/evaluation/Guilhem D.md | 33 --------- content/evaluation/Heidi L.md | 52 -------------- content/evaluation/Laura G.md | 16 ----- content/evaluation/Lea T.md | 61 ---------------- content/evaluation/Lena C.md | 87 ----------------------- content/evaluation/Leo S.md | 34 --------- content/evaluation/Lila C.md | 58 --------------- content/evaluation/Lilou B.md | 47 ------------ content/evaluation/Logan A.md | 9 --- content/evaluation/Lou B.md | 27 ------- content/evaluation/Lucie G.md | 75 -------------------- content/evaluation/Mailis B.md | 27 ------- content/evaluation/Marianne K.md | 114 ------------------------------ content/evaluation/Marion B.md | 33 --------- content/evaluation/Matheo P.md | 114 ------------------------------ content/evaluation/Matteo M.md | 75 -------------------- content/evaluation/Matthieu M.md | 16 ----- content/evaluation/Maxime V.md | 76 -------------------- content/evaluation/Melodie K.md | 22 ------ content/evaluation/Nina G.md | 47 ------------ content/evaluation/Romane M.md | 22 ------ content/evaluation/Salome T.md | 52 -------------- content/evaluation/Sara L.md | 61 ---------------- content/evaluation/Segolene L.md | 21 ------ content/evaluation/Tancrede G.md | 44 ------------ content/evaluation/Tanya F.md | 51 ------------- content/evaluation/Thomas G.md | 37 ---------- content/evaluation/Valentin G.md | 9 --- content/evaluation/Zacharie P.md | 30 -------- 42 files changed, 26 insertions(+), 2102 deletions(-) create mode 100644 content/bob.py diff --git a/content/bob.py b/content/bob.py new file mode 100644 index 000000000..c9668c423 --- /dev/null +++ b/content/bob.py @@ -0,0 +1,26 @@ +import os + + +def truncate_file_after_line_70(file_path): + with open(file_path, "r", encoding="utf-8", errors="ignore") as file: + lines = file.readlines() + + if len(lines) > 70: + lines = lines[:70] + + with open(file_path, "w", encoding="utf-8", errors="ignore") as file: + file.writelines(lines) + + +def process_files_in_directory(directory): + for filename in os.listdir(directory): + file_path = os.path.join(directory, filename) + if os.path.isfile(file_path): + truncate_file_after_line_70(file_path) + + +# Specify the directory containing the files +directory = os.path.dirname(os.path.abspath(__file__)) + "/evaluation/" + +# Process each file in the directory +process_files_in_directory(directory) diff --git a/content/evaluation/Adelie F.md b/content/evaluation/Adelie F.md index 0f977d541..024ec8a1f 100644 --- a/content/evaluation/Adelie F.md +++ b/content/evaluation/Adelie F.md @@ -68,61 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -void Boids::draw(const p6::Context& ctx, const float square_radius, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time) -{ - for (auto& i : m_boids) - { - i.draw(ctx, mesh, viewmatrix, program, time); - i.move(square_radius, maxspeed, minspeed, height); - } -} -``` -Le fait que la méthode `draw()` fasse à la fois draw et move peut être confusant. Il faudrait lui donner un nom plus explicite, ou encore mieux, séparer ça en deux fonctions draw() et move(). - -```cpp -public: - Boids() = default; - Boids(const std::vector vec); - Boids(const int number); - std::vector getVect() const; // get le vector de Boids - int numberOfBoids() const; - Boid getBoid(const int id); - void addBoid(const Boid& boid); - void deleteBoid(); - void changeSize(const int boids_number); - void draw(const p6::Context& ctx, const float square_radius, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time); // dessine tous les Boids - void alignement(const float neighbor_dist); - void cohesion(const float neighbor_dist); - void separation(const float avoid_factor); - void update(const p6::Context& ctx, const int boids_number, const float square_radius, const float neighbor_dist, const float avoid_factor, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time); - std::vector otherBoids(const Boid& b); -}; -``` -La plupart de ces méthodes pourraient être privées. - -```cpp -Scene scene; -scene.Init(ctx); -``` -Plutôt qu'une méthode Init faites ça directement dans le constructeur. - -```cpp -void Scene::cleanupRessources() -{ - m_fish_program.deleteTextureBufferArray(); - m_cube_program.deleteTextureBufferArray(); - m_arpenteur_program.deleteTextureBufferArray(); - m_seaweed_program.deleteTextureBufferArray(); - m_shark_program.deleteTextureBufferArray(); - m_cube.DeleteVboVao(); - m_fish.DeleteVboVao(); - m_fishlow.DeleteVboVao(); - m_fishverylow.DeleteVboVao(); - m_shark.DeleteVboVao(); - m_seaweed.DeleteVboVao(); - m_mushroom.DeleteVboVao(); -} -``` -Pas besoin de ces méthodes puisque vous avez mis un destructeur aux classes VBO et VAO. Le destructeur sera appelé automatiquement, c'est tout l'intérêt ! \ No newline at end of file diff --git a/content/evaluation/Alexandre M.md b/content/evaluation/Alexandre M.md index d1fe3f0a7..38930b8cc 100644 --- a/content/evaluation/Alexandre M.md +++ b/content/evaluation/Alexandre M.md @@ -68,79 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -inline glm::vec3 get_position() { return m_position; }; -``` -Cette méthode peut-être marquée `const`: -```cpp -inline glm::vec3 get_position() const { return m_position; }; -``` - -```cpp -if(m_position.x > 45) m_position.x = -45; -``` -Évitez d'hardcoder la taille du cube, surtout qu'elle est utilisée à la fois dans l'arpenteur et dans les boids. Faites plutôt une variable dans un .hpp que vous pouvez inclure là où vous en avez besoin. - -```cpp -void Boid::update(float dt, float aspect_ratio, -std::vector boids, float avoidance_multiplicator, float alignement_multiplicator, float centering_multiplicator) -{ - glm::vec3 force = compute_forces(boids, avoidance_multiplicator, alignement_multiplicator, centering_multiplicator); -``` -Vous pouvez faire une struct pour grouper les différents multiplicator. Comme ça le jour où vous voudrez en rajouter un, il suffira de changer la struct, plutôt que de devoir changer la signature de toutes les fonctions qui utilisent ces multiplicators (update(), compute_forces(), etc) - -```cpp - value.x = -2*velocity.x; - value.y = -2*velocity.y; - value.z = -2*velocity.z; -``` -vous pouvez faire les opérations sur es vecteurs directement: -```cpp - value = -2*velocity; -``` - -```cpp -class Light -{ - private: - glm::vec3 m_position; - glm::vec3 m_intensity; - public: - Light(glm::vec3 pos, glm::vec3 intensity) : m_position(pos), m_intensity(intensity){}; - inline glm::vec3 get_position() const {return m_position;}; - inline glm::vec3 get_intensity() {return m_intensity;}; - inline void set_position(glm::vec3 pos) {m_position = pos;}; - inline void set_intensity(glm::vec3 intensity) {m_intensity = intensity;}; -}; -``` -Une classe qui a juste des getters et setters triviaux, ça peut être écrit plus simplement comme une struct : -```cpp -struct Light -{ - glm::vec3 position; - glm::vec3 intensity; -}; -``` - -```cpp - Vbo cube_vbo(1); - cube_vbo.gen(); -``` -Si une méthode doit toujours être appelée après la construction de l'objet, alors elle devrait directemennt faire partie du constructeur. Ca évite le risque d'oublier de l'appeler. Une fois qu'un objet est construit `VBO cube_vbo(1);`il est censé être déjà valide et utilisable. - -Vous avez beaucoup de code mort, par ex -```cpp - Force avoidance(params._multiplicator_avoidance); //Tendance à augmenter la distance inter-boids - Force alignement(params._multiplicator_alignement); //Tendance à former des gros groupes facilement - Force centering(params._multiplicator_centering); //Tendance à diminuer le rayon d'un groupe de boid -``` -qui ne sont utilisées nulle part. - -```cpp -std::vector e = flock.get_boids(); -``` -Attention vous faites une copie qui peut être coûteuse si il y a beaucoup de boids ! Prenez plutôt une référence : -```cpp -const std::vector& e = flock.get_boids(); -``` \ No newline at end of file diff --git a/content/evaluation/Baptiste J.md b/content/evaluation/Baptiste J.md index 149c8b391..eae25ba03 100644 --- a/content/evaluation/Baptiste J.md +++ b/content/evaluation/Baptiste J.md @@ -68,84 +68,3 @@ import LessonLink from "@site/components/LessonLink" - 🌞 Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsdown: -```cpp - if (position.x <= params.bounds.x[0] + params.bounds_force_range) - { - float approach = position.x - params.bounds.x[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(map_to_max_speed(approach_normalised, params.max_speed), 0, 0); - } - - if (position.x >= params.bounds.x[1] - params.bounds_force_range) - { - float approach = params.bounds.x[1] - position.x; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(map_to_max_speed(approach_normalised, params.max_speed), 0, 0); - } - - if (position.y <= params.bounds.y[0] + params.bounds_force_range) - { - float approach = position.y - params.bounds.y[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(0, map_to_max_speed(approach_normalised, params.max_speed), 0); - } - - if (position.y >= params.bounds.y[1] - params.bounds_force_range) - { - float approach = params.bounds.y[1] - position.y; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(0, map_to_max_speed(approach_normalised, params.max_speed), 0); - } - - if (position.z <= params.bounds.z[0] + params.bounds_force_range) - { - float approach = position.z - params.bounds.z[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(0, 0, map_to_max_speed(approach_normalised, params.max_speed)); - } - - if (position.z >= params.bounds.z[1] - params.bounds_force_range) - { - float approach = params.bounds.z[1] - position.z; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(0, 0, map_to_max_speed(approach_normalised, params.max_speed)); - } -``` -Beaucoup de code dupliqué. - -:thumbsdown: -```cpp -GLuint texture_id; -``` -Vous auriez faire une classe wrapper pour Texture, comme vous l'avez fait pour VAO et VBO. Ça vous aurait évité d'oublier de détruire la texture dans le destructeur de Mesh. - -:thumbsup: -```cpp -{ - add_uniform_variable("uMVPMatrix"); - add_uniform_variable("uMVMatrix"); - add_uniform_variable("uNormalMatrix"); - - // add the texture uniform - add_uniform_variable("uText"); - - // add the light uniform - add_uniform_variable("uKd"); - add_uniform_variable("uKs"); - add_uniform_variable("uShininess"); - - add_light_uniforms(0); - add_light_uniforms(1); - add_light_uniforms(2); - - setup_mesh(); -} -``` -C'est très bien d'avoir fait des fonctions pour simplifier l'écriture de votre constructeur - -```cpp -void Mesh::change_mesh(const std::filesystem::path& obj_path, const std::filesystem::path& texture_path) -``` -Ce n'est pas idéal de reloader le mesh à chaque fois que vous voulez en changer, car ça prend du temps. Il aurait été plus performant de loader à l'avance tous les meshs que vous comptez utiliser, et de juste choisir le mesh à utiliser en fonction d'un booléen, ou d'un pointeur vers le mesh courant. \ No newline at end of file diff --git a/content/evaluation/Camille L.md b/content/evaluation/Camille L.md index 70f243d2f..059ae6947 100644 --- a/content/evaluation/Camille L.md +++ b/content/evaluation/Camille L.md @@ -68,37 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -public: - Boid(int sizeBoid); - - void display(glm::mat4 &ModelMatrix, glm::mat4 &ViewMatrix, glm::mat4 &ProjMatrix, - GLint uMVPMatrix, GLint uMVMatrix, GLint uNormalMatrix, Model3D &model) const; - void updatePosition(); - vec getPosition() const; - glm::vec3 getColor() const; - vec getDirection() const; - vec calculateSeparationForce(const std::vector& boids, float separation); - vec calculateCohesionForce(const std::vector& boids, float cohesion); - vec calculateAlignmentForce(const std::vector& boids, float alignment); - void applySteeringForces(const std::vector& boids, float separation, float cohesion, float alignment); -``` -La plupart de ces fonctions auraient ppu être privées - -```cpp -void ListeBoids::display(glm::mat4 &ModelMatrix, glm::mat4 &ViewMatrix, glm::mat4 &ProjMatrix, - const GLint uMVPMatrix, const GLint uMVMatrix, const GLint uNormalMatrix, Model3D &model) const -{ - for (const Boid& b : listeBoids) - { - b.display(ModelMatrix, ViewMatrix, ProjMatrix, uMVPMatrix, uMVMatrix, uNormalMatrix, model); - } -} -``` -Vous auriez pu faire une struct pour grouper tous ces paramètres pour éviter de les dupliquer entre ListeBoids::display() et Boid::display(). - -```cpp -void Model3D::drawObject(glm::mat4 &ViewMatrix, glm::mat4 &ModelMatrix, glm::mat4 &ProjMatrix, -``` -Passez par const& quand vous n'avez pas besoin de modifier l'objet \ No newline at end of file diff --git a/content/evaluation/Coralie B.md b/content/evaluation/Coralie B.md index ab7b5f4fe..a2fea2c5f 100644 --- a/content/evaluation/Coralie B.md +++ b/content/evaluation/Coralie B.md @@ -68,100 +68,3 @@ import LessonLink from "@site/components/LessonLink" - 🌞 Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsdown: -```cpp -Object objBoid; -objBoid.setInputfile("jellyfishvf"); -objBoid.load(); -``` -Ça aurait été mieux de faire tout ça dans le constructeur de Object. Ça évite de le dupliquer à chaque fois qu'on fait un Object : -```cpp -Object objBoid("jellyfishvf"); -``` - -:thumbsdown: -Ça aurait été bien d'alléger le *main()* en rangeant le code dans diverses classes / fonctions. - -:thumbsup: -```cpp -if (LOD) -{ - vaoLowBoid.bind(); - textureLowBoid.send(uTexture); - textureLowBoid.bind(); -} -else -{ - vaoBoid.bind(); - textureBoid.send(uTexture); - textureBoid.bind(); -}; -``` -C'est très bien d'avoir loadé à l'avance tous les meshs possibles pour le LOD (ça économise des performances, plutôt que de changer le mesh à chaque fois que le LOD change). -:thumbsdown: -Pour éviter la duplication de code, tu aurais pu faire comme ceci : -```cpp -// D'abord on choisit le bon vao et la bonne texture -const auto& vao = LOD ? vaoLowBoid : vaoBoid; -const auto& texture = LOD ? textureLowBoid : textureBoid; -// Puis on fait le code potentiellement long et compliqué -vao.bind(); -texture.send(uTexture); -texture.bind(); -``` - -:thumbsdown: -```cpp -public: - // default constructor - GroupBoid(); - explicit GroupBoid(int nbrBoid); - - /*METHODS*/ - - void addBoid(Boid&); - void addBoid(int nbr); - void removeBoid(int nbr); - void moveBoids(float cohesion, float separation, float forceCohesion, float forceSeparation, float mur, float forceMur, float alignement, float forceAlignement); - std::vector getGroup(); - int getSize(); - glm::vec wallForce(Boid boid, float posWall, int coord, float rayonMur, float forceMur); - glm::vec separationForce(glm::vec vector, float distance, float rayonSeparation, float forceSeparation); - glm::vec cohesionForce(glm::vec vector, float distance, float rayonCohesion, float forceCohesion); -``` -La plupart de ces méthodes peuvent être privées, elles ne sont utilisées qu'en interne par moveBoids(). - -:thumbsup: -```cpp -const int coordX = 0; -const int coordY = 1; -const int coordZ = 2; -``` -C'est très bien d'avoir fait ces alias ! - -:thumbsdown: -```cpp -void GroupBoid::addBoid(Boid& boid) -``` -Tu aurais pû mettre la référence const. - -:thumbsdown: -```cpp -int GroupBoid::getSize() -{ - int size = 0; - for (Boid& boid : group) - { - size++; - } - return size; -} -``` -Tu aurais pû utiliser la méthode size() de std::vector ! -```cpp -int GroupBoid::getSize() -{ - return static_cast(group.size()); // Conversion vers int car group.size() retourne un size_t. Ou alors (mieux) on aurait pû dire que la méthode getSize() retourne un size_t et non un int. -} -``` \ No newline at end of file diff --git a/content/evaluation/Corentin P.md b/content/evaluation/Corentin P.md index bfd1cc297..3a6623197 100644 --- a/content/evaluation/Corentin P.md +++ b/content/evaluation/Corentin P.md @@ -68,55 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - 🌞 Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -int state; // State of the boid (0 = normal, 1 = blue, 2 = green, 3 = red and predator) -``` -Vous pouvez utiliser un enum pour ça : -```cpp -enum class BoidState{ - Normal, - Blue, - Green, - RedPredator, -}; - -BoidState state; -``` - -```cpp -Model alg = Model(); -alg.loadModel("alg.obj"); -``` -Faites plutôt le load_model() directement dans le constructeur de Model ça sera plus simple. - -```cpp - GLuint bakeSkybox = 0; - glGenTextures(1, &bakeSkybox); - glBindTexture(GL_TEXTURE_2D, bakeSkybox); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_water.width(), img_water.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_water.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); - - GLuint bakeAlg = 0; - glGenTextures(1, &bakeAlg); - glBindTexture(GL_TEXTURE_2D, bakeAlg); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_alg.width(), img_alg.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_alg.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); -``` -Vous auriez pu faire une fonction qui load une texture plutôt que de dupliquer le code à chaque fois. - -```cpp - kw.~Model(); - skybox.~Model(); - turtle.~Model(); - alg.~Model(); - chest.~Model(); - floatie.~Model(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. \ No newline at end of file diff --git a/content/evaluation/Elies B.md b/content/evaluation/Elies B.md index 4e5ef5578..3d89cc8be 100644 --- a/content/evaluation/Elies B.md +++ b/content/evaluation/Elies B.md @@ -68,73 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ❌ Use `const` when appropriate () - ☁️ Use lambdas when appropriate () -➖ *What you **shouldn't** do:* - -- 🌞 Using `new` instead of a stack allocation or a smart pointer or a standard container (`std::vector`) -- ☁️ Forgetting `#pragma once` in your headers -- 🌞 Using `1` and `0` instead of `true` and `false` for your booleans - -## Investment (2 / 4 pts.) - -➕ *What you **should** do:* - -- ☁️ Ask questions and participate in class -- ☁️ Take my feedback into account, improve your old code if need be -- ☁️ Extend your project with additional features - working on that point. -::: - -## Tools (3 pts.) - -➕ *What you **should** do:* - -- ☁️ Follow [the good practices of the Git lesson](/lessons/git#good-practices) -- ☁️ Have a [.clang-format](/lessons/formatting-tool/) file, and apply it to your codebase -- ☁️ Have a [.clang-tidy](/lessons/static-analysers/) file, and fix the problems it shows you, or ask the teacher if you don't understand what the warning means or don't know how to fix it. - -➖ *What you **shouldn't** do:* - -- ☁️ Committing unwanted files like your *build* folder (see [.gitignore](/lessons/git#gitignore)) -- ☁️ Making no commits until near the end of the semester - -## Clean Code (12 pts.) - -➕ *What you **should** do:* - -- ☁️ Write many small functions () -- ☁️ Write small classes () -- ☁️ Write small structs () -- ☁️ Use strong types () -- ☁️ Use encapsulation when appropriate (`public` / `private`) () -- ☁️ Use free functions as often as possible, and methods only when appropriate () -- ☁️ - -➖ *What you **shouldn't** do:* - -- ☁️ Hard to understand or misleading names () -- ☁️ Duplicated code () -- ☁️ Overly complicated code -- ☁️ Global variables - -## C++ Code Quality (3 pts.) - -➕ *What you **should** do:* - -- ☁️ Use range-based loops or algorithms, instead of raw loops () -- ☁️ Use destructors when things need to be destroyed at the end. -- ☁️ Use `const` when appropriate () -- ☁️ Use lambdas when appropriate () - -➖ *What you **shouldn't** do:* - -- ☁️ Using `new` instead of a stack allocation or a smart pointer or a standard container (`std::vector`) -- ☁️ Forgetting `#pragma once` in your headers -- ☁️ Using `1` and `0` instead of `true` and `false` for your booleans - -## Investment (4 pts.) - -➕ *What you **should** do:* - -- ☁️ Ask questions and participate in class -- ☁️ Take my feedback into account, improve your old code if need be -- ☁️ Extend your project with additional features diff --git a/content/evaluation/Eliott D.md b/content/evaluation/Eliott D.md index 149c8b391..eae25ba03 100644 --- a/content/evaluation/Eliott D.md +++ b/content/evaluation/Eliott D.md @@ -68,84 +68,3 @@ import LessonLink from "@site/components/LessonLink" - 🌞 Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsdown: -```cpp - if (position.x <= params.bounds.x[0] + params.bounds_force_range) - { - float approach = position.x - params.bounds.x[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(map_to_max_speed(approach_normalised, params.max_speed), 0, 0); - } - - if (position.x >= params.bounds.x[1] - params.bounds_force_range) - { - float approach = params.bounds.x[1] - position.x; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(map_to_max_speed(approach_normalised, params.max_speed), 0, 0); - } - - if (position.y <= params.bounds.y[0] + params.bounds_force_range) - { - float approach = position.y - params.bounds.y[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(0, map_to_max_speed(approach_normalised, params.max_speed), 0); - } - - if (position.y >= params.bounds.y[1] - params.bounds_force_range) - { - float approach = params.bounds.y[1] - position.y; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(0, map_to_max_speed(approach_normalised, params.max_speed), 0); - } - - if (position.z <= params.bounds.z[0] + params.bounds_force_range) - { - float approach = position.z - params.bounds.z[0]; - float approach_normalised = approach / params.bounds_force_range; - velocity += glm::vec3(0, 0, map_to_max_speed(approach_normalised, params.max_speed)); - } - - if (position.z >= params.bounds.z[1] - params.bounds_force_range) - { - float approach = params.bounds.z[1] - position.z; - float approach_normalised = approach / params.bounds_force_range; - velocity -= glm::vec3(0, 0, map_to_max_speed(approach_normalised, params.max_speed)); - } -``` -Beaucoup de code dupliqué. - -:thumbsdown: -```cpp -GLuint texture_id; -``` -Vous auriez faire une classe wrapper pour Texture, comme vous l'avez fait pour VAO et VBO. Ça vous aurait évité d'oublier de détruire la texture dans le destructeur de Mesh. - -:thumbsup: -```cpp -{ - add_uniform_variable("uMVPMatrix"); - add_uniform_variable("uMVMatrix"); - add_uniform_variable("uNormalMatrix"); - - // add the texture uniform - add_uniform_variable("uText"); - - // add the light uniform - add_uniform_variable("uKd"); - add_uniform_variable("uKs"); - add_uniform_variable("uShininess"); - - add_light_uniforms(0); - add_light_uniforms(1); - add_light_uniforms(2); - - setup_mesh(); -} -``` -C'est très bien d'avoir fait des fonctions pour simplifier l'écriture de votre constructeur - -```cpp -void Mesh::change_mesh(const std::filesystem::path& obj_path, const std::filesystem::path& texture_path) -``` -Ce n'est pas idéal de reloader le mesh à chaque fois que vous voulez en changer, car ça prend du temps. Il aurait été plus performant de loader à l'avance tous les meshs que vous comptez utiliser, et de juste choisir le mesh à utiliser en fonction d'un booléen, ou d'un pointeur vers le mesh courant. \ No newline at end of file diff --git a/content/evaluation/Enora C.md b/content/evaluation/Enora C.md index 9255e55b1..431e1f0ee 100644 --- a/content/evaluation/Enora C.md +++ b/content/evaluation/Enora C.md @@ -68,90 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -void Boid::draw(GLuint vao, GLsizei vertexCount, glm::vec3 scale, float rotation, glm::mat4 viewMatrix, glm::mat4 ProjMatrix, glm::mat4& NormalMatrix, ObjectProgram& ObjectProgram, GLuint textureID, float coefLight, int typeLight) -{ - renderObject(vao, vertexCount, position, scale, rotation, viewMatrix, ProjMatrix, NormalMatrix, ObjectProgram, textureID, coefLight, typeLight); -} -``` -Vous auriez pu grouper tous ces paramètres dans une struct, pour éviter de les avoir dupliqués entre `Boid::draw()` et `renderObject()`. - -```cpp -if (falling == true) -``` -Préférez écrire plus simplement : -```cpp -if (falling) -``` - -```cpp -glm::vec3 averageAngle(0.0f, 0.0f, 0.0f); -``` -Le nom de cette variable est bizarre, car un vec3 ne peut pas être un angle. - -```cpp - Model Carl; - Carl.load_model("assets/models/carl_doug.obj"); -``` -Plutôt que de faire une méthode load_model() vous auriez dû faire ça dans le constructeur. - -```cpp - glDeleteBuffers(1, &vboBench); - glDeleteBuffers(1, &vboBoid); - glDeleteBuffers(1, &vboFloor); - glDeleteBuffers(1, &vboCube); - glDeleteBuffers(1, &vboBoidCube); - glDeleteBuffers(1, &vboCarl); - glDeleteBuffers(1, &vboHouse3D); - glDeleteBuffers(1, &vboRocks); - glDeleteBuffers(1, &vboTree1); - glDeleteBuffers(1, &vboTree2); -``` -Vous auriez pu faire une classe VBO avec un destructeur qui appelle glDeleteBuffers automatiquement. - -```cpp -switch (loi) -{ -case 1: // Loi binomiale de Bernoulli - return p * (1 - p); -case 2: // Loi exponentielle - return 1 / (p * p); -case 3: // Loi uniforme - return ((q - p) * (q - p)) / 12; -case 4: // Loi normale - return q * q; -case 5: // Loi de Laplace - return 2 * q * q; -default: - return -1; // Type de loi invalide -} -``` -Vous auriez pu utiliser un enum : -```cpp -enum class Law{ - Binomiale, - Exponentielle, - Uniforme, - Normale, - Laplace, -}; -float esperance(Law loi, float p, float q) -{ - switch (loi) - { - case Law::Binomiale: - return p * (1 - p); - case Law::Exponentielle: - return 1 / (p * p); - case Law::Uniforme: - return ((q - p) * (q - p)) / 12; - case Law::Normale: - return q * q; - case Law::Laplace: - return 2 * q * q; - default: - return -1; // Type de loi invalide - } -} -``` \ No newline at end of file diff --git a/content/evaluation/Enzo B.md b/content/evaluation/Enzo B.md index cd3a6b942..ea67f0aa3 100644 --- a/content/evaluation/Enzo B.md +++ b/content/evaluation/Enzo B.md @@ -68,54 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - -```cpp -struct Scene { -float cubeBaseSize = 10.f; // for reference only, do not touch -float groundBaseSize = 2.f; // for reference only, do not touch -``` -si ces valeurs ne doivent pas être changées, plutôt que de les mettre dans la struct vous auriez pu les mettre en constantes : -```cpp -static constexpr float cubeBaseSize = 10.f; -static constexpr float groundBaseSize = 2.f; -struct Scene { -``` -Même remarque pour -```cpp -class Flock { -// ... - std::vector _skinList = {"_red", "_orange", "_blue", "_green", "_blue", "_grey", "_monochrome"}; -``` -Cette _skinList est une constante partagée par toutes les flocks, pas besoin d'en stocker une copie dans chaque Flock. - -```cpp -Object3D::Object3D(const std::string& name, const std::string& vertexShaderPath, const std::string& fragmentShaderPath) - : _model(name), _texture(name), _shader(vertexShaderPath, fragmentShaderPath) -{ - defineVBO(); - defineVAO(); -}; - -Object3D::Object3D(const std::string& name, const std::string& vertexShaderPath, const std::string& fragmentShaderPath, const std::string& skinID, const std::string& lodID) - : _model(name + lodID), _texture(name + skinID), _shader(vertexShaderPath, fragmentShaderPath) -{ - defineVBO(); - defineVAO(); -} -``` -Plutôt que de dupliquer le constructeur, vous pouvez faire en sorte que l'un appelle l'autre, en rajoutant un troisième constructeur qui prend en paramètre model_name et texture_name. Et les deux autres peuvent déléguer à celui-là. - -```cpp -void Object3D::clear() -{ - glDeleteBuffers(1, &_vbo); // Delete the vertex buffer - glDeleteVertexArrays(1, &_vao); // Delete the vertex array -} -``` -Faites plutôt un destructeur, qui sera appelé automatiquement au bon moment. - -```cpp -_renderer.clearAll(); -``` -C'est confusant que vous utilisiez parfois le terme "clear" pour désigner une méthode qui détruit les objets, et d'autres fois pour désigner une méthode qui reset la couleur de l'écran et bind des choses. \ No newline at end of file diff --git a/content/evaluation/Erwan T.md b/content/evaluation/Erwan T.md index c5642369e..53b4cc794 100644 --- a/content/evaluation/Erwan T.md +++ b/content/evaluation/Erwan T.md @@ -68,47 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - - -```cpp - float posX; - float posY; - float posZ; -``` -Vous auriez pû utiliser un glm::vec3, ça vous aurait simplifié la vie ! Par exemple au lieu de faire -```cpp - posX += (velocityX * ctx.delta_time()) * speedFactor; - posY += (velocityY * ctx.delta_time()) * speedFactor; - posZ += (velocityZ * ctx.delta_time()) * speedFactor; -``` -vous auriez pû faire -```cpp - pos += (velocity * ctx.delta_time()) * speedFactor; -``` - -```cpp -bool oscillationDirection = true; // true pour une augmentation, false pour une diminution -``` -Si on a besoin d'un commentaire pour comprendre cette variable, c'est qu'elle mériterait d'être mieux nommée. Par exemple : `oscillationsDoIncrease`. Ou alors carrément faire un enum : -```cpp -enum class OscillationDirection -{ - Increase, - Decrease, -} - -OscillationDirection oscillationDirection = OscillationDirection::Increase; -``` - -```cpp - void setLightIntensity(unsigned int index, float intensity) - { - if (index < lights.size()) - { - lights[index].intensity = intensity; - } - } -``` -Ça peut être bien de remonter une erreur quand l'index est invalide (via une assert, une exception, etc., au choix), plutôt que de silencieusement ne rien faire. Ça vous évitera de passer une heure à debuguer un `setLightIntensity()` qui ne fait rien, tout ça pour vous rendre compte à la fin que c'était juste l'index qui n'était pas bon. - diff --git a/content/evaluation/Florian T.md b/content/evaluation/Florian T.md index 9948ddf9b..557e10ea1 100644 --- a/content/evaluation/Florian T.md +++ b/content/evaluation/Florian T.md @@ -68,24 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp - int find_biome( - std::vector - &tableau_de_biomes) -``` -Passer par const& si vous ne modifiez pas le tableau. - -```cpp - void assign_DND_alignement(std::vector &tableau_de_biomes) { -``` -C'est quoi DND ? Évitez les abbreviations, ça rend votre code difficile à comprendre. - -```cpp -void Objet3D::clear() -{ - glDeleteBuffers(1, &vbo); - glDeleteVertexArrays(1, &vao); -} -``` -Plutôt qu'une fonction clear, faites un destructeur. Il sera appelé automatiquement c'est vachement pratique. \ No newline at end of file diff --git a/content/evaluation/Guilhem D.md b/content/evaluation/Guilhem D.md index 0901fd1b5..1f7cdcf86 100644 --- a/content/evaluation/Guilhem D.md +++ b/content/evaluation/Guilhem D.md @@ -68,36 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsup: -```cpp -if (coeffs.isLowPoly) -{ - star_boid_low.change_color(b.get_color()); - star_boid_low.set_position(b.get_position()); - boids_program.render_game_object(star_boid_low, view_matrix, proj_matrix); -} -else -{ - star_boid.change_color(b.get_color()); - star_boid.set_position(b.get_position()); - boids_program.render_game_object(star_boid, view_matrix, proj_matrix); -} -``` -C'est très bien d'avoir loadé à l'avance tous les meshs possibles pour le LOD (ça économise des performances, plutôt que de changer le mesh à chaque fois que le LOD change). -:thumbsdown: -Pour éviter la duplication de code, vous auriez pu faire comme ceci : -```cpp -// D'abord on choisit le bon objet -const auto& star = coeffs.isLowPoly ? star_boid_low : star_boid; -// Puis on fait le code potentiellement long et compliqué -star.change_color(b.get_color()); -star.set_position(b.get_position()); -boids_program.render_game_object(star, view_matrix, proj_matrix); -``` - -:thumbsdown: -```cpp -set_factors({1.0f, 1.0f, 1.0f}, {0.5f, 0.5f, 0.5f}, 64.0f); -``` -Le nom de cette fonction n'est pas clair du tout ! `factor` c'est un terme ultra générique ! `set_material` ou `set_lighting_factors` aurait été mieux. \ No newline at end of file diff --git a/content/evaluation/Heidi L.md b/content/evaluation/Heidi L.md index bfd1cc297..3a6623197 100644 --- a/content/evaluation/Heidi L.md +++ b/content/evaluation/Heidi L.md @@ -68,55 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - 🌞 Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -int state; // State of the boid (0 = normal, 1 = blue, 2 = green, 3 = red and predator) -``` -Vous pouvez utiliser un enum pour ça : -```cpp -enum class BoidState{ - Normal, - Blue, - Green, - RedPredator, -}; - -BoidState state; -``` - -```cpp -Model alg = Model(); -alg.loadModel("alg.obj"); -``` -Faites plutôt le load_model() directement dans le constructeur de Model ça sera plus simple. - -```cpp - GLuint bakeSkybox = 0; - glGenTextures(1, &bakeSkybox); - glBindTexture(GL_TEXTURE_2D, bakeSkybox); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_water.width(), img_water.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_water.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); - - GLuint bakeAlg = 0; - glGenTextures(1, &bakeAlg); - glBindTexture(GL_TEXTURE_2D, bakeAlg); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_alg.width(), img_alg.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_alg.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); -``` -Vous auriez pu faire une fonction qui load une texture plutôt que de dupliquer le code à chaque fois. - -```cpp - kw.~Model(); - skybox.~Model(); - turtle.~Model(); - alg.~Model(); - chest.~Model(); - floatie.~Model(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. \ No newline at end of file diff --git a/content/evaluation/Laura G.md b/content/evaluation/Laura G.md index c839f88ad..e68ef7e49 100644 --- a/content/evaluation/Laura G.md +++ b/content/evaluation/Laura G.md @@ -68,19 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - -```cpp -int _type = 0 ; //0 for fish 1 for star; -``` -Vous pouvez faire un enum pour ça : -```cpp -enum class BoidType{ - Fish, - Star, -}; - -BoidType _type = BoidType::Fish; -``` - -Énormément de code dupliqué dans votre `main()` pour la création des objets OpenGL. Vous auriez pu faire une classe qui aurait fait tout ça automatiquement. \ No newline at end of file diff --git a/content/evaluation/Lea T.md b/content/evaluation/Lea T.md index fc3e7d2f5..45660ef36 100644 --- a/content/evaluation/Lea T.md +++ b/content/evaluation/Lea T.md @@ -68,64 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -inline void gen() { glGenVertexArrays(1, &this->id); }; -``` -Plutôt qu'une méthode gen(), vous auriez pu mettre ce code directement dans le constructeur. Ça vous aurait évité de devoir ensuite appeler gen() sur absolument tous les objets que vous créez. - -```cpp -class IBO { -std::vector indices; -inline void fill() { glBufferData(GL_ELEMENT_ARRAY_BUFFER, this->indices.size() * sizeof(uint32_t), this->indices.data(), GL_STATIC_DRAW); }; -// ... -}; -``` -Vous n'avez pas besoin de stocker les indices dans la classe, il ne servent qu'au moment de la construction. Il aurait mieux valu passer les indices en paramètre au constructeur, qui se serait ensuite chargé de faire le code de fill(). - -```cpp -for (size_t i = 0; i < allBoids.size(); i++) -``` -Vous auriez pu faire une boucle foreach : -```cpp -for (auto& boid : allBoids) -``` - -```cpp -if (this->flock[i].getRadius() == this->radiusLittleFish) -{ - texLittleFish.activate(); - texLittleFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texLittleFish.unbind(); -} -else if (this->flock[i].getRadius() == this->radiusMediumFish) -{ - texMediumFish.activate(); - texMediumFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texMediumFish.unbind(); -} -else -{ - texBigFish.activate(); - texBigFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texBigFish.unbind(); -} -``` -Pour éviter de dupliquer le code vous auriez pu faire comme ça : -```cpp -// D'abord on récupère une référence vers la bonne texture -auto& texture = this->flock[i].getRadius() == this->radiusLittleFish - ? texLittleFish - : this->flock[i].getRadius() == this->radiusMediumFish - ? texMediumFish - : texBigFish; - -// Et ensuite on fait le code qui utilise la texture -texture.activate(); -texture.bind(); -this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); -texture.unbind(); -``` \ No newline at end of file diff --git a/content/evaluation/Lena C.md b/content/evaluation/Lena C.md index 9255e55b1..431e1f0ee 100644 --- a/content/evaluation/Lena C.md +++ b/content/evaluation/Lena C.md @@ -68,90 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -void Boid::draw(GLuint vao, GLsizei vertexCount, glm::vec3 scale, float rotation, glm::mat4 viewMatrix, glm::mat4 ProjMatrix, glm::mat4& NormalMatrix, ObjectProgram& ObjectProgram, GLuint textureID, float coefLight, int typeLight) -{ - renderObject(vao, vertexCount, position, scale, rotation, viewMatrix, ProjMatrix, NormalMatrix, ObjectProgram, textureID, coefLight, typeLight); -} -``` -Vous auriez pu grouper tous ces paramètres dans une struct, pour éviter de les avoir dupliqués entre `Boid::draw()` et `renderObject()`. - -```cpp -if (falling == true) -``` -Préférez écrire plus simplement : -```cpp -if (falling) -``` - -```cpp -glm::vec3 averageAngle(0.0f, 0.0f, 0.0f); -``` -Le nom de cette variable est bizarre, car un vec3 ne peut pas être un angle. - -```cpp - Model Carl; - Carl.load_model("assets/models/carl_doug.obj"); -``` -Plutôt que de faire une méthode load_model() vous auriez dû faire ça dans le constructeur. - -```cpp - glDeleteBuffers(1, &vboBench); - glDeleteBuffers(1, &vboBoid); - glDeleteBuffers(1, &vboFloor); - glDeleteBuffers(1, &vboCube); - glDeleteBuffers(1, &vboBoidCube); - glDeleteBuffers(1, &vboCarl); - glDeleteBuffers(1, &vboHouse3D); - glDeleteBuffers(1, &vboRocks); - glDeleteBuffers(1, &vboTree1); - glDeleteBuffers(1, &vboTree2); -``` -Vous auriez pu faire une classe VBO avec un destructeur qui appelle glDeleteBuffers automatiquement. - -```cpp -switch (loi) -{ -case 1: // Loi binomiale de Bernoulli - return p * (1 - p); -case 2: // Loi exponentielle - return 1 / (p * p); -case 3: // Loi uniforme - return ((q - p) * (q - p)) / 12; -case 4: // Loi normale - return q * q; -case 5: // Loi de Laplace - return 2 * q * q; -default: - return -1; // Type de loi invalide -} -``` -Vous auriez pu utiliser un enum : -```cpp -enum class Law{ - Binomiale, - Exponentielle, - Uniforme, - Normale, - Laplace, -}; -float esperance(Law loi, float p, float q) -{ - switch (loi) - { - case Law::Binomiale: - return p * (1 - p); - case Law::Exponentielle: - return 1 / (p * p); - case Law::Uniforme: - return ((q - p) * (q - p)) / 12; - case Law::Normale: - return q * q; - case Law::Laplace: - return 2 * q * q; - default: - return -1; // Type de loi invalide - } -} -``` \ No newline at end of file diff --git a/content/evaluation/Leo S.md b/content/evaluation/Leo S.md index 70f243d2f..059ae6947 100644 --- a/content/evaluation/Leo S.md +++ b/content/evaluation/Leo S.md @@ -68,37 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -public: - Boid(int sizeBoid); - - void display(glm::mat4 &ModelMatrix, glm::mat4 &ViewMatrix, glm::mat4 &ProjMatrix, - GLint uMVPMatrix, GLint uMVMatrix, GLint uNormalMatrix, Model3D &model) const; - void updatePosition(); - vec getPosition() const; - glm::vec3 getColor() const; - vec getDirection() const; - vec calculateSeparationForce(const std::vector& boids, float separation); - vec calculateCohesionForce(const std::vector& boids, float cohesion); - vec calculateAlignmentForce(const std::vector& boids, float alignment); - void applySteeringForces(const std::vector& boids, float separation, float cohesion, float alignment); -``` -La plupart de ces fonctions auraient ppu être privées - -```cpp -void ListeBoids::display(glm::mat4 &ModelMatrix, glm::mat4 &ViewMatrix, glm::mat4 &ProjMatrix, - const GLint uMVPMatrix, const GLint uMVMatrix, const GLint uNormalMatrix, Model3D &model) const -{ - for (const Boid& b : listeBoids) - { - b.display(ModelMatrix, ViewMatrix, ProjMatrix, uMVPMatrix, uMVMatrix, uNormalMatrix, model); - } -} -``` -Vous auriez pu faire une struct pour grouper tous ces paramètres pour éviter de les dupliquer entre ListeBoids::display() et Boid::display(). - -```cpp -void Model3D::drawObject(glm::mat4 &ViewMatrix, glm::mat4 &ModelMatrix, glm::mat4 &ProjMatrix, -``` -Passez par const& quand vous n'avez pas besoin de modifier l'objet \ No newline at end of file diff --git a/content/evaluation/Lila C.md b/content/evaluation/Lila C.md index 0f977d541..024ec8a1f 100644 --- a/content/evaluation/Lila C.md +++ b/content/evaluation/Lila C.md @@ -68,61 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -void Boids::draw(const p6::Context& ctx, const float square_radius, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time) -{ - for (auto& i : m_boids) - { - i.draw(ctx, mesh, viewmatrix, program, time); - i.move(square_radius, maxspeed, minspeed, height); - } -} -``` -Le fait que la méthode `draw()` fasse à la fois draw et move peut être confusant. Il faudrait lui donner un nom plus explicite, ou encore mieux, séparer ça en deux fonctions draw() et move(). - -```cpp -public: - Boids() = default; - Boids(const std::vector vec); - Boids(const int number); - std::vector getVect() const; // get le vector de Boids - int numberOfBoids() const; - Boid getBoid(const int id); - void addBoid(const Boid& boid); - void deleteBoid(); - void changeSize(const int boids_number); - void draw(const p6::Context& ctx, const float square_radius, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time); // dessine tous les Boids - void alignement(const float neighbor_dist); - void cohesion(const float neighbor_dist); - void separation(const float avoid_factor); - void update(const p6::Context& ctx, const int boids_number, const float square_radius, const float neighbor_dist, const float avoid_factor, const float maxspeed, const float minspeed, const Mesh& mesh, const glm::mat4 viewmatrix, const Program& program, float& height, const int& time); - std::vector otherBoids(const Boid& b); -}; -``` -La plupart de ces méthodes pourraient être privées. - -```cpp -Scene scene; -scene.Init(ctx); -``` -Plutôt qu'une méthode Init faites ça directement dans le constructeur. - -```cpp -void Scene::cleanupRessources() -{ - m_fish_program.deleteTextureBufferArray(); - m_cube_program.deleteTextureBufferArray(); - m_arpenteur_program.deleteTextureBufferArray(); - m_seaweed_program.deleteTextureBufferArray(); - m_shark_program.deleteTextureBufferArray(); - m_cube.DeleteVboVao(); - m_fish.DeleteVboVao(); - m_fishlow.DeleteVboVao(); - m_fishverylow.DeleteVboVao(); - m_shark.DeleteVboVao(); - m_seaweed.DeleteVboVao(); - m_mushroom.DeleteVboVao(); -} -``` -Pas besoin de ces méthodes puisque vous avez mis un destructeur aux classes VBO et VAO. Le destructeur sera appelé automatiquement, c'est tout l'intérêt ! \ No newline at end of file diff --git a/content/evaluation/Lilou B.md b/content/evaluation/Lilou B.md index 65ca5b509..43e46047e 100644 --- a/content/evaluation/Lilou B.md +++ b/content/evaluation/Lilou B.md @@ -68,50 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -std::vector boids(75); -for (auto& boid : boids) -{ - boid.set_speed(); - boid.set_position(); -} -``` -set_speed() et set_position() auraient pu être fait dans le constructeur de Boids. -Et pareil pour ce load_model() : -```cpp -Model ladybug = Model(); -ladybug.loadModel("dragonfly.obj"); -``` - -```cpp - GLuint textureLadybug; - glGenTextures(1, &textureLadybug); - glBindTexture(GL_TEXTURE_2D, textureLadybug); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_ladybug.width(), img_ladybug.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_ladybug.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); -``` -Vous auriez pu faire une fonction pour loader une texture. Ca vous aurait évité de devoir dupliquer le code pour chaque texture que vous loadez. - -```cpp - ladybug.~Model(); - personModel.~Model(); - barque.~Model(); - ocean.~Model(); - ile.~Model(); - trees.~Model(); - pirate.~Model(); - cone.~Model(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. - -```cpp -class LoadShader { -private: - std::filesystem::path m_vertexShader; - std::filesystem::path m_fragmentShader; -``` -Pas besoin de stocker les chemins dans la classe, ils ne servent que pendant le constructeur pour loader le shader. \ No newline at end of file diff --git a/content/evaluation/Logan A.md b/content/evaluation/Logan A.md index ae51517c5..b7e3a0e52 100644 --- a/content/evaluation/Logan A.md +++ b/content/evaluation/Logan A.md @@ -68,12 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - 🌞 Extend your project with additional features - - -```cpp -class Texture { -private: - img::Image image; - GLuint imagePointer; -``` -Vous auriez pu faire un destructeur pour automatiquement appeler glDeleteTextures \ No newline at end of file diff --git a/content/evaluation/Lou B.md b/content/evaluation/Lou B.md index 8e2b01c29..ae3b5d642 100644 --- a/content/evaluation/Lou B.md +++ b/content/evaluation/Lou B.md @@ -68,30 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -Parameters getData() -{ - return Parameters(); -} -``` -Pourquoi faire une fonction quand on pourrait juste appeler le constructeur directement ? - -```cpp -auto Zone = loadModel("../Models/zone.obj"); -BufferManager bufferManager2; -bufferManager2.createBuffers(Zone); -``` -Plutôt que de faire 3 lignes pour chaque objet, il aurait mieux valu que le constructeur de buffer manager prenne le chemin du obj en paramètre, et fasse tout par lui même. Comme ça ça serait plus simple de créer plein d'objets : -```cpp -BufferManager bufferManagerZone{"../Models/zone.obj"}; -``` -Et au passage, donner des noms plus explicites aux différents buffers managers permet de mieux s'y retrouver dans la suite du code. - -```cpp -// If pour le LOD - if (parameters.LOD == 0) - { - // ... -``` -Plutôt que de dupliquer un énorme pavé dans le if, je suis sûr que la plupart du code aurait pu être refactor. \ No newline at end of file diff --git a/content/evaluation/Lucie G.md b/content/evaluation/Lucie G.md index 3ff66c4d8..e163838de 100644 --- a/content/evaluation/Lucie G.md +++ b/content/evaluation/Lucie G.md @@ -68,78 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsup: -```cpp -Object3D turtle(VaoType::Classic); -turtle - .loadObj("assets/models/Turtle.obj") - .loadTexture("assets/textures/turtle_texture.png") - .setProgram(renderer, "shaders/singleObject.vs.glsl", "shaders/texture.fs.glsl") - .setTransform( - glm::vec3(0.0, -4.5, 0.0), - glm::vec3(0.0), - glm::vec3(1.0)); -``` -Très bonne API. - -:thumbsdown: -```cpp -constexpr size_t N = 500; -``` -C'est quoi N ? Un nom plus explicite aurait été mieux. - -:thumbsdown: -```cpp - auto isProgramLoaded( - std::filesystem::path const& vert, - std::filesystem::path const& frag - ) const -> std::optional; -``` -Le nom donne l'impression que la fonction renvoie un booléen. Il aurait mieux valu l'appeler qqch comme `maybeProgramIndex`. - -:thumbsdown: -```cpp -auto uniform0to1() -> float { - thread_local std::default_random_engine gen{std::random_device{}()}; - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(gen); -} - -auto duniform0to1() -> double { - thread_local std::default_random_engine gen{std::random_device{}()}; - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(gen); -} -``` -Ces deux fonctions pourraient partager le même `std::default_random_engine`, ça éviterait d'en créer trop : -```cpp -static auto random_engine() -> std::default_random_engine& -{ - thread_local std::default_random_engine gen{std::random_device{}()}; - return gen; -} - -auto uniform0to1() -> float { - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(random_engine()); -} - -auto duniform0to1() -> double { - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(random_engine()); -} -``` - -:thumbsdown: -```cpp -void Object3D::render(Renderer const &renderer) { - if (!this->glProgramIndex) { - return; - } -``` -C'est bien de générer une erreur (assert ou exception, ici une assert est plus adapté) pour vous aider à débuguer le jour où votre mesh ne s'affichera pas parce que vous avez oublié de set le programme. \ No newline at end of file diff --git a/content/evaluation/Mailis B.md b/content/evaluation/Mailis B.md index 8e2b01c29..ae3b5d642 100644 --- a/content/evaluation/Mailis B.md +++ b/content/evaluation/Mailis B.md @@ -68,30 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -Parameters getData() -{ - return Parameters(); -} -``` -Pourquoi faire une fonction quand on pourrait juste appeler le constructeur directement ? - -```cpp -auto Zone = loadModel("../Models/zone.obj"); -BufferManager bufferManager2; -bufferManager2.createBuffers(Zone); -``` -Plutôt que de faire 3 lignes pour chaque objet, il aurait mieux valu que le constructeur de buffer manager prenne le chemin du obj en paramètre, et fasse tout par lui même. Comme ça ça serait plus simple de créer plein d'objets : -```cpp -BufferManager bufferManagerZone{"../Models/zone.obj"}; -``` -Et au passage, donner des noms plus explicites aux différents buffers managers permet de mieux s'y retrouver dans la suite du code. - -```cpp -// If pour le LOD - if (parameters.LOD == 0) - { - // ... -``` -Plutôt que de dupliquer un énorme pavé dans le if, je suis sûr que la plupart du code aurait pu être refactor. \ No newline at end of file diff --git a/content/evaluation/Marianne K.md b/content/evaluation/Marianne K.md index e3544cc72..931034302 100644 --- a/content/evaluation/Marianne K.md +++ b/content/evaluation/Marianne K.md @@ -68,117 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp - Light lightCharacter(1, shader); - Light lightFixed(2, shader); - Light lightDir(3, shader); -``` -Plutôt que de spécifier manuellement l'index pour chaque light, vous pouvez utiliser une variable static dans le constructeur de Light, qui incrémente à chaque fois qu'une Light est créée : -```cpp -static int next_index() -{ - static int index{0}; - index++; - return index; -} - -Light(const p6::Shader& shader) - : index(next_index()), color(randUniform(1.0f, 1.0f), randUniform(1.0f, 1.0f), randUniform(1.0f, 1.0f)), position(0.0f, 0.0f, 0.0f), uKdLocation(0), uKsLocation(0), uShininessLocation(0), uLightPosLocation(0), uLightIntensityLocation(0) -{ - setup(shader); -} -``` - -```cpp -if (index == 1) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity"); -} -else if (index == 2) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd2"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs2"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess2"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs2"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity2"); -} -else if (index == 3) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd3"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs3"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess3"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs3"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity3"); -} -``` -Pour éviter la duplication de code : -```cpp -uKdLocation = glGetUniformLocation(shader.id(), "uKd" + std::to_string(index)); -uKsLocation = glGetUniformLocation(shader.id(), "uKs" + std::to_string(index)); -uShininessLocation = glGetUniformLocation(shader.id(), "uShininess" + std::to_string(index)); -uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs" + std::to_string(index)); -uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity" + std::to_string(index)); -``` - -```cpp -if (context.getIsLowPoly()) -{ - character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_lod); - boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_lod); -} -else -{ - character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid); - boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid); -} -``` -Plutôt que de dupliquer le code, vous pouvez faire comme ça : -```cpp -// On choisit le bon mesh en fonction du LOD -const auto& boid_mesh = context.getIsLowPoly() ? boid_lod : boid; - -// On en fait le code qu'une seule fois, en utilisant la référence de mesh précédemment déterminée -character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_mesh); -boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_mesh); -``` - -```cpp -boid.~Mesh(); -decor.~Mesh(); -character.~Character(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. - -```cpp -void Boid::drawMesh(GLuint uMVPMatrixLocation, GLuint uMVMatrixLocation, GLuint uNormalMatrixLocation, glm::mat4 ProjMatrix, glm::mat4 viewMatrix, Mesh mesh) const -{ - glm::mat4 MVMatrix = viewMatrix * glm::translate(glm::mat4{1.f}, position) * glm::scale(glm::mat4{1.f}, glm::vec3(0.015f)); - - glm::mat4 NormalMatrix = glm::transpose(glm::inverse(MVMatrix)); - - glUniformMatrix4fv(uMVPMatrixLocation, 1, GL_FALSE, glm::value_ptr(ProjMatrix * MVMatrix)); - glUniformMatrix4fv(uMVMatrixLocation, 1, GL_FALSE, glm::value_ptr(MVMatrix)); - glUniformMatrix4fv(uNormalMatrixLocation, 1, GL_FALSE, glm::value_ptr(NormalMatrix)); - - mesh.draw(position, scale, ProjMatrix, viewMatrix, uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, rotation); -} -``` -Vous auriez pu faire une struct pour grouper tous les paramètres dupliqués entre Boid::drawMesh() et mesh.draw() (et aussi Character::draw() etc.). - -```cpp -void Mesh::draw(glm::vec3 pos, glm::vec3 scale, glm::mat4 ProjMatrix, glm::mat4 viewMatrix, GLuint uMVPMatrixLocation, GLuint uMVMatrixLocation, GLuint uNormalMatrixLocation, float angle) -{ - // ... - setBuffers(); - // ... -} -``` -Attention vous recréez vos buffers à chaque frame !! C'est une grosse perte de performance ! (Et aussi une memory leak puisque vous ne deletez pas l'ancien buffer). - -La plupart des méthodes de la classe Mesh auraient pu être privées, elles ne sont utilisées que par Mesh en interne. - diff --git a/content/evaluation/Marion B.md b/content/evaluation/Marion B.md index 0901fd1b5..1f7cdcf86 100644 --- a/content/evaluation/Marion B.md +++ b/content/evaluation/Marion B.md @@ -68,36 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsup: -```cpp -if (coeffs.isLowPoly) -{ - star_boid_low.change_color(b.get_color()); - star_boid_low.set_position(b.get_position()); - boids_program.render_game_object(star_boid_low, view_matrix, proj_matrix); -} -else -{ - star_boid.change_color(b.get_color()); - star_boid.set_position(b.get_position()); - boids_program.render_game_object(star_boid, view_matrix, proj_matrix); -} -``` -C'est très bien d'avoir loadé à l'avance tous les meshs possibles pour le LOD (ça économise des performances, plutôt que de changer le mesh à chaque fois que le LOD change). -:thumbsdown: -Pour éviter la duplication de code, vous auriez pu faire comme ceci : -```cpp -// D'abord on choisit le bon objet -const auto& star = coeffs.isLowPoly ? star_boid_low : star_boid; -// Puis on fait le code potentiellement long et compliqué -star.change_color(b.get_color()); -star.set_position(b.get_position()); -boids_program.render_game_object(star, view_matrix, proj_matrix); -``` - -:thumbsdown: -```cpp -set_factors({1.0f, 1.0f, 1.0f}, {0.5f, 0.5f, 0.5f}, 64.0f); -``` -Le nom de cette fonction n'est pas clair du tout ! `factor` c'est un terme ultra générique ! `set_material` ou `set_lighting_factors` aurait été mieux. \ No newline at end of file diff --git a/content/evaluation/Matheo P.md b/content/evaluation/Matheo P.md index e3544cc72..931034302 100644 --- a/content/evaluation/Matheo P.md +++ b/content/evaluation/Matheo P.md @@ -68,117 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp - Light lightCharacter(1, shader); - Light lightFixed(2, shader); - Light lightDir(3, shader); -``` -Plutôt que de spécifier manuellement l'index pour chaque light, vous pouvez utiliser une variable static dans le constructeur de Light, qui incrémente à chaque fois qu'une Light est créée : -```cpp -static int next_index() -{ - static int index{0}; - index++; - return index; -} - -Light(const p6::Shader& shader) - : index(next_index()), color(randUniform(1.0f, 1.0f), randUniform(1.0f, 1.0f), randUniform(1.0f, 1.0f)), position(0.0f, 0.0f, 0.0f), uKdLocation(0), uKsLocation(0), uShininessLocation(0), uLightPosLocation(0), uLightIntensityLocation(0) -{ - setup(shader); -} -``` - -```cpp -if (index == 1) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity"); -} -else if (index == 2) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd2"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs2"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess2"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs2"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity2"); -} -else if (index == 3) -{ - uKdLocation = glGetUniformLocation(shader.id(), "uKd3"); - uKsLocation = glGetUniformLocation(shader.id(), "uKs3"); - uShininessLocation = glGetUniformLocation(shader.id(), "uShininess3"); - uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs3"); - uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity3"); -} -``` -Pour éviter la duplication de code : -```cpp -uKdLocation = glGetUniformLocation(shader.id(), "uKd" + std::to_string(index)); -uKsLocation = glGetUniformLocation(shader.id(), "uKs" + std::to_string(index)); -uShininessLocation = glGetUniformLocation(shader.id(), "uShininess" + std::to_string(index)); -uLightPosLocation = glGetUniformLocation(shader.id(), "uLightPos_vs" + std::to_string(index)); -uLightIntensityLocation = glGetUniformLocation(shader.id(), "uLightIntensity" + std::to_string(index)); -``` - -```cpp -if (context.getIsLowPoly()) -{ - character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_lod); - boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_lod); -} -else -{ - character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid); - boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid); -} -``` -Plutôt que de dupliquer le code, vous pouvez faire comme ça : -```cpp -// On choisit le bon mesh en fonction du LOD -const auto& boid_mesh = context.getIsLowPoly() ? boid_lod : boid; - -// On en fait le code qu'une seule fois, en utilisant la référence de mesh précédemment déterminée -character.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_mesh); -boids.draw(uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, ProjMatrix, viewMatrix, boid_mesh); -``` - -```cpp -boid.~Mesh(); -decor.~Mesh(); -character.~Character(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. - -```cpp -void Boid::drawMesh(GLuint uMVPMatrixLocation, GLuint uMVMatrixLocation, GLuint uNormalMatrixLocation, glm::mat4 ProjMatrix, glm::mat4 viewMatrix, Mesh mesh) const -{ - glm::mat4 MVMatrix = viewMatrix * glm::translate(glm::mat4{1.f}, position) * glm::scale(glm::mat4{1.f}, glm::vec3(0.015f)); - - glm::mat4 NormalMatrix = glm::transpose(glm::inverse(MVMatrix)); - - glUniformMatrix4fv(uMVPMatrixLocation, 1, GL_FALSE, glm::value_ptr(ProjMatrix * MVMatrix)); - glUniformMatrix4fv(uMVMatrixLocation, 1, GL_FALSE, glm::value_ptr(MVMatrix)); - glUniformMatrix4fv(uNormalMatrixLocation, 1, GL_FALSE, glm::value_ptr(NormalMatrix)); - - mesh.draw(position, scale, ProjMatrix, viewMatrix, uMVPMatrixLocation, uMVMatrixLocation, uNormalMatrixLocation, rotation); -} -``` -Vous auriez pu faire une struct pour grouper tous les paramètres dupliqués entre Boid::drawMesh() et mesh.draw() (et aussi Character::draw() etc.). - -```cpp -void Mesh::draw(glm::vec3 pos, glm::vec3 scale, glm::mat4 ProjMatrix, glm::mat4 viewMatrix, GLuint uMVPMatrixLocation, GLuint uMVMatrixLocation, GLuint uNormalMatrixLocation, float angle) -{ - // ... - setBuffers(); - // ... -} -``` -Attention vous recréez vos buffers à chaque frame !! C'est une grosse perte de performance ! (Et aussi une memory leak puisque vous ne deletez pas l'ancien buffer). - -La plupart des méthodes de la classe Mesh auraient pu être privées, elles ne sont utilisées que par Mesh en interne. - diff --git a/content/evaluation/Matteo M.md b/content/evaluation/Matteo M.md index 3ff66c4d8..e163838de 100644 --- a/content/evaluation/Matteo M.md +++ b/content/evaluation/Matteo M.md @@ -68,78 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -:thumbsup: -```cpp -Object3D turtle(VaoType::Classic); -turtle - .loadObj("assets/models/Turtle.obj") - .loadTexture("assets/textures/turtle_texture.png") - .setProgram(renderer, "shaders/singleObject.vs.glsl", "shaders/texture.fs.glsl") - .setTransform( - glm::vec3(0.0, -4.5, 0.0), - glm::vec3(0.0), - glm::vec3(1.0)); -``` -Très bonne API. - -:thumbsdown: -```cpp -constexpr size_t N = 500; -``` -C'est quoi N ? Un nom plus explicite aurait été mieux. - -:thumbsdown: -```cpp - auto isProgramLoaded( - std::filesystem::path const& vert, - std::filesystem::path const& frag - ) const -> std::optional; -``` -Le nom donne l'impression que la fonction renvoie un booléen. Il aurait mieux valu l'appeler qqch comme `maybeProgramIndex`. - -:thumbsdown: -```cpp -auto uniform0to1() -> float { - thread_local std::default_random_engine gen{std::random_device{}()}; - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(gen); -} - -auto duniform0to1() -> double { - thread_local std::default_random_engine gen{std::random_device{}()}; - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(gen); -} -``` -Ces deux fonctions pourraient partager le même `std::default_random_engine`, ça éviterait d'en créer trop : -```cpp -static auto random_engine() -> std::default_random_engine& -{ - thread_local std::default_random_engine gen{std::random_device{}()}; - return gen; -} - -auto uniform0to1() -> float { - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(random_engine()); -} - -auto duniform0to1() -> double { - thread_local auto distrib = std::uniform_real_distribution{0.0, 1.0}; - - return distrib(random_engine()); -} -``` - -:thumbsdown: -```cpp -void Object3D::render(Renderer const &renderer) { - if (!this->glProgramIndex) { - return; - } -``` -C'est bien de générer une erreur (assert ou exception, ici une assert est plus adapté) pour vous aider à débuguer le jour où votre mesh ne s'affichera pas parce que vous avez oublié de set le programme. \ No newline at end of file diff --git a/content/evaluation/Matthieu M.md b/content/evaluation/Matthieu M.md index c839f88ad..e68ef7e49 100644 --- a/content/evaluation/Matthieu M.md +++ b/content/evaluation/Matthieu M.md @@ -68,19 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - -```cpp -int _type = 0 ; //0 for fish 1 for star; -``` -Vous pouvez faire un enum pour ça : -```cpp -enum class BoidType{ - Fish, - Star, -}; - -BoidType _type = BoidType::Fish; -``` - -Énormément de code dupliqué dans votre `main()` pour la création des objets OpenGL. Vous auriez pu faire une classe qui aurait fait tout ça automatiquement. \ No newline at end of file diff --git a/content/evaluation/Maxime V.md b/content/evaluation/Maxime V.md index d1fe3f0a7..38930b8cc 100644 --- a/content/evaluation/Maxime V.md +++ b/content/evaluation/Maxime V.md @@ -68,79 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -inline glm::vec3 get_position() { return m_position; }; -``` -Cette méthode peut-être marquée `const`: -```cpp -inline glm::vec3 get_position() const { return m_position; }; -``` - -```cpp -if(m_position.x > 45) m_position.x = -45; -``` -Évitez d'hardcoder la taille du cube, surtout qu'elle est utilisée à la fois dans l'arpenteur et dans les boids. Faites plutôt une variable dans un .hpp que vous pouvez inclure là où vous en avez besoin. - -```cpp -void Boid::update(float dt, float aspect_ratio, -std::vector boids, float avoidance_multiplicator, float alignement_multiplicator, float centering_multiplicator) -{ - glm::vec3 force = compute_forces(boids, avoidance_multiplicator, alignement_multiplicator, centering_multiplicator); -``` -Vous pouvez faire une struct pour grouper les différents multiplicator. Comme ça le jour où vous voudrez en rajouter un, il suffira de changer la struct, plutôt que de devoir changer la signature de toutes les fonctions qui utilisent ces multiplicators (update(), compute_forces(), etc) - -```cpp - value.x = -2*velocity.x; - value.y = -2*velocity.y; - value.z = -2*velocity.z; -``` -vous pouvez faire les opérations sur es vecteurs directement: -```cpp - value = -2*velocity; -``` - -```cpp -class Light -{ - private: - glm::vec3 m_position; - glm::vec3 m_intensity; - public: - Light(glm::vec3 pos, glm::vec3 intensity) : m_position(pos), m_intensity(intensity){}; - inline glm::vec3 get_position() const {return m_position;}; - inline glm::vec3 get_intensity() {return m_intensity;}; - inline void set_position(glm::vec3 pos) {m_position = pos;}; - inline void set_intensity(glm::vec3 intensity) {m_intensity = intensity;}; -}; -``` -Une classe qui a juste des getters et setters triviaux, ça peut être écrit plus simplement comme une struct : -```cpp -struct Light -{ - glm::vec3 position; - glm::vec3 intensity; -}; -``` - -```cpp - Vbo cube_vbo(1); - cube_vbo.gen(); -``` -Si une méthode doit toujours être appelée après la construction de l'objet, alors elle devrait directemennt faire partie du constructeur. Ca évite le risque d'oublier de l'appeler. Une fois qu'un objet est construit `VBO cube_vbo(1);`il est censé être déjà valide et utilisable. - -Vous avez beaucoup de code mort, par ex -```cpp - Force avoidance(params._multiplicator_avoidance); //Tendance à augmenter la distance inter-boids - Force alignement(params._multiplicator_alignement); //Tendance à former des gros groupes facilement - Force centering(params._multiplicator_centering); //Tendance à diminuer le rayon d'un groupe de boid -``` -qui ne sont utilisées nulle part. - -```cpp -std::vector e = flock.get_boids(); -``` -Attention vous faites une copie qui peut être coûteuse si il y a beaucoup de boids ! Prenez plutôt une référence : -```cpp -const std::vector& e = flock.get_boids(); -``` \ No newline at end of file diff --git a/content/evaluation/Melodie K.md b/content/evaluation/Melodie K.md index d6bf81155..9223ef725 100644 --- a/content/evaluation/Melodie K.md +++ b/content/evaluation/Melodie K.md @@ -68,25 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -switch (options.getLods()) -{ -case 1: - verticesSphere = glimac::sphere_vertices(1.f, 32, 16); - break; -case 0: - verticesSphere = glimac::sphere_vertices(1.f, 10, 4); - break; - -default: - verticesSphere = glimac::sphere_vertices(1.f, 32, 16); -}; -``` -Plutôt que de recréer le mesh à chaque frame, il aurait mieux valu créer toutes les variantes une fois pour toutes au début de l'application. Ca aurait été plus performant. - -```cpp - std::vector _swarm; // vecteur qui contient tous les boids - float _size; -``` -Pas besoin de stoker la taille, on peut déjà y accéder avec _swarm.size() \ No newline at end of file diff --git a/content/evaluation/Nina G.md b/content/evaluation/Nina G.md index 65ca5b509..43e46047e 100644 --- a/content/evaluation/Nina G.md +++ b/content/evaluation/Nina G.md @@ -68,50 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -std::vector boids(75); -for (auto& boid : boids) -{ - boid.set_speed(); - boid.set_position(); -} -``` -set_speed() et set_position() auraient pu être fait dans le constructeur de Boids. -Et pareil pour ce load_model() : -```cpp -Model ladybug = Model(); -ladybug.loadModel("dragonfly.obj"); -``` - -```cpp - GLuint textureLadybug; - glGenTextures(1, &textureLadybug); - glBindTexture(GL_TEXTURE_2D, textureLadybug); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_ladybug.width(), img_ladybug.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_ladybug.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); -``` -Vous auriez pu faire une fonction pour loader une texture. Ca vous aurait évité de devoir dupliquer le code pour chaque texture que vous loadez. - -```cpp - ladybug.~Model(); - personModel.~Model(); - barque.~Model(); - ocean.~Model(); - ile.~Model(); - trees.~Model(); - pirate.~Model(); - cone.~Model(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. - -```cpp -class LoadShader { -private: - std::filesystem::path m_vertexShader; - std::filesystem::path m_fragmentShader; -``` -Pas besoin de stocker les chemins dans la classe, ils ne servent que pendant le constructeur pour loader le shader. \ No newline at end of file diff --git a/content/evaluation/Romane M.md b/content/evaluation/Romane M.md index d6bf81155..9223ef725 100644 --- a/content/evaluation/Romane M.md +++ b/content/evaluation/Romane M.md @@ -68,25 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -switch (options.getLods()) -{ -case 1: - verticesSphere = glimac::sphere_vertices(1.f, 32, 16); - break; -case 0: - verticesSphere = glimac::sphere_vertices(1.f, 10, 4); - break; - -default: - verticesSphere = glimac::sphere_vertices(1.f, 32, 16); -}; -``` -Plutôt que de recréer le mesh à chaque frame, il aurait mieux valu créer toutes les variantes une fois pour toutes au début de l'application. Ca aurait été plus performant. - -```cpp - std::vector _swarm; // vecteur qui contient tous les boids - float _size; -``` -Pas besoin de stoker la taille, on peut déjà y accéder avec _swarm.size() \ No newline at end of file diff --git a/content/evaluation/Salome T.md b/content/evaluation/Salome T.md index bfd1cc297..3a6623197 100644 --- a/content/evaluation/Salome T.md +++ b/content/evaluation/Salome T.md @@ -68,55 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - 🌞 Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -int state; // State of the boid (0 = normal, 1 = blue, 2 = green, 3 = red and predator) -``` -Vous pouvez utiliser un enum pour ça : -```cpp -enum class BoidState{ - Normal, - Blue, - Green, - RedPredator, -}; - -BoidState state; -``` - -```cpp -Model alg = Model(); -alg.loadModel("alg.obj"); -``` -Faites plutôt le load_model() directement dans le constructeur de Model ça sera plus simple. - -```cpp - GLuint bakeSkybox = 0; - glGenTextures(1, &bakeSkybox); - glBindTexture(GL_TEXTURE_2D, bakeSkybox); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_water.width(), img_water.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_water.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); - - GLuint bakeAlg = 0; - glGenTextures(1, &bakeAlg); - glBindTexture(GL_TEXTURE_2D, bakeAlg); - - glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, img_alg.width(), img_alg.height(), 0, GL_RGBA, GL_UNSIGNED_BYTE, img_alg.data()); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR); - glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR); - glBindTexture(GL_TEXTURE_2D, 0); -``` -Vous auriez pu faire une fonction qui load une texture plutôt que de dupliquer le code à chaque fois. - -```cpp - kw.~Model(); - skybox.~Model(); - turtle.~Model(); - alg.~Model(); - chest.~Model(); - floatie.~Model(); -``` -Pas besoin d'appeler manuellement le destructeur, c'est fait automatiquement !! C'est tout l'intérêt d'un destructeur. Ne rien écrire aurait fait exactement la même chose. \ No newline at end of file diff --git a/content/evaluation/Sara L.md b/content/evaluation/Sara L.md index fc3e7d2f5..45660ef36 100644 --- a/content/evaluation/Sara L.md +++ b/content/evaluation/Sara L.md @@ -68,64 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp -inline void gen() { glGenVertexArrays(1, &this->id); }; -``` -Plutôt qu'une méthode gen(), vous auriez pu mettre ce code directement dans le constructeur. Ça vous aurait évité de devoir ensuite appeler gen() sur absolument tous les objets que vous créez. - -```cpp -class IBO { -std::vector indices; -inline void fill() { glBufferData(GL_ELEMENT_ARRAY_BUFFER, this->indices.size() * sizeof(uint32_t), this->indices.data(), GL_STATIC_DRAW); }; -// ... -}; -``` -Vous n'avez pas besoin de stocker les indices dans la classe, il ne servent qu'au moment de la construction. Il aurait mieux valu passer les indices en paramètre au constructeur, qui se serait ensuite chargé de faire le code de fill(). - -```cpp -for (size_t i = 0; i < allBoids.size(); i++) -``` -Vous auriez pu faire une boucle foreach : -```cpp -for (auto& boid : allBoids) -``` - -```cpp -if (this->flock[i].getRadius() == this->radiusLittleFish) -{ - texLittleFish.activate(); - texLittleFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texLittleFish.unbind(); -} -else if (this->flock[i].getRadius() == this->radiusMediumFish) -{ - texMediumFish.activate(); - texMediumFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texMediumFish.unbind(); -} -else -{ - texBigFish.activate(); - texBigFish.bind(); - this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); - texBigFish.unbind(); -} -``` -Pour éviter de dupliquer le code vous auriez pu faire comme ça : -```cpp -// D'abord on récupère une référence vers la bonne texture -auto& texture = this->flock[i].getRadius() == this->radiusLittleFish - ? texLittleFish - : this->flock[i].getRadius() == this->radiusMediumFish - ? texMediumFish - : texBigFish; - -// Et ensuite on fait le code qui utilise la texture -texture.activate(); -texture.bind(); -this->flock[i].run(this->flock, this->separationCoeff, this->alignmentCoeff, this->cohesionCoeff, wallSize, ctx, camMVMatrix, uniMVP, uniMV, uniNormal, fishVertices); -texture.unbind(); -``` \ No newline at end of file diff --git a/content/evaluation/Segolene L.md b/content/evaluation/Segolene L.md index 9948ddf9b..557e10ea1 100644 --- a/content/evaluation/Segolene L.md +++ b/content/evaluation/Segolene L.md @@ -68,24 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - -```cpp - int find_biome( - std::vector - &tableau_de_biomes) -``` -Passer par const& si vous ne modifiez pas le tableau. - -```cpp - void assign_DND_alignement(std::vector &tableau_de_biomes) { -``` -C'est quoi DND ? Évitez les abbreviations, ça rend votre code difficile à comprendre. - -```cpp -void Objet3D::clear() -{ - glDeleteBuffers(1, &vbo); - glDeleteVertexArrays(1, &vao); -} -``` -Plutôt qu'une fonction clear, faites un destructeur. Il sera appelé automatiquement c'est vachement pratique. \ No newline at end of file diff --git a/content/evaluation/Tancrede G.md b/content/evaluation/Tancrede G.md index c5642369e..53b4cc794 100644 --- a/content/evaluation/Tancrede G.md +++ b/content/evaluation/Tancrede G.md @@ -68,47 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - - -```cpp - float posX; - float posY; - float posZ; -``` -Vous auriez pû utiliser un glm::vec3, ça vous aurait simplifié la vie ! Par exemple au lieu de faire -```cpp - posX += (velocityX * ctx.delta_time()) * speedFactor; - posY += (velocityY * ctx.delta_time()) * speedFactor; - posZ += (velocityZ * ctx.delta_time()) * speedFactor; -``` -vous auriez pû faire -```cpp - pos += (velocity * ctx.delta_time()) * speedFactor; -``` - -```cpp -bool oscillationDirection = true; // true pour une augmentation, false pour une diminution -``` -Si on a besoin d'un commentaire pour comprendre cette variable, c'est qu'elle mériterait d'être mieux nommée. Par exemple : `oscillationsDoIncrease`. Ou alors carrément faire un enum : -```cpp -enum class OscillationDirection -{ - Increase, - Decrease, -} - -OscillationDirection oscillationDirection = OscillationDirection::Increase; -``` - -```cpp - void setLightIntensity(unsigned int index, float intensity) - { - if (index < lights.size()) - { - lights[index].intensity = intensity; - } - } -``` -Ça peut être bien de remonter une erreur quand l'index est invalide (via une assert, une exception, etc., au choix), plutôt que de silencieusement ne rien faire. Ça vous évitera de passer une heure à debuguer un `setLightIntensity()` qui ne fait rien, tout ça pour vous rendre compte à la fin que c'était juste l'index qui n'était pas bon. - diff --git a/content/evaluation/Tanya F.md b/content/evaluation/Tanya F.md index cd3a6b942..ea67f0aa3 100644 --- a/content/evaluation/Tanya F.md +++ b/content/evaluation/Tanya F.md @@ -68,54 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - -```cpp -struct Scene { -float cubeBaseSize = 10.f; // for reference only, do not touch -float groundBaseSize = 2.f; // for reference only, do not touch -``` -si ces valeurs ne doivent pas être changées, plutôt que de les mettre dans la struct vous auriez pu les mettre en constantes : -```cpp -static constexpr float cubeBaseSize = 10.f; -static constexpr float groundBaseSize = 2.f; -struct Scene { -``` -Même remarque pour -```cpp -class Flock { -// ... - std::vector _skinList = {"_red", "_orange", "_blue", "_green", "_blue", "_grey", "_monochrome"}; -``` -Cette _skinList est une constante partagée par toutes les flocks, pas besoin d'en stocker une copie dans chaque Flock. - -```cpp -Object3D::Object3D(const std::string& name, const std::string& vertexShaderPath, const std::string& fragmentShaderPath) - : _model(name), _texture(name), _shader(vertexShaderPath, fragmentShaderPath) -{ - defineVBO(); - defineVAO(); -}; - -Object3D::Object3D(const std::string& name, const std::string& vertexShaderPath, const std::string& fragmentShaderPath, const std::string& skinID, const std::string& lodID) - : _model(name + lodID), _texture(name + skinID), _shader(vertexShaderPath, fragmentShaderPath) -{ - defineVBO(); - defineVAO(); -} -``` -Plutôt que de dupliquer le constructeur, vous pouvez faire en sorte que l'un appelle l'autre, en rajoutant un troisième constructeur qui prend en paramètre model_name et texture_name. Et les deux autres peuvent déléguer à celui-là. - -```cpp -void Object3D::clear() -{ - glDeleteBuffers(1, &_vbo); // Delete the vertex buffer - glDeleteVertexArrays(1, &_vao); // Delete the vertex array -} -``` -Faites plutôt un destructeur, qui sera appelé automatiquement au bon moment. - -```cpp -_renderer.clearAll(); -``` -C'est confusant que vous utilisiez parfois le terme "clear" pour désigner une méthode qui détruit les objets, et d'autres fois pour désigner une méthode qui reset la couleur de l'écran et bind des choses. \ No newline at end of file diff --git a/content/evaluation/Thomas G.md b/content/evaluation/Thomas G.md index 47e54e5dc..dbe615f43 100644 --- a/content/evaluation/Thomas G.md +++ b/content/evaluation/Thomas G.md @@ -68,40 +68,3 @@ import LessonLink from "@site/components/LessonLink" - 🌞 Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - 🌞 Extend your project with additional features - -```cpp -auto get_stats_force = [&spawner] { - switch (which_force) { - case AVOID: - return spawner.stats().forces.avoid; - case MATCH: - return spawner.stats().forces.match; - case CENTER: - return spawner.stats().forces.center; - case BIAS: - return spawner.stats().forces.bias; - } - return std::vector(); -}; -``` -N'oublie pas les const ! C'est un des gros avantages des IIFE, de pouvoir avoir une variable const qui est le résultat d'un switch -```cpp -const auto get_stats_force = [&spawner] { - // ... -}; -``` - -```cpp -void Boid::setParameters(const BoidForceParameters& forces) { - _avoid_force = forces.avoid; - _match_force = forces.match; - _center_force = forces.center; - _bias_force = forces.bias; -} -``` -Si les forces étaient aussi stockées sous forme de struct dans la classe Boid, tu aurais juste à faire -```cpp -void Boid::setParameters(const BoidForceParameters& forces) { - _forces = forces; -} -``` \ No newline at end of file diff --git a/content/evaluation/Valentin G.md b/content/evaluation/Valentin G.md index ae51517c5..b7e3a0e52 100644 --- a/content/evaluation/Valentin G.md +++ b/content/evaluation/Valentin G.md @@ -68,12 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - 🌞 Extend your project with additional features - - -```cpp -class Texture { -private: - img::Image image; - GLuint imagePointer; -``` -Vous auriez pu faire un destructeur pour automatiquement appeler glDeleteTextures \ No newline at end of file diff --git a/content/evaluation/Zacharie P.md b/content/evaluation/Zacharie P.md index 27c082083..ccf1f2607 100644 --- a/content/evaluation/Zacharie P.md +++ b/content/evaluation/Zacharie P.md @@ -68,33 +68,3 @@ import LessonLink from "@site/components/LessonLink" - ☁️ Ask questions and participate in class - ☁️ Take my feedback into account, improve your old code if need be - ☁️ Extend your project with additional features - - -```cpp -public: - Boid(); - - void drawBoid(GLuint vao, GLsizei vertexCount, glm::vec3 scale, float rotation, glm::mat4 viewMatrix, glm::mat4 ProjMatrix, glm::mat4& NormalMatrix, ObjectProgram& ObjectProgram, GLuint textureID); - void applyVelocity(); - - vec getPosition() const; - vec getVelocity() const; - - // Propriétés du boid - vec distance(const Boid& boid1, const Boid& boid2); - void alignmentBoid(const std::vector& boids, float alignment); - void cohesionBoid(const std::vector& boids, float cohesion); - void separationBoid(const std::vector& boids, float separation); - - void updateBoid(const std::vector& boids, float alignment, float cohesion, float separation); -}; -``` -La plupart de ces fonctions pourraient être privées. - -```cpp -void Boid::drawBoid(GLuint vao, GLsizei vertexCount, glm::vec3 scale, float rotation, glm::mat4 viewMatrix, glm::mat4 ProjMatrix, glm::mat4& NormalMatrix, ObjectProgram& ObjectProgram, GLuint textureID) -{ - renderObject(vao, vertexCount, position, scale, rotation, viewMatrix, ProjMatrix, NormalMatrix, ObjectProgram, textureID); -} -``` -Tu aurais pu faire une struct pour grouper tous ces paramètres, ça aurait évité de les dupliquer entre drawBoid() et renderObject() \ No newline at end of file