sprite_assign with free_texture=true corrupts in some cases.

[sorlok]

In certain cases, sprite_assign will corrupt something graphics-related if the value for parameter free_texture is true (the default).

I noticed this issue in Iji, and have trimmed down a test case to the following:
https://drive.google.com/file/d/0B1P7NepPcOsld3M1RlBYVk55ZUk/edit?usp=sharing

There is one (large) map, which contains only one object. That object calls sprite_assign on crate(). If you comment out that line, the image shown will be a slightly different shade of blue. Given that sprite_assign occurs multiple times in Iji, the end result is far more wild:
http://imgur.com/Od9MX3G

I'm still narrowing this down; no idea what's causing a seemingly-harmless call to sprite_assign to wreak so much havoc. Some other test cases I wrote from scratch do not feature this problem.

[sorlok]

Upon further investigation, we have this, in spritestruct.h:

struct sprite
{
//...some stuff, then:
   vector<int> texturearray; //Each subimage has a texture
};

Later, when we delete the texture:

textureStructs.erase(textureStructs.begin() + tex);

That will certainly corrupt memory (esp. for multi-image sprites), since sprite_assign frees memory by looping through and clearing items by id. So it calls:

graphics_delete_texture(0);
graphics_delete_texture(1);
graphics_delete_texture(2);
//..etc.

..but texture 1 is now texture 0 after the call to delete.

Here's a fix:
https://github.com/sorlok/enigma-dev/compare/texturecache_fix

I need to port this fix to DirectX and other backends, and then I will issue a Pull Request.

[RobertBColton]

I see the issue but I am not understanding the need for the change to the texture management itself, it seems like it should be easily fixable inside the spritestruct. The sprite struct just needs to hold the array of texture id's returned by graphics_create_texture and then loop that container when the sprite_assign function is called or sprite_delete or w/e and then it gets refilled with texture id's via graphics_create_texture again.

[sorlok]

Won't this invalidate any texture IDs held by other sprites?

For example, if I have three different sprites, all with only one image:

    spr_0 has texID 0
    spr_1 has texID 1
    spr_2 has texID 2

...and the textureStructs vector has:

    [texX, texY, texZ]

Now, I re-assign something to spr_1. Let's ignore the "creation" phase and just focus on deleting. We get:

    spr_0 has texID 0   //still ok
    spr_2 has texID 2   //This is wrong.

...and the textureStructs vector has:

    [texX, texZ]

spr_2 is now pointing to an out-of-bounds texture.

So I think it actually affects any texture deletion call, not just multi-sub-image ones. Right?

[sorlok]

Here is an example game that demonstrates the glitch without using sub-images:
https://drive.google.com/file/d/0B1P7NepPcOslQVNBYjE1SlNKV0k/edit?usp=sharing

Press X to delete a sprite that isn't being used in the room. Press Y to call sprite_assign() on two sprites present in the room.

Either X or Y will glitch the graphics, and pressing X then Y (or Y then X) will make the glitch even more obvious.

My texturecache_fix branch fixes both cases. (I still need to get it tested on DirectX; will get back to you.)

[JoshDreamland]

This shouldn't be happening; the only reason ENIGMA keeps track of the textures at all is so they can be iterated. The integers themselves (eg, texture 0, texture 1, texture 2...) are all assigned by the graphics backend (eg, OpenGL). But since vector::erase is O(n), he should indeed not be using it to store any information about textures that he intends to erase.

[sorlok]

Well, it's possible I uncovered a different bug that my patch fixes, but I'd rather understand the issue before I push the patch.

Any idea what's causing my test cases to crash?

[JoshDreamland]

A crash would likely result from the fact that Robert deletes stuff from the vector, then expects everything else to match up with the GL IDs, which are still assigned in the sprites, etc. But what I'm noticing looking through his code is that he seems to store the GL texture index in his own texture structure as well. There will be bloodshed if he's calling textures[sprite->texId]->texId every time he needs to look up a texture.

[RobertBColton]

with the GL IDs, which are still assigned in the sprites, etc.
Well that's one problem, why are GL id's in universal system? Only the texture id's should be there.

he seems to store the GL texture index in his own texture structure as well.
That is just doing the same thing the D3D9 texture struct does, except in D3D it's an object.

struct TextureStruct {
        LPDIRECT3DTEXTURE9 gTexture;

[JoshDreamland]

Affirmative. He decided that since DirectX needs to store a map of LPD3LONG32TEXTUREPOINTERLONGWORDSTRUCT32* or whatever, OpenGL may as well store a vector of GLuints. Then he decided to use a vector for it since I use a vector for the resources we don't erase frequently. He does have the thing a vector of pointers, he says, so it should be as simple as replacing that erase call with a delete call. However, my recommendation would be to replace the vector with a map, as you suggested, using the GLuint as the key. Then delete gltex from that structure altogether. It should not be storing that.

edit: Well, looks like he beat me to the punch. Either way, now you know.

[RobertBColton]

OpenGL may as well store a vector of GLuints.
No where in GL is there such a container in use.

However, my recommendation would be to replace the vector with a map, as you suggested, using the GLuint as the key. Then delete gltex from that structure altogether. It should not be storing that.
I'm fine with that but I really don't see a difference either way other than your way is just inconsistent with the D3D equiv.

[JoshDreamland]

Your vector contains a structure of a single GLuint, and a boolean. That's one bit better than a vector of GLuints.

"My way" prevents a lookup except when you need extra information about the texture, which is essentially never. Most code should never use your map. A sprite can get all the information it needs in GL from that one integer.

[RobertBColton]

Your vector contains a structure of a single GLuint, and a boolean. That's one bit better than a vector of GLuints.
What file, what line?

"My way" prevents a lookup except when you need extra information about the texture, which is essentially never. Most code should never use your map. A sprite can get all the information it needs in GL from that one integer.
Now I see what you are saying, but all that does is let it bypass where you can intercept and check in debug mode of whether or not it still exists.

[JoshDreamland]

GLTextureStruct.h:26
textures.find(texId) != textures.end()

[RobertBColton]

That's not a vector of GLuint's that's a vector of texture struct pointers. The sprite class holds a vector of the id's of these texture objects, which is necessary because you can't debug whether a GL texture exists or not, and that you can not use GL texture ID in the universal system.The whole point of the texture object is to make it graphics system analogous really.

[JoshDreamland]

Do you read the things that I write, or do you just keep posting the same thing until it looks like I'm agreeing with you?

Your vector contains a structure of a single GLuint, and a boolean. That's one bit better than a vector of GLuints.

i.e, your structure contains no useful information other than a GLuint for the texture. You basically never need to know that boolean.

textures.find(texId) != textures.end()

This is how you tell if a texture is defined. Replace texId with the integer the sprite stores.

ENIGMA stored exclusively a GLuint for five years, and then you showed up. Now it stores a GLuint and a boolean. It should be storing texture width information. But let me be clear: there is no reason that the GLuint should appear in your structure. It should be only the key.

[TheExDeus]

Just my two cents is that we should use GL functions to get information about textures as most GL calls cause the pipeline to stall (as in glEnable, glDisable, glSet and glGet usually does it). So we need a struct storing all the texture information + GL texture id (which can be used as an index in the map if it is any better).

Not gonna do it though, so I am out.

[RobertBColton]

Actually the intention is that class can pack multiple textures together as well for texture paging, which could also be utilized by the font_pack function.

[JoshDreamland]

Sprites are already aware of their size in the texture page; this is due to the fact that ENIGMA ensures power-of-two texture sizes.

Your texture structure could include the GLuint, but the sprite's subimage field should, in that case, point DIRECTLY to an array of these texture structures. Similarly, the background struct would own an instance of your texture structure, as a non-pointer member. This means that instead of your texture map owning the texture structures—that is, instead of this map having the liberty to allocate and free them—, individual engine components that create textures would have to be responsible for freeing the structure themselves.

The idea is that a texture lookup would not have to go through a map. If you have your background, you have everything you need to draw it. Similar for sprites, except one dereference is required to obtain the texture, anyway: your method requires at least two (more when a map is used instead of a vector).

This is a small optimization which costs nothing provided there is no better way to manage DirectX structures using integers, which we have already established is the case. For DirectX, the texture map can certainly be a vector, as you are assigning the texture IDs. In GL, assigning an ID on top of the GL ID is asinine.

[RobertBColton]

You know I keep trying to read this but since I just got done watchin' me crocodile hunter movies, I can' help but attach an aussie accent to your entire post mate, therefore finding it too ridiculous to respond appropriately.

[JoshDreamland]

That's almost exactly what I pictured was happening.

[RobertBColton]

http://www.opengl.org/wiki/Common_Mistakes#The_Object_Oriented_Language_Problem

[JoshDreamland]

Was that meant to be a statement as to why we need a structure containing nothing but a GLint and boolean? The only way that could possibly be less relevant is if it were a how-to on using GL in functional languages.

[RobertBColton]

To come back to this again, sorlok, it looks like we are actually going to do what Josh is suggesting. Game Maker Studio early access changes all _get_texture() functions to return a pointer to the Direct3D texture or the GLunsigned of an OpenGL texture. So Josh's map suggestion will be the way to go, but I don't want this done until it enters a stable release of GM.
http://store.yoyogames.com/downloads/gm-studio-ea/release-notes-studio.html

[sorlok]

Sounds good. The delay is fine; at least we understand the problem and required solution now.

[sorlok]

Game Maker has had a new stable release. Do you know if the *_get_texture() changes you mentioned have made it into this release? (I know texture storage is a sensitive issue, but I'd like to at least know the state of what GM is doing.)

[RobertBColton]

Actually it has been in for a while now sorlok, at least according to documentation. I know it is in the Studio manual somewhere but I can not find it atm, but Google results seem to suggest they already made the change.
https://www.google.com/?gws_rd=ssl#q=gm+studio+texture+pointer

[fundies]

@sorlok do you have the example game still? I refactored sprite stuff recently and am curious if this is still an issue

[RobertBColton]

This one shouldn't exist anymore, it was caused by us actually erasing a texture from the texture array (which resizes and moves everything), instead of just nulling it like GM. We don't do that anymore, so it should be fixed now.

enigma-dev/ENIGMAsystem/SHELL/Graphics_Systems/General/GStextures.cpp

Line 138 in 9cf788d

enigma::textures[texid] = nullptr; // GM ids are forever!

[fundies]

so close it thanks

[RobertBColton]

Yeah, I am quite certain this was actually resolved, the erasing from the vector sorlok reported is gone now. I totally misunderstood him originally and this issue got derailed into us talking about how to store the metadata. We've already established AssetArray and how we should handle assigning/creating/deleting resources as well as storing metadata.

For example, our GLTexture inherits our generic Texture these days.

enigma-dev/ENIGMAsystem/SHELL/Graphics_Systems/OpenGL-Common/textures_impl.h

Line 31 in 9cf788d

struct GLTexture : Texture {

Texture deletion now involves deleting the peer, then assigning nullptr to where that texture was.

enigma-dev/ENIGMAsystem/SHELL/Graphics_Systems/General/GStextures.cpp

Line 136 in 9cf788d

void texture_delete(int texid) {

No textures are moved or reallocated during deletion.