New thruster graphics

[Luomu]

Replaces the thrusters with new ones that are not quite so polygonal:

Feel free to suggest alternative colours, like:

[Brianetta]

Perhaps thruster colours can be determined in the Lua model?

[Luomu]

We could come up with a scheme where colours are determined from maximum thrust so you have a bit of a visual hint about other crafts' capabilities... or give factions colour-coded engines :-)

[Brianetta]

The factions idea has already been done in the X series.

[robn]

I'm not sure how I feel about this. So far Texture and its subclasses don't change once instantiated. That appeals to the purist in me, but it might be silly to hang on to it. We could tweak and subclass ModelTexture to get the same results, though it would mean a little bit of mucking about with the TextureCache. I'm much less picky about the cache interface though (I hate it).

Am I just being paranoid/sentimental/stupid? What do you think?

[Luomu]

I don't think subclassing ("BillboardTexture") to just set a wrap mode is good, so it's either this or constructor parameters (and GetModelTexture parameters)
In the future you might want to set the wrap mode on some few specific UI textures as well: repeating vs clamped background images, or let's say a gradient background that you want to clamp in one direction and stretch in other (at that point you need to add a new WrapMode).

[robn]

I think BillboardTexture is a lovely idea for a class as its name makes it very clear what it is - a flat thing that will be drawn in world space. I also like making lots of classes because they're cheap - the compiler sorts them all out. The point is that you have a bunch of Texture objects and you don't care what's inside - you just know that when you draw them you'll get the best thing for whatever they are.

I think constructor args would be acceptable (its really just exposing a constructor arg that's already there), but as you say, there's then a problem with the cache calls (like GetModelTexture). The cache isn't sitting well with me at the moment anyway. I think its because it cares about filename only as the identifier. Maybe this isn't a suitable use of the cache? Or maybe the cache would be better used as void AddToCache(const std::string &name, Texture *t). That is, you add the texture yourself and the cache doesn't care about the type or constructor args. It does make the "get or create" path a little more complicated (oh why can't C++ have convenient closures?).

I'm not sure about "in the future you might want to..." because too many complicated things get done in the name of future flexibility that never gets used (and I count myself as a major culprit here, something I'm trying to undo in places like Lua). Will we end up with a SetFilterMode()? I think changing format is probably a stretch.

I guess my broader concern here is that this code has just been cleaned, and if we're going to add stuff to it I want to make sure we're doing it the right way. That's not me saying I think my way is the right way. I just don't want us to have to clean this up again in the future.

[johnbartholomew]

I don't feel strongly about this, but here are my thoughts anyway:

I agree that subclassing is ok. There is one circumstance I can see where it would cause a problem: if you want a container of texture objects (as opposed to a container of pointers to texture objects), subclassing will bite you. I think that would work against the current design anyway though.

I agree that textures should be built with one set of properties and shouldn't change after that. SetWrapMode adds another thing to be wary of when using a texture ("Is the wrap mode correct? Do I have to reset it myself before use?"). Not a big deal, but a point against it.

I'd be happy with exposing the wrap mode as an optional constructor parameter, although the interface for the cache makes that more annoying than it should be. Probably the texture cache system will have to be redesigned again at some point.

I'm not totally convinced by BillboardTexture as a name, since the functional properties of the object are to do with the texture wrap mode (and filtering mode and so on), which is not really anything to do with being a billboard -- it just happens that billboards want textures with wrap-mode set to clamp. However, the existing Texture subclasses are named in a similar way (by use rather than by properties), and I can't think of a better name that fits that convention.

There's another question about BillboardTexture which is: should it know how to draw a billboard? Texture already has DrawQuad and DrawUIQuad -- should there be a DrawBillboard somewhere? I don't like this, because that kind of method kills batching and therefore kills render performance.

In a way I'd prefer the wrap mode (and similar things) to be data bundled with the texture file/resource itself, but that's a lot of work, probably requiring tool support and understanding from artists, so I don't think it's worth going there (maybe one day).

Overall I think adding BillboardTexture is the simplest solution for now, but perhaps could be scrapped again after a cache system redesign.

[Luomu]

There's another question about BillboardTexture which is: should it know how to draw a billboard? Texture already has DrawQuad and DrawUIQuad -- should there be a DrawBillboard somewhere? I don't like this, because that kind of method kills batching and therefore kills render performance.

No. Assuming #913 goes somewhere Texture::DrawQuad as it is will be gone too.

[johnbartholomew]

No. Assuming #913 goes somewhere Texture::DrawQuad as it is will be gone too.

Ah, cool. I hadn't looked at any of the renderer stuff yet.

[Luomu]

@Ziusudra has a tweak for the fade values here: http://pastebin.com/jErPYZtK

Feel free to use those values if you find it better, I can't decide. The colour could maybe still be more blue... :)

[Ziusudra]

Same thing updated for the latest changes: http://pastebin.com/LkPrxhpj

It's not a big thing, I just think it looks strange if they start fading at 90 degrees.