I have created a button class that just contains a texture and sprite. However, when I create an array of these Buttons, window.draw(buttons[0].sprite) doesn't draw the sprite.
Ok. Let's do a little code review.
Here is the class "Button":
#include <stdio.h>
#include <SFML/Graphics.hpp>
#include "ResourcePath.hpp"
class Button {
public:
Button();
Button(sf::Texture myTexture, sf::Vector2f pos);
sf::Sprite sprite;
sf::Texture texture;
};
First of all, since you don't actually do anything in the default constructor, if you want to have one then a more explicit and concise way to declare it would be
Button() = default;
which let's you get rid of the empty implementation and guarantees that it will be
trivial.
Secondly, your other constructor takes a sf::Texture by value - you probably don't want to do that. Textures are expensive to copy. It would probably be better to take a reference or pointer (if the caller owns the texture) or a
std::unique_ptr if the Button should take ownership of the texture.
I would also swap the declaration order of the
sprite and
texture members of the class. The members will be constructed in the declared order and destructed in the reverse order - and since the sprite will use/depend on the texture it should be destroyed first, before the object it depends on - you need to think about object lifetimes and dependencies - what if the sprite (or any object in general) needes to do something with its texture in its destructor and that object has been destroyed first?
Additionally I'd make the
texture and
sprite members
private: and add a draw() member to the class (after inheriting from sf::Drawable) so the button can handle its own drawing and don't expose implementation details to the world.
Button::Button(sf::Texture myTexture, sf::Vector2f pos) {
texture = myTexture;
if (!texture.loadFromFile(resourcePath() + "cute_image.jpg")) {
return EXIT_FAILURE;
}
sprite = sf::Sprite(texture);
sprite.setPosition(pos);
}
There are a number of things wrong here.
1. You need to deside if the caller should load the texture or if the Button class should do it.
If the Button class is in charge of loading, then it doesn't make much sense for the constructor to take a texture as argument at all - it can just create the texture itself. If the caller should load the texture then get rid of the .loadFromFile inside the ctor. In any case, the
texture = myTexture
directly followed by texture.loadFromFile makes no sense at all. Why should the caller pass in a texture if you are just going to instantly replace its contents anyway?
2. You can't return values from constructors, so
return EXIT_FAILURE;
is nonsense. The only two sensible options a constructor has are A) fully construct the object to a sane working state or B) throw an exception.
(yes, it could also take an out parameter to signal error or have a getError() member function, but those options are just plain ugly and wrong, so I'll pretend they don't exist).
3. You really should use an
initializer list to initialize your class members. If we assume you changed the signature to take a std::unique_ptr<sf::Texture> to a caller loaded texture (and fixed the member order), then it would look something like this:
Button::Button(std::unique_ptr<sf::Texture> myTexture, sf::Vector2f pos)
: texture(std::move(myTexture)), sprite(texture)
{
sprite.setPosition(pos);
}
.
An here is my main():
int main(int, char const**)
If you don't need/want
argc or
argv then just declare main as
int main()
that's perfectly valid.
sf::Texture texture;
if (!texture.loadFromFile(...
There you go, loading the texture in the caller, so why did you do it again in the button ctor?
Button *buttonArr = new Button[5];
I'd, almost always, use a
std::vector. Perhaps a
std::array, But very,
very rarely a built-in array.
And then there's your memory leak of course (as was already pointed out) - learn about
RAII and don't use naked
new's.
buttonArr[0] = Button(texture, sf::Vector2f(0, 0));
As was already pointed out to you, this does a copy you do not want.
return EXIT_SUCCESS;
The standard guarantees that this is the return value of main if you don't return anything, so it's sort of redundant.
I hope that's of some use.
Have a nice evening. :-)
