GameDev General

Pages: 1... 3 456 7

You mean you don't try to use the object even after it possibly fails to allocate memory for it?

I don't think you caught it all at first glance. mesh allocated by new (assuming an exception isn't thrown) is immediately leaked. The mesh after the catch block has to be a different mesh.

BHX (3450)

I was thinking of it as if he was trying to create the first mesh, but has the exception thrown which means he has nothing allocated and trying to access the object.

chrisname (7395)

@Lumpkin
Why don't you put all the different kinds of resources into a union and have a member that determines which one to return? It costs no extra memory - all union members start at the same offset, so the memory usage is whatever that of the largest member is (e.g. if you have a union of std::uint8_t, std::uint16_t and std::uint32_t, the memory usage is 4 bytes - the size of the larger member (std::uint32_t)). The main caveat of unions is that you can't write to one member and then read a different one, that's undefined behaviour. You can only read the one that was the most recently written to, and writing to a different one discards any previous writing. In practice, it'll probably work as expected on most architectures (if you take endianness into account), but to guarantee maximum portability you should stick to one union member all the time.

Here's an example of what I'm talking about:

enum resource_type {
    music,
    shader,
    sound,
    texture
};

struct resource {
    resource_type type;
    union {
        sf::Music music;
        sf::Shader shader;
        sf::SoundBuffer sound;
        sf::Texture texture;
    };
};

(note: if you don't name the union or create any objects for it, then it exports its members into the outer class' namespace, so you can access them as if they were direct members of struct resource.)

This way, you don't have to use complex overloads or templates, and you can store all your resources in one array so there's no extra runtime or memory overhead.

Last edited on

closed account (o3hC5Di1)

Slightly off topic, but wouldn't it be easier to have a separate "game developing forum" rather than one big thread? I know there are several forums specific to game developing out there, but seeing the amount of questions about it on here (and because game development is a big user of c++) perhaps adding another forum would make sense.

Just a suggestion.

All the best,
NwN

closed account (N36fSL3A)

cire wrote:
I don't think you caught it all at first glance. mesh allocated by new (assuming an exception isn't thrown) is immediately leaked. The mesh after the catch block has to be a different mesh.

You're right, that was a bonehead mistake on my part. :P

JLBorges wrote:
What's all this? Why aren't you using a weak pointer if unreferenced resources are to be automatically unloaded? That is canonical, and will avoid the race condition that exists in your code. Something like what is outlined below, perhaps? (Assuming that the resource manager is not going to be concurrently accessed from multiple threads).

I was headed towards that approach, but then what if I needed that resource 1 cycle after it was unloaded? I'd rather sacrifice RAM over CPU time.

chrisname wrote:
@Lumpkin Why don't you put all the different kinds of resources into a union and have a member that determines which one to return? It costs no extra memory - all union members start at the same offset, so the memory usage is whatever that of the largest member is (e.g. if you have a union of std::uint8_t, std::uint16_t and std::uint32_t, the memory usage is 4 bytes - the size of the larger member (std::uint32_t)). The main caveat of unions is that you can't write to one member and then read a different one, that's undefined behaviour. You can only read the one that was the most recently written to, and writing to a different one discards any previous writing. In practice, it'll probably work as expected on most architectures (if you take endianness into account), but to guarantee maximum portability you should stick to one union member all the time. Here's an example of what I'm talking about:

chrisname wrote:

That's a nice idea actually. I'll consider using it.

Last edited on

JLBorges (13770)

> I was headed towards that approach, but then what if I needed that resource 1 cycle after it was unloaded?

See: http://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

If multiple threads of execution access the object referenced by the same std::shared_ptr without synchronization, a data race may occur, unless all such access is performed through these functions

> Why don't you put all the different kinds of resources into a union ...

It is such a dumb idea that you do not want to even think about it.

In any case, I'm getting out of this. Should have had better sense than to attempt to engage veterans of the lounge in any kind of technical discussion.

Disch (13742)

<Late to the convo>

Unions are rubbish.

I don't see the need to treat multiple resources as the same type here. Doing so will only complicate your code (ie, making a union... having to switch on a "type" identifier to figure out which type of resource you actually have... or worse... downcasting... ugh).

Don't treat all things the same unless you can get away with treating them all the same. Since a generic "Resource" is of little/no use... you should not bend over backwards to create that abstraction. Your resource manager is already a bit overcomplicated.

Rather than having one resource manager to manage all resources and treat them all the same... it's much easier to write a templated resource manager class which handles one type of resource.. and then just have multiple managers.

Also... I'm a little confused by this:

					case FILE_OBJ:
						try {Mesh* mesh = new Mesh;}

						catch(std::bad_alloc& ba)
						{
							std::cerr << "Failure to allocate memory: " << ba.what() << '\n';
						}

						// Load the file into memory
						mesh->LoadOBJ(location.c_str());
					break;

How does this even compile? 'mesh' is local to the 'try' block... so trying to access it outside of that should give you an error.

Furthermore... catching the exception that mesh failed to allocate... then using mesh anyway will at best cause your program to crash... or at worst will cause very strange/bizarre things to happen.

Proper error handling is a little tricky. If your resource loading fails you have a few options.

option 1) Forcibly exit the program. Easy, but kind of crappy.

option 2) Report the failure back to the calling code by returning an "error" code. IE, return "false" if the resource failed... or give a nullptr instead of a pointer to the resource. The disadvantage to this is now to correctly handle the error... all the code which calls this function must now watch the return value and make sure the function succeeded, which leads to a lot of duplicate code and a lot of error checking. I am not a fan of error code returns.

option 3) Throw an exception. Or in this case... just let the bad_alloc exception fall through (no need to catch it here). Code that can recover from a failed resource can put the loading in a try block and can handle the failure their own way. But for code which can't handle the failed load (which I've found to be most of the code in a game) -- the exception will whip past them and go to error handling code higher up.

Lastly... it doesn't look like your resource manager bothers to pool the resources... which kind of defeats the whole point of a resource manager in the first place. IE: if two different objects try to load "foo.png" as a texture... the texture will be loaded twice.

If you're not going to pool the resource and share the same resources between multiple objects that need them... then why bother with a resource manager in the first place? Why not just have those objects own their own resources?

closed account (N36fSL3A)

See: http://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

If multiple threads of execution access the object referenced by the same std::shared_ptr without 
synchronization,a data race may occur, unless all such access is performed through these functions

I don't understand how I'll get a data race. My program is only going to use 2 threads, one for the program's loop, one for the resource manager loading (I might add a thread pool). Semaphores definitely will be used to prevent that.

Last edited on

chrisname (7395)

JLBorges wrote:
In any case, I'm getting out of this. Should have had better sense than to attempt to engage veterans of the lounge in any kind of technical discussion.

If that's your attitude towards helping people, then good riddance. No-one was disrespectful of you, so you can't claim that your knowledge is not being appreciated or some such. I for one consider you one of the most knowledgeable members of the forum, and usually one of the most helpful, but I'm disappointed and slightly insulted by that remark.

Disch wrote:
Unions are rubbish.

Says the person who recommended me to use a union for my event structure. Why is it different for resources?

having to switch on a "type" identifier to figure out which type of resource you actually have

My implementation only does that in the constructor and that's just to store it. After that, the onus is on the code that's about to use the resource to know what it's about to use it for. I can't imagine it's a big performance hit and when there's only a few different kinds of resource*, it's not too ugly either.

* I have six and that will go down to five when I get around to combining fragment and vertex shaders into one resource type (they are already one object, though).

Rather than having one resource manager to manage all resources and treat them all the same... it's much easier to write a templated resource manager class which handles one type of resource.. and then just have multiple managers.

I considered that, but I decided it was more complex, having to manage multiple arrays of different resources, than simply storing them all in once place. My resource manager is very simple because the complexity has been pushed on to the resource structure, which itself is also quite simple:
* Resource: http://codepad.org/MAJ9HMzQ
* Resource manager: http://codepad.org/c8tHzH0d
* Resource loader: http://codepad.org/vQgGK6UH
It also wouldn't easily fit with my resource loading model, where resources are loaded by a separate class running its own thread. That way, resource loading doesn't block the game. Also, having five different resource managers would not only make the loader more complex and coupled to more classes, but then I'd also need more resource loaders and thus more threads.

Last edited on

Disch (13742)

chrisname wrote:
Says the person who recommended me to use a union for my event structure. Why is it different for resources?

Using unions for this purpose is "downcasting without actually downcasting." IE, you have a general "Resource" abstract type and need to 'downcast' to a more specific type.

I don't like downcasting. It's clunky, easy to make mistakes with, and is not flexible.

You're right, events have the same problem, though in their case it's often forgivable since all events usually have to be treated in a uniform way for the sake of processing ease. For example... there can be a single "pollevent" style function which you can access any and all event types from. If you had to poll each type of event separately, it would be clunky... you'd lose the exact order the events occurred in... and you'd have to handle all events (even ones you're not interested in) or else they get backed up in the queue, etc, etc.

In short... the way I see it is that events are treated as a single type to outside code... whereas with resource...no outside code would ever have any use for a generic 'Resource'. Outside code always knows what kind of resource it has... and what kind of resource it wants. There's no need for abstraction there. So why should the manager abstract something that isn't abstracted anywhere else?

My implementation only does that in the constructor and that's just to store it. After that, the onus is on the code that's about to use the resource to know what it's about to use it for.

That can't be the only place, can it? If you're handing generic resources to outside code... then the outside code must also be doing some sort of downcasting to actually use the resource, right?

Besides that... switch-on-types also imposes and expandability problem. If you need to add a new resource type you have to do several steps:

1) Create a new loader
2) Add another enum/identifier to mark the resource type ID
3) Add another entry in your union
4) Update all switch-on-types to include the new type (hopefully you only have one -- but that seems unlikely -- and is impossible to guarantee with outside code)

With multiple (templated) managers, expanding is much simpler:
1) Create a new loader
2) typedef a new manager (1 line of code)

I considered that, but I decided it was more complex, having to manage multiple arrays of different resources, than simply storing them all in once place.

OOP, silly goose. You don't have to manage multiple different arrays.. you just have to manage 1 inside a class. Then instantiate multiple classes.

Also, having five different resource managers would not only make the loader more complex and coupled to more classes

I don't see how. All the loader does is load a resource, right? What does that have to do with how many managers there are? The loader shouldn't even need to be aware of any -- or at worst.. it would only need to be aware of the one it's working with.

but then I'd also need more resource loaders and thus more threads.

Hrm..... we might have different ideas of what a loader is. I'm just thinking it something where you give it a filename and it gives you the loaded resource. You're talking about coupling with other classes and making threads and stuff... seems a little wonky to me.

As for more threads... that'd be entirely up to you. If you want each manager to use it's own thread for loading resources.. or if you want all of them to share a common secondary thread. Both are easy to do.

Last edited on

closed account (3qX21hU5)

To demonstrate Disch's point (At least I believe this is what he means) on multiple resource holders here is one that I have used to manage my resources.

#include <map>
#include <memory>
#include <string>
#include <cassert>
#include <stdexcept>


template <typename Resource, typename Identifier>
class ResourceHolder
{
    public:
        void                load(Identifier id, const std::string& filename);

        template <typename Parameter>
        void                load (Identifier id, const std::string& filename, const Parameter& secondParam);

        Resource&           get(Identifier id);
        const Resource&     get(Identifier id) const;

        void                insertResource(Identifier id, std::unique_ptr<Resource> resource);

    private:
        std::map<Identifier, std::unique_ptr<Resource>> resourceMap;
};

#include "ResourceHolder.inl"

#include <iostream>


template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::load(Identifier id, const std::string& filename)
{
    std::unique_ptr<Resource> resource(new Resource());
    if (!resource->loadFromFile(filename))
        throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);

    auto inserted = resourceMap.insert(std::make_pair(id, std::move(resource)));
    assert(inserted.second);
}

template <typename Resource, typename Identifier>
template <typename Parameter>
void ResourceHolder<Resource, Identifier>::load(Identifier id, const std::string& filename,
                                                const Parameter& secondParam)
{
    std::unique_ptr<Resource> resource(new Resource());
    if (!resource->loadFromFile(filename, secondParam))
        throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);

    auto inserted = resourceMap.insert(std::make_pair(id, std::move(resource)));
    assert(inserted.second);
}

template <typename Resource, typename Identifier>
Resource& ResourceHolder<Resource, Identifier>::get(Identifier id)
{
    auto foundResource = resourceMap.find(id);
    assert(foundResource != resourceMap.end());

    return *foundResource->second;
}

template <typename Resource, typename Identifier>
const Resource& ResourceHolder<Resource, Identifier>::get(Identifier id) const
{
    auto foundResource = resourceMap.find(id);
        assert(foundResource != resourceMap.end());
                
    return *foundResource->second;
}

template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::insertResource(Identifier id, std::unique_ptr<Resource> resource)
{
    auto inserted = resourceMap.insert(std::make_pair(id, std::move(resource)));
    assert(inserted.second);
}

I choose to use enum's to manage the ID's but you can choose to do it however you wish to.

namespace Textures
{
    enum ID
    {
        Entities,
                Sprites,
                Jungle,
                TitleScreen,
                MenuScreen,
                SpaceBackground,
                Buttons,
                Explosion,
                Particle,
                FinishLine,
    };
}

namespace Shaders
{
        enum ID
        {
                BrightnessPass,
                DownSamplePass,
                GaussianBlurPass,
                AddPass,
        };
}

namespace Fonts
{
        enum ID
        {
                Main,
                Label,
        };
}


template <typename Resource, typename Identifier>
class ResourceHolder;

typedef ResourceHolder<sf::Texture, Textures::ID>            TextureHolder;
typedef ResourceHolder<sf::Font, Fonts::ID>                  FontHolder;
typedef ResourceHolder<sf::Shader, Shaders::ID>              ShaderHolder;

In my opinion a resource manager should have only a simple job of loading a resource and providing a way to access that resource.

Last edited on

chrisname (7395)

Disch wrote:
That can't be the only place, can it? If you're handing generic resources to outside code... then the outside code must also be doing some sort of downcasting to actually use the resource, right?

It really is; here's an example of using it:

void bgobject::draw(sf::RenderTarget& target, sf::RenderStates states) const
{
    auto weak_res = resource_manager::get(m_texture_id);
    auto res = weak_res.lock();
    sf::Sprite sprite(res->texture);
    sprite.setPosition(position());
    target.draw(sprite, states);
}

The draw function knows it wants a texture, so all it does it retrieve the resource struct and then use the texture. It could use the sf::Music member instead, which would be bad, but while I generally don't use the "honour system" in programming, I made an exception there for simplicity's sake. While code could be written that uses sf::Music when it's supposed to use sf::Texture, the person writing that code would have to be doing it intentionally because they had to request the resource in the first place. The resource IDs even begin with the first letter of the kind of resource they represent, e.g. t for texture, s for shader, m for music or sb for sound buffer (since the type is sf::SoundBuffer). You really would have to do it on purpose, and if someone is that determined to break the code they're writing, they have much easier methods at their disposal, like std::terminate.

If you need to add a new resource type you have to do several steps:

1) Create a new loader
2) Add another enum/identifier to mark the resource type ID
3) Add another entry in your union
4) Update all switch-on-types to include the new type (hopefully you only have one -- but that seems unlikely -- and is impossible to guarantee with outside code)

1. I don't know where "create a new loader" came from: the loader and manager are not aware that there are different resource types (only the resource class itself is), so I only need one loader with my current architecture.
2. I'd have to create another identifier for the resource type anyway because the resource type is how the engine constructs the path to the resource (it's essentially "assets/" + resource_type + "/" + resource_id + "." + extension).*
3. That's true.
4. Also true, but I really do only have one switch (see above). The default case of the switch throws an exception so that I don't forget to add the new case. See, I thought ahead :) Of course, that exception is only thrown if the new resource type is actually used, but that's the only situation where it would cause a bug anyway.

* that information does currently have to be updated by hand, which is definitely a problem:

std::map<resource_type, std::string> resource::paths = {
    { resource_type::font,        "assets/fonts"    },
    { resource_type::frag_shader, "assets/shaders"  },
    { resource_type::music,       "assets/music"    },
    { resource_type::sound,       "assets/sounds"   },
    { resource_type::texture,     "assets/textures" },
    { resource_type::vert_shader, "assets/shaders"  }
};

std::map<resource_id, resource_type> resource::types = {
    { resource_id::tbgcity000, resource_type::texture }
};

std::map<resource_id, std::string> resource::filenames = {
    { resource_id::tbgcity000, "bgcity000.png" }
};

but that's only temporary, I plan to have it loaded automatically from configuration files just as soon as I can figure out how to convert strings to enum values without having more switch()es like you can in C#.

OOP, silly goose. You don't have to manage multiple different arrays.. you just have to manage 1 inside a class. Then instantiate multiple classes.

But

template <class TResource>
class resource_manager {
private:
    friend void Zarathustra();
    static std::map<resource_id, TResource> resources;
};

void Zarathustra()
{
    std::cout << std::boolalpha << &(resource_manager<sf::Texture>::resources) ==
                                   &(resource_manager<sf::Shader>::resources);
}

false

Thus Spake Zarathustra.

silly goose

If I were a goose, wouldn't I be migrating south for the winter and unable to use the Internet?

Well, in the main thread, objects call the resource manager's "load" method, which then calls the resource loader's request method which stores the ID of the requested resource. Then, the gameplay state pushes a "loading" state which calls the resource loader's "load" method. The load method tells the background thread to stop idling. The background thread moves the contents of the queue into a temporary (so that the process doesn't block while the resources are loading, only while the queue is being moved which I assume is very fast) and then starts loading the requested resources. When it's done, it informs the main thread (via an atomic variable that is accessed with an accessor function) and then goes back to sleep. When that happens, in the main thread, the loading state returns control to the gameplay state and gameplay continues. Although the non-blocking is kind of pointless at the moment because the game blocks anyway, I plan to have some pre-loading going on in the background in the future.

Anyway, you're right that with my current design, there wouldn't be any need for more loaders or more threads because the resource loader doesn't even know what kind of resources its loading. That is, again, pushed onto the resource thread. In fact, it doesn't even do the loading itself, directly - that happens in the resource loader's constructor. This way, neither the resource manager nor the resource loader are even aware that there are different types of resource. In fact, only the resource struct itself knows. Everything else is based around the belief "I put an x in, so I must get an x out".

Last edited on

closed account (o1vk4iN6)

chrisname wrote:

std::map<resource_id, resource_type> resource::types = {
    { resource_id::tbgcity000, resource_type::texture }
};

std::map<resource_id, std::string> resource::filenames = {
    { resource_id::tbgcity000, "bgcity000.png" }
};

That information shouldn't be in code, if you ever want to add a resource (ie texture or otherwise) you are going to have to recompile... which shouldn't be the case.

Last edited on

chrisname (7395)

@xerzi
I know:

I wrote:
that's only temporary, I plan to have it loaded automatically from configuration files just as soon as I can figure out how to convert strings to enum values without having more switch()es like you can in C#.

I might give in and just use switches but I'm really hoping I can find something else, even if it means using a custom enum class.

Disch (13742)

It really is; here's an example of using it:

Ah right. I see now. Still... that makes me uneasy because you're doing a theoretically unsafe union access without first confirming that the type is actually what you'd expect.

The draw function knows it wants a texture, so all it does it retrieve the resource struct and then use the texture

EXACTLY... this is my point. The draw function will always know this... so why can't it just get a texture directly from a texture manager... rather than a generic "resource" from a resource manager? Wouldn't that be simpler?

Treating all resources as generic obfuscates the API.

while I generally don't use the "honour system" in programming, I made an exception there for simplicity's sake

I guess this sums up our difference in opinion. I think we both agree that the "honour system" is not ideal... the difference is that you see it as simpler whereas I see it as more complicated and dangerous.

We have different ideas of a loader.

My idea of a loader: you give it an identifier, it loads the resource and gives it back. You would need a separate loader for textures and fonts because loading textures is (or can be) done differently from loading fonts.

It sounds like your idea of a loader is an extra layer on top of that.

2. I'd have to create another identifier for the resource type anyway

Well yeah... that's my point. It's more maintenance.
Additional maintenance, complication, and chance for the user to accidentally screw up. This is why I don't like this approach.

But [snip] Thus Spake Zarathustra.

I don't see what you're getting at with that code snippit. Of course the managers aren't going to have the same address... they're different objects. That's the whole point.

But we can go back and forth all day... why don't I just give you an idea of what I have in mind. Gimmie a minute

Disch (13742)


template <typename T>
class ResourceLoader    {};

// specialized loaders
template <>
class ResourceLoader<sf::Texture>
{
public:
    static std::shared_ptr<sf::Texture> load(const std::string& name)
    {
        std::shared_ptr<sf::Texture> res( new sf::Texture );
        res->loadFromFile( ("assets/textures/" + name).c_str() );
        return res;
    }
};


// Shared resource manager
template <typename T>
class SharedResourceManager
{
public:
    typename std::shared_ptr<T>     ResT;
    
    static ResT get(const std::string& name)
    {
        ResT& res( resources[name] );
        if(!res)
        {
            res = ResourceLoader<T>::load(name);
        }
        return res;
    }
    
private:
    static std::map<std::string, ResT>     resources;
};


typedef SharedResourceManager<sf::Texture>  TextureManager;


/////////////////////////////////////////////
/////////////////////////////////////////////

// Adding new managers:

// Step 1:  Add a loader
template <>
class ResourceLoader<sf::Font>
{
public:
    static std::shared_ptr<sf::Font> load(const std::string& name)
    {
        std::shared_ptr<sf::Font> res( new sf::Font );
        res->loadFromFile( ("assets/fonts/" + name).c_str() );
        return res;
    }
};

// Step 2:  Typedef the manager
typedef SharedResourceManager<sf::Font> FontManager;

This is the single-threaded version for simplicity. Though if you want to move loading to another thread, the concept is the same.

Now TextureManager::get("image.png"); actually gets you a Texture, rather than a generic resource.

closed account (N36fSL3A)

How does shooting work in FPSes? They get mouse coordinates and calculate where the bullet should be placed and a directional vector, however I don't know how they do it.

Disch (13742)

Any first-person 3D game is going to have a vector to indicate which direction the player is facing. Move movement would simply rotate that vector.

Shooting a "perfect" bullet would just be a matter of originating the bullet on the player, and giving it a velocity going in the direction the player is facing. It'd be as simple as this:

Vector3 bulletvelocity = bulletspeed * playerdirection;

Although... you usually wouldn't want a perfect bullet, you'd want to miss some shots (due to recoil or less than ideal aim), so you could muck up the angle a bit with some randomness.

closed account (o1vk4iN6)

You'd want to avoid using velocity on a bullet if you plan on having netcode. Just doing a ray trace and testing collision is going to be preferred otherwise you will end up with a game that requires a lot of leading for online play. There are other ways to reduce it but i've yet see any game accomplish this perfectly.

Last edited on

Disch (13742)

Bullet velocity seems pretty typical in games I've played. Games like Borderlands have guns which have faster/slower bullets (the speed difference is noticable).

Other games fire projectiles which are intentionally slow (like arrows).

Pages: 1... 3 456 7