Some IMGs may be indexing palettes incorrectly

[afritz1]

In trying to replace my PNGs for the new game cinematic, I found that the intro images INTRO01.IMG, INTRO02.IMG, etc. aren't using any of the color palettes correctly. Note the 0 in the filenames; a similarly named set of INTRO images are junk.

The fact that INTRO01.IMG, et al. can be loaded by TextureManager::loadIMG() says that their header flags are recognized, but perhaps there is something else missing. Maybe the red and blue bytes are switched around for certain IMG files? Or perhaps there should be another level of indirection with the palette indices? I don't know, but it appears that some IMG files are an exception.

Looking at tool-extracted PNGs of the palettes, it looks like the most correct candidate is PAL.COL, but in that case, it should be using the second-to-last row of palette pixels, not the first row. I could be wrong, though. @kcat?

[kcat]

I think I saw somewhere that the INTRO*.IMG files are displayed using the palette in SCROLL03.IMG. I haven't verified that, though. In that case, you'll need a <std::string,Surface> map for each palette, and set the palette in SCROLL03.IMG for the intro sequence, so getSurface will get the intro images with that palette.

[afritz1]

I don't know if you've seen my changes to the texture manager yet, but the TextureManager::getSurface() and TextureManager::getTexture() methods now optionally take a PaletteName parameter for choosing which palette to load with. I could add another enum element called PaletteName::Scroll03, and that could point to a new palette in the std::map along with the others in the texture manager.

Maybe there could be an "initPaletteFromIMG()" method for extracting the palette from SCROLL03.IMG. This is all assuming that what you heard is correct.

[kcat]

I'd probably add a setPalette method to TextureManager, since it would reflect how the original behaved (only one palette can be active for a given frame). It makes sense to me for TextureManager to manage the current palette, instead of everything that can call getSurface knowing which palette should be used.

[afritz1]

I suppose a TextureManager::setPalette(PaletteName) method would be okay. That's pretty much what I was using a few days ago but I could change it back. I was just having some slight issues with it because when getSurface() was being called with one particular palette active, it wouldn't change the surface when setPalette() was called because getSurface() just returned the existing surface. So then should I use a std::map<std::string, Surface> for each palette like you were saying?

[kcat]

I was thinking something like a map<std::string,SurfaceGroup>, where a SurfaceGroup has a Palette and a map<std::string,Surface>. Then, setPalette would lookup the SurfaceGroup (creating a new entry and filling in its palette as needed) and set it as current. Future getSurface calls would look in the current SurfaceGroup, and creating new surfaces would use the current SurfaceGroup's palette.

[afritz1]

Surface groups are a good idea. I like how pairing the image filename and palette name via std::pair<std::string, PaletteName> for each surface has already been working, so I think I'll keep using that instead for now. I think it's generally the same idea as your suggestion, only without the separation into "groups". A surface is loaded if it hasn't already been paired with the active palette.

It's my understanding that the original game used one palette per screen, but this also causes a problem with the mouse cursor colors. Sometimes the mouse cursor looks correct, sometimes it looks white-ish, and sometimes it's completely black. setPalette() isn't expensive to use multiple times a frame, so I could just do that first for the screen and second for the cursor.

Edit: See this commit for details on using setPalette() again.

[kcat]

I like how pairing the image filename and palette name via std::pair<std::string, PaletteName> for each surface has already been working, so I think I'll keep using that instead for now. I think it's generally the same idea as your suggestion, only without the separation into "groups". A surface is loaded if it hasn't already been paired with the active palette.

In that case, I'd recommend using an unordered_map to store surfaces and SDL textures. A regular map will end up doing a lot of string compares since it needs to compare the input string against the key strings until it finds a match. An unordered_map instead sorts entries using an integer hash of the key (the unordered basically means that the ordering is unknown, since the hashing algorithm is implementation-defined), so it only needs to calculate an integer hash from the input <string,PaletteName> pair, then do a binary search lookup of that integer hash to find the surface or texture.

It's my understanding that the original game used one palette per screen, but this also causes a problem with the mouse cursor colors. Sometimes the mouse cursor looks correct, sometimes it looks white-ish, and sometimes it's completely black.

Effectively, yeah. An 8-bit video mode can only have one palette active at a time, so everything displayed would reference the same (current) palette.

setPalette() isn't expensive to use multiple times a frame, so I could just do that first for the screen and second for the cursor.

Alternatively, you could have two functions, a getSurface which takes the image filename (and which uses the current palette), and a getSurface which takes an image filename and a palette name (when you want to use a specific palette, e.g. for the mouse cursor).

Something else to be careful of is that an IMG may have its own palette, which it may or may not use (a bunch of the fullscreen maps, for example). You'd either need to add palette names for them, or you could have a special palette name called BuiltIn which can be passed to the second form of getSurface, to make an IMG use its own palette.

[afritz1]

Alternatively, you could have two functions, a getSurface which takes the image filename (and which uses the current palette), and a getSurface which takes an image filename and a palette name (when you want to use a specific palette, e.g. for the mouse cursor).

Hmm. I think I want to either use setPalette() exclusively, or make every getSurface() also take a PaletteName, not both. Having both options would make the palette behavior a tad more complex than it needs to be. I'm happy with just using setPalette() as it is now.

I could use an unordered_map for the surfaces and textures. I had been avoiding it because the vanilla std::map has been just fine for my needs, and I like having it be consistent across the project. That's just nitpicking, though, and string compares aren't cheap. I'll use the unordered_map.

Something else to be careful of is that an IMG may have its own palette, which it may or may not use (a bunch of the fullscreen maps, for example).

I don't think this is an issue because of the code we wrote here in TextureManager::loadIMG(). It uses the built-in palette if there is one, and ignores any parameter palette name.

[kcat]

I don't think this is an issue because of the code we wrote here in TextureManager::loadIMG(). It uses the built-in palette if there is one, and ignores any parameter palette name.

Right, but that's what I mean. Just because an IMG has a built-in palette doesn't mean it's necessarily what should be used to display it.

[afritz1]

Oh, I see. It's an odd idea that an IMG would not use its own palette. Hmm. If an IMG's palette isn't used for itself, then what's the point of it?

Well I'll consider adding PaletteName::BuiltIn. It'll be easy for the paletteRef in loadIMG() to check the palette name to decide which path to take.

So to solve the problem of built-in palette usage, I'll add back the optional palette name argument to getSurface(). Wait, wouldn't it always be PaletteName::BuiltIn? We would otherwise just call setPalette() right before. Maybe it should be a boolean instead; something like useBuiltInPalette.

[kcat]

If an IMG's palette isn't used for itself, then what's the point of it?

It could be used in some instances but not others. Or it could be a vestigial remnant from development. Like mentioned, 8-bit video modes can only have one active palette at a time, so even though an IMG may have a palette, that doesn't mean it would always be used with it.

I'll add back the optional palette name argument to getSurface(). Wait, wouldn't it always be PaletteName::BuiltIn?

In my mind, the secondary form of getSurface (with the palette) is mostly for images that are designed to look more or less the same with different palettes. That is, they use a specific subset of palette entries that are the same color in most other palettes the game uses. Like the aforementioned cursor, which doesn't have a built-in palette. You could specify the default palette for it so you don't load the same image multiple times because of multiple palettes when it's supposed to look the same anyway (and calling setPalette just before getSurface would be kinda ugly since you'd need to set it right back to what it was; better to just specify the palette with the call without messing about with the TextureManager state).

Further, I'd say that specifying BuiltIn with an IMG that doesn't have a built-in palette would be an error, since otherwise there's no indication of what palette it should use.

[afritz1]

@kcat, I took another glance through this issue and decided I'd try another way of storing the surfaces and textures. So instead of using a std::pair<std::string, PaletteName> key, it's now a std::string key into a map that uses a PaletteName key to get the Surface or SDL_Texture*.

Here are the new containers and the commit they were in. I also added new containers designed for animations and movies (the surfaceSets and textureSets), and I was able to get rid of that annoying hash function, too, which is nice. Let me know your opinion on the containers (if it's what you had in mind, etc.).

I don't know how you approach this in your Daggerfall project. On a side note, you use OpenSceneGraph? I've seen it mentioned in OpenMW sometimes, but I don't know what it's all about or when it's a good idea to program with it.

[kcat]

You generally want to avoid indirect lookups, especially for things you're going to be doing a lot per frame. Combining two objects into one hash for one hash lookup is better than two separate lookups. std::map in particular is also pretty bad at cache coherency because of the way it's designed, meaning it'll be triggering a lot of cache misses, stalling the CPU and making a mess of the cache, and just being a general drag on performance. std::unordered_map isn't that great either, but it's probably the best option STL has for an associative lookup, short of rolling your own.

[kcat]

I don't exactly remember the details about how I handle it in my Daggerfall project. Though I think the general idea was that I reference as much as I can with a unique integer ID, then use a sparse array to store them. The sparse array is effectively implemented as a <uint,object> associative map, although I use two vectors for better memory utilization (the uint "keys" in one vector, and the objects in another, so all the lookup values are near each other and all objects are near each other).

As for OpenSceneGraph, it's basically a library that handles rendering a scene (with OpenGL) using a node graph, where nodes can specify rendering properties in a hierarchal relationship, what geometry to draw, etc. It's also persistent state as you can define a graph to render, then only change the nodes that are different on subsequent renders. It's a bit higher level than OpenGL itself since it'll handle things like state management and rendering order (that is, try to render objects in an order that minimizes costly state changes, while also culling what can't be seen), as well as loading and caching OpenGL objects, though it's not a full-on engine like Unity3D.

[afritz1]

Ah, okay. I wasn't even thinking about cache coherency. So maybe I want the texture manager to return a unique ID for each new surface or texture it loads instead. That way I could use a vector instead of a map. But, I mean... I'm using C++ instead of C so I can use maps and not just arrays! The only thing I'm really worried about performance-wise is the ray tracing.

Anyway, yeah, I was just curious about OpenSceneGraph in general. I figured it involved a hierarchical relationship of geometry a lot like a bounding volume hierarchy. Right now, I'm still kind of a novice with OpenGL, though. I don't think I'd be able to help out very much on OpenMW graphics code with my current knowledge.