Object initialization design issue

Started by
20 comments, last by Hodgman 10 years ago


I'd be hard pressed to say exactly why I find this easier to deal with and maintain if I'm honest but over the course of an entire system, it feels cleaner to me.
I don't know Aardvajk, I'm not quite on it. The main problem I have here is that you cannot take a Texture object and poll which device created it (maybe we can pull out the creation device from the OS handle, but in isolation, that's still not enough). This information will have to be tracked with surrounding code. It's not like I'm against this idea in itself but I always considered statements like "parameter X must be creator" a last resort. If I am allowed to specify a different object "for every method", then I'd intuitively feel entitled in calling load on a device and bind on multiple.

That's a very good point. Its hidden in this example by the fact that I only have the one GraphicsDevice, but thinking about this in a different context where multiple "creators" of some kind of "resource" can exist, it would make much more sense to bind the creator to the resource I suppose.

I suppose I just dislike the idea of pointers to objects floating about inside class implementations when I can just supply the object when it is needed, but your point is well made and fair.

Advertisement

Both of these:


//#1
class Texture {
  Device* device;
public:
  Texture(Device* device);
  void Bind(int slot);
}

//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}

And my personal choice:


//#3
class Texture {
  friend class Device;
  Texture();//n.b. private
}
class Device {
public:
  Texture* CreateTexture();
  void Bind(Texture*, int slot);
}

...all suffer from one of two variations of that same issue.

With #2 and #3, you can create a texture using one device, and then try and bind it to another, which won't work.


Device a, b;
//#2
Texture t(a);
t.Bind(b, 0);//error
//#3
Texture* t = a.CreateTexture();
b.Bind(t, 0);//error

With #1, you can have client code that looks correct though, but is ultimately flawed:


void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}

The client code for option #1 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #1 to catch these kinds of errors.

To solve this issue, I'd give the texture object a pointer back to the device in debug mode only, because it's not normally required, which you can then use to catch these kinds of mistakes at runtime.


#ifdef _DEBUG
# define myDEBUG(a) a
#else
# define myDEBUG(a) 
#endif
class Texture {
friend class Device;
  Texture( Device* d ) myDEBUG(:dbg_Owner(d)) {}
  myDEBUG( Device* dbg_Owner );
}
void Device::Bind( Texture* t, int slot )
{
  myASSERT( t->dbg_Owner == this ); // if created in a different device, let the programmer know their code is broken
}

//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}

With #2, you can have client code that looks correct though, but is ultimately flawed:


void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}

The client code for option #2 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #2 to catch these kinds of errors.

Unless I'm missing something (which I might very well be!), this wouldn't compile.

Given the function definition, it should be

diffuse.Bind(&device, 0); and normals.Bind(&device, 1);

Or...?

Hello to all my stalkers.

//#2
class Texture {
public:
  Texture(Device* device);
  void Bind(Device* device, int slot);
}
With #2, you can have client code that looks correct though, but is ultimately flawed:
void DrawThing( Texture& diffuse, Texture& normals )
{
  diffuse.Bind(0);
  normals.Bind(1);
  device.DrawQuad();//were those textures actually bound to this device, or to another one?? Who knows!
}
The client code for option #2 seems too much like "spooky action at a distance" to me, so I avoid it as well.
There's no way to automatically audit your code in option #2 to catch these kinds of errors.
Unless I'm missing something (which I might very well be!), this wouldn't compile.
Given the function definition, it should be
diffuse.Bind(&device, 0); and normals.Bind(&device, 1);
Or...?

It's just pseudo code. Doesn't need to compile to illustrate the point Hodge was making.

It's just pseudo code. Doesn't need to compile to illustrate the point Hodge was making.

True, but the comment indicates that you can't know that the device you're drawing with is the same device as the textures were bound to.

With the device explicitly used in the Bind(), you'd be able to see it's the same, no? That said, I'm not at all familiar with automatic audits, which Hodge refers to directly after.

---

EDIT: Although I guess the problem is apparent in more complex situations, where the bind doesn't necessarily occur in the same place as the draw. I can see the beast rear its head in that case.

Hello to all my stalkers.

I numbered the examples wrong. Replace "#2" with "#1" and vice versa... [edit] I've edited the post now to swap those numbers around [/edit]

#1's Bind function doesn't take the device as a parameter, instead getting it from a member. This means the user doesn't know which device it's being bound to (as indicated by the comment).

[edit] I accidentally down-voted Aardvajk below when scrolling with fat thumbs on a tiny phone... Someone care to countervote that for me? smile.png

To be fair though, I'd expect this imaginary Device to return some kind of error if we attempt to bind a resource to it that it didn't create, and we can catch this error and respond accordingly. This is only really necessary in the case that an untrappable error condition would occur as a result of binding to the wrong device, which is difficult to imagine in any real world system.

But an interesting thread and food for thought.

Wow thank you guys for all the answers smile.png

I went with Hodgman's solution (with CreateDevice) for my Renderer class, but I'll keep the other solutions in mind. Great stuff.

@Hodgman: I countervoted wink.png

I numbered the examples wrong. Replace "#2" with "#1" and vice versa... [edit] I've edited the post now to swap those numbers around [/edit]

#1's Bind function doesn't take the device as a parameter, instead getting it from a member. This means the user doesn't know which device it's being bound to (as indicated by the comment).

[edit] I accidentally down-voted Aardvajk below when scrolling with fat thumbs on a tiny phone... Someone care to countervote that for me? smile.png

Heh, I wondered why I got a message about a 0 vote on that. No probs, can spare a few points among friends smile.png


@Hodgman: I countervoted

Why thank you, sir.

I support the idea of ASSUMING that data from device A wont be used with device B at runtime, and doing checks but only in debug mode.

This is quaranteed to give the optimal runtime performance since it is basically the same as compile time checks, except we have to do it manually because the compiler is dumb. For the same reason I disagree containing a pointer to the device within the object UNLESS actually required, in which case the pointer still shouldnt go INSIDE the object but rather be "bundled" with the object in places where this data is needed. eg. Texture and std::pair<Texture,Device*> respectively.

So, Texture should not contain a Device*, but you may still bundle a Device* with the texture in places where this is required.

o3o

This topic is closed to new replies.

Advertisement