SFML Game Development - Use of assert in resource holder

[eXpl0it3r]

So I've been reading parts of the book on my vacation and I've noticed a little issue. It's more a question of deign than an actual bug. The implementation of the resource holder makes use of assert, which is great and it's explained quite nice, but the issue is, that there's no way to prevent stepping into the assert, except by logical exclusion up on programming. As we probably all agree, we should write code that leaves as less doors for making mistakes as possible, thus having no way to check, whether a resource already exists from outside of the class, forces the user to either step into the assert or make unnecessary comments everywhere, which might get over read later on, to prevent the loading of two identical resources/with the same ID.

My conclusion is, that the resource holder class should have a exists() function to check the existence of resources, before loading them and praying it won't blow up in the assert. I mean one doesn't necessarily have to check the existence every time, but having no option to do so, seems rather dangerous/bad.

For examples:

TextureHolder textures;

if(!textures.exists(Textures::Airplane))
{
    std::vector<std::pair<Textures::ID, std::string>> textures_assets = load_textures_info_from_file();

    for(auto& asset : textures_assets)
    {
        if(!textures.exists(asset.first))
            textures.load(asset.first, asset.second);
    }
}
 

I know the other code looks prettier and one doesn't always have to check it if the logic allows it, but letting the user step into asserts because there's no way to check any kind of existence seems like a bad design.
Sure asserts are there for debugging, but you can't guarantee that all the possible scenarios will be played out, while running a debug build, thus releasing a game with possible bugs.

What do you guys think? Why didn't you implement such a function?

What about a separate forum for the book?

[Hiura]

Assertions are nice for debugging and when designing a library, to make sure internal state are valid. But when there is an interface with the user code it's not the best tool. I would instead recommend exception or return code (for simpler case) instead.

[Nexus]

The philosophy behind ResourceHolder is quite simple: You load the resources initially and once, and you use them again and again. At the time you use the resources, you require them to be already loaded.

An exists() method provokes a different design: Users don't have a clear owner and scope of the resource anymore. That is, the ResourceHolder cannot guarantee the availability of resources, and code accessing becomes more complicated because it needs to check for the existence before using the resources.

I agree that more sophisticated scenarios such as loading on-demand require a more complex API. But exists() is only the first step towards it; eventually you want to hide the case differentiation and simply request the resource, be it allocated or not. As soon as you have this, you might also require the opposite, namely releasing a resource as soon as it is not used anymore. You will end up with a much more complex design, similar to thor::ResourceCache.

Assertions are nice for debugging and when designing a library, to make sure internal state are valid. But when there is an interface with the user code it's not the best tool. I would instead recommend exception or return code (for simpler case).

Assertions are fine to check preconditions in functions. There's a long section in the book that explains exactly why we use assertions and not return types or exceptions

[Hiura]

It depends on the kind of preconditions. Quoting Andrew Koenig:

I believe, therefore, that when a program discovers something that is irrefutably wrong with its internal state, it is better off terminating at once, rather than giving its caller the opportunity to pretend that nothing is wrong.

If you like, I think that exceptions should be reserved for situations in which it is possible to do something sensible after catching the exception. When you discover a condition that you thought was impossible, it's hard to say much about what might happen afterward.

[Tank]

Nexus, the philosophy is alright, but you still allow to get() a specific resource which carries an assert(), without a way to prevent trapping into it. This will only happen in typo scenarios mostly, but the interface still forces me to trial'n'error.

The developer is not able to guarantee that all preconditions are met before calling get() without remembering what gets loaded in own code.

An exception in this case would fit better in my opinion: The developer can't avoid the error case, thus you can't assert that the certain preconditions are met.

[Tank]

Assertions are nice for debugging and when designing a library, to make sure internal state are valid. But when there is an interface with the user code it's not the best tool. I would instead recommend exception or return code (for simpler case) instead.

I do not agree at all. Personally I separate between programming errors and exceptions. A programming error for example is this:

resource_manager.get( "non_existant" );

If there's no resource with the ID "non_existant", and the resource manager provides me with a function to check for that resource prior getting it, then I've used the class wrong. This is a perfect case for assert(). (By the way: make sure to run your stuff without NDEBUG/asserts enabled!)

If I do this, however:

resource_manager.load( "cool_stuff.png" );

and the file can't be loaded, then it's an exception, because I can't guarantee beforehand that at the time the file is going to be read, it exists, is readable etc. Ergo an exception should be thrown.

Lastly, my opinion on returning error codes: Just don't do it. C++ provides us with exceptions, so I don't see a reason for using error codes. They require me to check them every time (I can simply overlook them) and don't scream when I forget to do so. They also go down the whole callstack, which is awesome for error handling delegation.

By the way, bool values are also some kind of error codes. Personally I'd design, for example, sf::Image::loadFromFile() to be a void function. Laurent, would you still follow the same way of designing the functions or choose another if you'd start now? I ask because I changed my way of doing this too at some time.

[Ixrec]

I think returning booleans as error codes for functions like loadFromFile is much better than leaving them void.  With a function like that, there's such an obvious, intuitive, probable and easy to handle error case (the file can't be loaded) that any programmer using the function really should be expected to check the return value and have a contingency plan for when it fails in that obvious way.  If that was an exception instead, we'd merely be wrapping every loadFromFile in a try/catch block instead of an if(!) statement, and I'm pretty sure the latter is more intuitive, more concise, and probably less expensive to implement.

Of course most functions aren't like this, non-bool error codes are definitely bad, and asserts and exceptions should be used for the majority of error handling, but making loadFromFile return void instead of bool would just take away useful, easy-to-understand information for no good reason, in my opinion.

[Laurent]

Actually, I'm already thinking about using exceptions in SFML 3.

I had two arguments in favor of error codes:
1. no error is fatal, SFML always manages to provide a valid fallback in case of error
2. exceptions are hard to translate to C (and don't talk about long jumps, I'm referring to design issues)

But:
1. displaying a white texture is not what the user wants, and doing it is much less helpful than throwing an exception with an explicit error message
2. if I manage to translate exceptions at the C level, then error handling will be much easier in bindings (many people try to catch sf::err() output from their binding to retrieve error messages, to no avail)
3. exceptions are really the way to go in C++

[Ixrec]

SFML 3 might throw an exception when textures go out of scope?

That sounds amazing.

[Laurent]

No no no. This would be crazy, there's nothing wrong with going out of scope (every objects ends up being out of its scope at some point)

SFML 3 will throw an exception on error instead of returning false. For example, if loadFromFile fails.

[Nexus]

An exception in this case would fit better in my opinion: The developer can't avoid the error case, thus you can't assert that the certain preconditions are met.

load() throws an exception upon failure, because not being able to load a resource is a runtime error, which can indeed not be avoided. However, accessing a wrong resource is a logic error; it implies a bug in the program which can be avoided and which won't happen if the program is correctly written.

If that was an exception instead, we'd merely be wrapping every loadFromFile in a try/catch block instead of an if(!) statement, and I'm pretty sure the latter is more intuitive, more concise, and probably less expensive to implement.

That's exactly how you don't handle exceptions. Other than Java code, C++ code that uses exceptions typically has very few try-catch statements -- one at the top (e.g. in main()), and possibly some in places where you need exception translation, logging, or rollback semantics. The big advantage of exceptions is that you can keep code clean of error handling.

SFML 3 might throw an exception when textures go out of scope?

That sounds amazing.

Amazingly insane

A destructor should never let an exception escape (GotW #47).

[Hiura]

However, accessing a wrong resource is a logic error; it implies a bug in the program and won't happen if the program is correctly written.

Not necessarily. If you have some config file to, well, configure your app and customize your images or anything else, you cannot be sure that it will always be valid.

Also, there are different kind of exceptions. For example std::logic_error, and many more.

[Nexus]

That's right. But as mentioned previously, to cover all these cases, the design should be fundamentally different, a failing get() alone isn't enough. ResourceHolder is very simple in its implementation, yet covers the typical use cases (load resources initially, use them later). I think it's a good tradeoff between complexity and genericity. Like many techniques described in the book, you should not consider it the solution to every problem, but rather an introduction to the topic. Keep in mind that the book doesn't require previous game development knowledge, so we can't directly start with complex use cases such as config files, shared ownership or loading on-demand

In the C++ community, there has always been a controversy about the utility of std::logic_error and its derived classes. I personally tend to use assert to report logic errors, but there are clearly scenarios where exceptions make sense, most of which are related to user input that is directly forwarded. But some are really questionable, e.g. std::vector::at() which throws std::out_of_range. I've seen a lot of people use it in the expectation that it is somehow safer than operator[], but in the end, you sacrifice a lot of performance for an error which you can't meaningfully react to (other than terminating the program and writing a log, for example). The philosophy differs also strongly between languages, Java for example uses exceptions for everything, and even has strong exception specifications.

[Hiura]

Keep in mind that the book doesn't require previous game development knowledge

My apologies, my answers were not really targeted to the book but more on general programming practice. If we were talking about a book about designing and building software, I stand on my position. But here it's indeed more «affordable» to use assertions, since its target audience is more beginners than professional.

[Tank]

However, accessing a wrong resource is a logic error; it implies a bug in the program which can be avoided and which won't happen if the program is correctly written.

I don't think that it can always be avoided if "correctly written". Let's say, for example, a library prepares a resource manager with some default resources. A newer version renames the identifiers, so calling get() with the old ones leads to an assertion failure. I have no way of avoiding programming errors.

And generally, using a container or similar for tracking what I've put into the resource manager and what not is no real option for me. What exactly does ResourceManager::isLoaded() add to the complexity?

Granted, the examples/scenarios are often to be found in more complex source code, but in my opinion that's not a valid reason for not choosing a solution that's fine for every complexity. For me, having no way of avoiding trapping into an assert() is not the ideal code design.

I don't remember if it was Bjarne, Scott or Andrei, but I've been loving the quote since I found it: "Make it hard to use your code wrong." -- except I miss something like this: "..., but make it easy to use it right!".