Comments on Week 1 #1
Conversation
include/core/particle_controller.h
Outdated
| namespace idealgas { | ||
| using glm::vec2; | ||
|
|
||
| struct Particle{ |
There was a problem hiding this comment.
I don't think a struct is the best option here. There are a lot of operations that the Particle should be doing, like updating position, handling collision, and checking if it should be colliding, which is why having a Particle Class would be a good idea.
include/core/particle_controller.h
Outdated
| class ParticleController { | ||
| public: | ||
| ParticleController(); | ||
|
|
There was a problem hiding this comment.
I also think it would make sense to have more functions here to help control the particles throughout the simulation. Otherwise, everything is being done by the Cinder App, which means you really have nothing that you can test.
There was a problem hiding this comment.
Also, it looks like you have a simulation class, which is essentially the same thing that you have here, but with more functionality. If you aren't going to use the ParticleController, be sure to get rid of it in your final version of the project.
src/visualizer/simulator.cc
Outdated
| top_left_corner_ + vec2(simulator_size_, simulator_size_); | ||
| ci::Rectf pixel_bounding_box(top_left_corner_, pixel_bottom_right); | ||
| ci::gl::drawStrokedRect(pixel_bounding_box); | ||
| ci::gl::color(ci::Color("black")); |
There was a problem hiding this comment.
Try to avoid hardcoding the colors like this.
src/visualizer/simulator.cc
Outdated
|
|
||
| void Simulator::UpdateParticlePosition() { | ||
| for(int i = 0; i < particles.size(); ++i){ | ||
| if (particles[i].pos_.x - 7 <= top_left_corner_.x && |
There was a problem hiding this comment.
Again, there is a lot of magic numbers and strings throughout this class implementation to look out for. There is really no reason for a lot of these, since particles should be storing their radius themselves.
src/visualizer/simulator.cc
Outdated
| particles[i].vel_.y > 0){ | ||
| particles[i].vel_.y = -1 * particles[i].vel_.y; | ||
| } | ||
| for(int j = 0; j < particles.size(); ++j){ |
There was a problem hiding this comment.
I would start by looking at particles after i in the list, that way you avoid checking every particle pair twice.
include/visualizer/simulator.h
Outdated
| */ | ||
| void Clear(); | ||
|
|
||
| double GteSimulatorSize() const; |
There was a problem hiding this comment.
Double check for name typos.
Comments on Week2
Week 1 Comments