The "design" of sf::View is broken.

[Jefffrey]

Considering a 2D game in which the camera is supposed to move often (actually at every frame). Why is the design of sf::View made so that the only way to edit an sf::View (e.g.: move it by a certain offset) involves the copying, the editing and the setting of the new view?

Does:

    sf::View copy = window.getView();
    copy.move(x);
    window.setView(copy);

makes more sense then:

    window.getView().move(x);

?

Also, what does it mean that sf::View is supposed to be used "by value"? What does it really contain? Why does it need to be used by value?

- Thanks

[eXpl0it3r]

The idea is to ise your own view and directly manipulate that, thus you'll only have to set it each frame.

The design decisions involve multiple step, one is that saving the reference can lead to undefined behavior if the window get destroyed and the reference still gets used, which can lead to crashes and since SFML tries to be a library that doesn't just crash, it's not really an option.
I think there's also more to it regarding the implementation but I don't know right now.

[Nexus]

In SFML 1, sf::Window allowed to set a view by reference. This was changed to store a value (copy), because this approach is simpler and more flexible. One can pass temporary objects and doesn't need to manage the lifetime of the view. Furthermore, the signature is consistent to other setters that copy their parameters. By the way, performance is definitely not a concern here, and thus not a relevant factor for the design.

If the current way of moving the view is cumbersome for you, then write a global function for this task.

[Jefffrey]

Yes, I understand that storing a reference is "bad" design, because you can incur in a lot of problem. I never meant that. But what I really meant is: why not store the sf::View within the classes that needs it (for example sf::RenderWindow), as I believe is right now, and be able to return a reference to it that is not constant.

Here's an example:

namespace sf {
    class RenderWindow {
    private:
        View m_view;
    public:
        // ...
        View& getView(); // this is key point here, this is the function I'd like
        const View& getView() const; // this already exists
    };
};

If sf::RenderWindow stores a copy (as apparently it does) why not allow to modify that copy? The above design is constant correct and avoid the creation of a new sf::View every time you have to really modify the current one.

By the way, performance is definitely not a concern here, and thus not a relevant factor for the design.

I agree that the performance loss of copying a sf::View in most cases can be ignored. But if you have a clean way to do something better, don't you do it just because it is not a relevant factor perfomance-wise?

[Ixrec]

Because setView() does a bit more than just assign the view to a member.  That "bit more" is something you shouldn't have to think about, and don't have to under the current API.

Since there's no performance problem (no one changes views that often), encapsulation takes priority.

[FRex]

(no one changes views that often)

I'd say many people probably do it every frame even if it's redundant because camera moved insignificantly or not at all..

[Ixrec]

You're right, I forgot about that case since my program doesn't require that.  Correction:

Performance isn't a big issue because a View is just a dozen PODs or so.

[binary1248]

From a performance point of view, the current implementation is so easily optimizable that if you try profiling your code, setting the view shouldn't even show up in the hot spot list unless your compiler did something wrong. If SFML had C++11 support, the move would make it even less significant than it already is. In this respect, your suggestion would indeed fall into the category "premature optimization".

From an interface design point of view, the current implementation follows the idea that you should really only be able to mutate an object through its mutator methods. Since the sf::View is part of the sf::RenderWindow, mutating it should only be possible through the sf::RenderWindow interface. And if you follow convention, getter methods should be const and thus only return const references if not PODs. Getting a view in order to change it in its owner doesn't convince me of a clean interface even if it saves those few lines of code, which really has to only be done once in a well designed application. As many C++ experts say regarding modern C++, pass by const reference if you want to inspect the object, pass by value if the object is a sink object (only makes sense if there is move support). The same can be said about returning values as well.

[Ixrec]

Just to be devil's advocate for a moment:

We could always add this method:

void sf::RenderWindow::setView (const FloatRect &rectangle) {
    sf::View v(rectangle);
    setView(v);
}

Seems like it would make him happy (lets you change the view in one line) while preserving all the Good Things about the current interface (only mutate through mutators, etc).  Though it's such a minor thing I'm not sure I personally care either way.

[Nexus]

why not store the sf::View within the classes that needs it (for example sf::RenderWindow), as I believe is right now, and be able to return a reference to it that is not constant.

Because returning a non-const reference breaks encapsulation, it's almost like a public member. It makes the implementation much less flexible: The window cannot perform additional tasks or cache the value when the view is set, because it simply doesn't know when. To return a view, there must be an internal member that stays valid as long as the user accesses the reference to it; it is therefore impossible to compute a view on the fly.

By the way: It's not the reference which is constant or not; the "const" in "const reference" stands for the referenced object.