So over all your code looks quite fine for a first project.
Turning on some more warnings in the compiler, immediately reveled two problems in your code:
eye.h||In constructor 'Eye::Eye(Eye::Eyeball)':|
eye.h|84|warning: 'Eye::eyeball_lid_width' will be initialized after [-Wreorder]|
eye.h|70|warning: 'float Eye::scaleX' [-Wreorder]|
eye.h|25|warning: when initialized here [-Wreorder]|
eye.h|71|warning: 'Eye::scaleY' will be initialized after [-Wreorder]|
eye.h|68|warning: 'float Eye::MOVE_SPEED' [-Wreorder]|
eye.h|25|warning: when initialized here [-Wreorder]|
eye.h|68|warning: 'Eye::MOVE_SPEED' will be initialized after [-Wreorder]|
eye.h|67|warning: 'bool Eye::blinking' [-Wreorder]|
eye.h|25|warning: when initialized here [-Wreorder]|
eye.h|67|warning: 'Eye::blinking' will be initialized after [-Wreorder]|
eye.h|66|warning: 'int Eye::frame_counter' [-Wreorder]|
eye.h|25|warning: when initialized here [-Wreorder]|
eye.h|25|warning: 'Eye::texture_eye_lid' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::texture_eye_white' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::texture_eye_color' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::sprite_eye_lid' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::sprite_eye_white' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::sprite_eye_color' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::rendered_eye' should be initialized in the member initialization list [-Weffc++]|
eye.h|25|warning: 'Eye::eye_bound' should be initialized in the member initialization list [-Weffc++]|
nose.h||In constructor 'Nose::Nose()':|
nose.h|8|warning: 'Nose::texture_nose' should be initialized in the member initialization list [-Weffc++]|
nose.h|8|warning: 'Nose::sprite_nose' should be initialized in the member initialization list [-Weffc++]|
nose.h|8|warning: 'Nose::nose_width' should be initialized in the member initialization list [-Weffc++]|
nose.h|8|warning: 'Nose::nose_height' should be initialized in the member initialization list [-Weffc++]|
When initializing class members in the initialization list, you have to initialize them in the order you've declared them.
And for the second problem, you should always initialize all the variables (that don't have default constructor) like native types (int, float, etc) in the initialization list, otherwise, if you forget to init them later and then try to access them, the behavior is undefined.
One a bit more serious thing you should not do and you could eventually run into problems, is using
std::vector<sf::Texture>, because every time you push_back a new element, all elements could get moved to a different address in memory, which will then break all the references of any
sf::Sprite. It's probably better to use either a different container like
std::deque or
std::map, or use a smart pointer in the vector:
std::vector<std::unique_ptr<sf::Texture>>.
Then again when thinking about those multiple textures, you should rather use one single texture with all the images on it and simply use
sf::Sprite::setTextureRect() to actually choose which part you want.
In connection with the textures, you're using a container that can hold an arbitrary amount of elements, while you actually expect to have exactly 4 textures in there, with the exact order specification. This is very problematic, since someone who will use your class, could forget about loading lid1 before lid2 or only load 3 textures, which will eventually lead to a crash, when trying to access texture 4. This correlates with the often stated fact, that one should try to avoid 'magic numbers' as often as possible. And if one uses fixed values, one should also make sure that the user of class isn't able to actually break the code.
So if I were you, I'd create one texture with everything on it and then either use an external file, that defines the different rectangles for the different images, or implement them fixed
within the class itself. Then instead of letting the class load the texture themselves, I'd only pass a
std::shared_ptr to the class. Now if the image isn't the correct one or none is given at all, you'll only get a bad looking result instead of a crash.
Your movement code is depending directly on the framerate, thus if it's higher as expected then the eyes will move way to quite and the other way around.
Now to a few code design decisions:
You're using quite a heavy interface for the Eye class, which doesn't really seem to be necessary, except maybe for the debug output, which on the other hand could also happen within the class. Do you really need all the get...Width/get...Height? If you really need them, then I suggest you use a
sf::Vector2, so you can at least remove every other function.
It's quite common in C++ programming to prefix member variables, e.g. I usually use
m_, so it's clear at one glance where the variables come from. If you see a none prefixed variable, it's a var local to the current scope, if it has a m_ prefix, it's a member of the class.
Then you should probably use a bit a more consistent naming convention. At the moment you for instance have
bool eyeball_position,
float MOVE_SPEED and
scaleX. As said before you should use a
sf::Vector2 for scale, so you can drop the X and Y. For the uppercase variable, I usually use uppercase for constants. Mixing different styles can lead to wrong assumptions when reading the code, which then can often lead to long bug hunts...
Instead of requesting the sprites every frame iteration, you could derive your class from sf::Drawable, implement the draw function and get a bit a nicer syntax:
window.draw(eye);If you're working with booleans or boolean expressions, one can often do a bit better than always checking with
if. It's not really wrong and can help for the beginner, but it's also not necessary and in some cases can be very usful. For example:
void Eye::blink()
{
blinking = !blinking;
}
or
bool Eye::loadEyeLid( std::string file )
{
sf::Texture tmp;
texture_eye_lid.push_back( tmp );
return texture_eye_lid.back().loadFromFile( file);
}
or
bool Eye::loadEyeColor( std::string file )
{
if( !texture_eye_color.loadFromFile(file) )
return false;
eyeball_color_width = texture_eye_color.getSize().x;
eyeball_color_height = texture_eye_color.getSize().y;
sprite_eye_color.setTexture(texture_eye_color);
return true;
}
Although it makes sense to include things at the point where you actually need it, it's better to include everything you need for the file in the beginning (i.e. put
#include<iostream> in eye.cpp at after the eye.h include). So people will immediately see what your using in that file and don't have to searching for all the includes between the functions.
Last but not least, I'd suggest to use matching .hpp/.cpp file endings. Some if one could eventually determine on the header file ending, if it's a C file or a C++ file.
I hope, I could point you in some directions.
