Skip to content

Commit

Permalink
Remove comments
Browse files Browse the repository at this point in the history
  • Loading branch information
JulesFouchy committed May 23, 2024
1 parent 0bb161e commit fdbe0f4
Show file tree
Hide file tree
Showing 42 changed files with 26 additions and 2,102 deletions.
26 changes: 26 additions & 0 deletions content/bob.py
Original file line number Diff line number Diff line change
@@ -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)
58 changes: 0 additions & 58 deletions content/evaluation/Adelie F.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boid> vec);
Boids(const int number);
std::vector<Boid> 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<Boid> 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 !
76 changes: 0 additions & 76 deletions content/evaluation/Alexandre M.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boid> 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<Boid> 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<Boid>& e = flock.get_boids();
```
81 changes: 0 additions & 81 deletions content/evaluation/Baptiste J.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
34 changes: 0 additions & 34 deletions content/evaluation/Camille L.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boid>& boids, float separation);
vec calculateCohesionForce(const std::vector<Boid>& boids, float cohesion);
vec calculateAlignmentForce(const std::vector<Boid>& boids, float alignment);
void applySteeringForces(const std::vector<Boid>& 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
97 changes: 0 additions & 97 deletions content/evaluation/Coralie B.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Boid> getGroup();
int getSize();
glm::vec<DIMENSION, float> wallForce(Boid boid, float posWall, int coord, float rayonMur, float forceMur);
glm::vec<DIMENSION, float> separationForce(glm::vec<DIMENSION, float> vector, float distance, float rayonSeparation, float forceSeparation);
glm::vec<DIMENSION, float> cohesionForce(glm::vec<DIMENSION, float> 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<int>(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.
}
```
Loading

0 comments on commit fdbe0f4

Please sign in to comment.