Slick Forums

Currently the only ways of creating a blank image with a specific colour would be to:

• Create an image, then use FBO/render-to-texture to fill/clear it with the given colour (lots of code, inefficient, requires RTT capabilities)
• Create an ImageBuffer, then manually loop through and set each pixel to the given RGBA (lots of code, ugly, not very "slick-esque")

I suggest the following:


This would be accomplished by adding an optional color parameter to EmptyImageData and the corresponding Image constructor. Changes to the EmptyImageData class:


Then, the following Image constructor would be added:


Related problems that should also be addressed:

• In my opinion ImageBuffer.getImage() should be deprecated for the following reasons:
  
  • the naming does not make it clear that a new image and new texture is created
  • The ImageData constructor already exists for this purpose; IMO redundant code like this clutters Slick
  • Currently, the getImage() method utilizes the package-protected Image(ImageBuffer) constructor instead of the public Image(ImageData). Is this necessary? The only difference is a call to bindNone -- so why does one constructor unbind textures while the other doesn't?
    
    Code:
    ImageBuffer buf = new ImageBuffer(256, 256);
    Image img = new Image(buffer);
    System.out.println(TextureImpl.getLastBind()); //returns org.newdawn.slick.opengl.TextureImpl@XXXX
    Image img2 = buf.getImage();
    System.out.println(TextureImpl.getLastBind()); //returns null
• For the sake of consistency and code minimization, the constructor Image(int, int) should simply pass a new EmptyImageData to the Image(ImageData) constructor:
  
  Code:
  public Image(int width, int height) {
       this(new EmptyImageData(width, height));
  }
• Certain methods do not check init() very safely: draw, drawSheared, drawEmbedded, etc. The following code doesn't draw anything, for example, because init() is never called: (try adding a getWidth() in there and it will work)
  
  Code:
          img5.bind();
          GL11.glBegin(GL11.GL_QUADS);
          img5.drawEmbedded(0, 0, 100, 50, 0, 0, 100, 50);
          GL11.glEnd();

Discuss your thoughts.

P.S. if i'm not using this forum right let me know...

Some other issues:

• If you destroy() an image, and then give it a new texture, it will still think it's been "destroyed".
  
• Currently, Image.getGraphics creates a new texture; this is useful when you don't want to alter the original texture. However, in some cases the user will use Image.getGraphics for the sake of caching, say, a tile map. Their code might look like this:
  
  Code:
      public void recreate() throws SlickException {
          if (cachedImage!=null)
              cachedImage.destroy();
          cachedImage = new Image(1000, 1000);
          Graphics g = cachedImage.getGraphics();
          // ... render the tiles and such ... //
          g.flush();
      }
  
  
  The problem is, this causes a memory leak and eventually the program will crash. Two textures are created here, and only one is being destroyed (even though this isn't clear to the user). The solution would be to use the following non-intuitive code:
  
  
  Code:
      public void recreate() throws SlickException {
          if (cachedImage!=null)
              cachedImage.destroy();
          cachedImage = new Image(1000, 1000);
          //get the texture which we won't be using
          Texture old = cachedImage.getTexture();
          Graphics g = cachedImage.getGraphics();
          // ... render the tiles and such ... //
          g.flush();
          //release the unused texture
          old.release();
      }
  
  
  And that's not exactly a pretty solution, since we are creating two 1000x1000 textures every 'recreate()' instead of only one.
  
  Of course, the best solution would be to create a single Image and just g.clear() it; but users don't always think of this and it still doesn't take care of the unused texture that is just wasting memory.

I agree with everything you said, this topic is amazing, should be sticky!

so davedes, I'm not Image savvy, but let me see if I understand,

Currently, if you create a new image, a new texture is created for it. If you decide to change the image, another new texture will be created for that changed image. However the original texture will not be destroyed. It will be left alive because most textures are leaded from files and used often, and it would be very inefficient to load the image again whenever it was reused. The solution would be to have the resource loader keep track of which textures were loaded from files, and if an image is changed, and the old image was not loaded, and is not in use by other images, it is destroyed automatically.

is this right?

_________________
"Artificial intelligence will never be a match for human stupidity" - "Jamos Kennedynos"

Ideally, getGraphics should modify the original texture, since that's what the name and usage implies. The user should assume that the original texture is being modified. The problem is, as described in FBO/PBuffer code, because of a limitation we should only create FBOs and such immediately after the texture has just been created. So we can't truly modify the original texture, hence the reason kevglass created a copy of the original within the FBO/PBuffer classes. The problem is that getGraphics tries to emulate Java2D's createGraphics method, when really we should have implemented it a little differently.


In my opinion this is too much "under the hood" stuff and it will lead to problems down the road. Since images are often programatically generated, we should not treat it differently if loaded from a file. Plus, it doesn't address the real problem: two textures are being created instead of one.

I posted a hack/workaround to the two-texture problem here:
viewtopic.php?f=1&t=4511

The basic idea is that we don't create a new texture in the Image constructor. This leads to some problems (getTexture will be different after getGraphics is called, the constructor is bulky and non-intuitive, if we want render-to-texture on a file-loaded image we will still need to create two textures, etc).

A different solution would be to use a new class, MutableImage, which sets up its texture for use with FBO/PBuffer, and then Image.getGraphics would be deprecated/removed. The code might look like this:



I'll think about it a little more. Either way, these things will break a lot of existing code, so maybe they would be better suited for a "slick 2.0" if we ever come to that. If anyone has different ideas, don't hesitate to post 'em...

Maybe the easier change would be to add a class ImmutableImage? And Image works like it used to work before (meaning no changes).

This way we wouldn't break existing code and people who care about this issue could use ImmutableImages where possible?

And I'm with Mr. Kenkron: absolutely not image savvy

All stuff you already found out, davedes, is completely amazing to me
Thanks a lot for even finding and explaining those issues!

_________________
Right Angle Games | Marte Engine
Back to the past | Star Cleaner | SpiderTrap

Since backwards compatibility is our priority, here's an alternative:
- InternalTextureLoader doesn't defer ImageData textures (i.e. EmptyImageData textures), so we may as well generate them lazily. That is, only generate the texture internally when absolutely necessary (i.e. to draw it, or when getGraphics is called, or when the user forces it with a method like create()).

This doesn't introduce any new classes and still uses the same old getGraphics API as per usual. In some cases (e.g. loading an image from file) there will still be two textures created; I can't think of any way of getting around this without refactoring.

And we could change the FBO/PBuffer to destroy the old texture by default, because most users aren't savvy enough to manually destroy the old texture with release(). We could then add the method getGraphics(boolean maintainOld) or something similar for those that want to maintain the old texture.

Gotchas:
- graphics.copyArea() needs a generated texture to work; truth is, I think we can make a few changes to copyArea to improve it a little (so that users have better access to glTexSubImage2D)
- FBO/PBuffer etc. copies the original texture by drawing() it; if the original texture is empty (or not yet created) then the draw call should be ignored. Maybe we could add isEmpty() for TextureImpl to check if the texture has no pixel data to draw.

I'll keep thinking about it.

There may be a problem with destroying an image by default. The user may not actually want the image destroyed. If the user wants to keep the texture (say, because he has more images that use it) then how would he/she prevent the texture from being destroyed.
I backwards compatibility sounds good, but it seems that the best solution would be to force the user to create a new image if he wanted an edited image (Like tommy said I think).

_________________
"Artificial intelligence will never be a match for human stupidity" - "Jamos Kennedynos"

True, the image shouldn't be destroyed by default as the user might want to modify a sub image.


Not sure what you mean or how this solves the problem...

The problem (as far as I can make out) is that the user can create new textures without realizing they are creating new textures. Thus, they don't know to destroy old ones. One alternative is to make it so that an old image is automatically destroyed. Another is to change it so that no new image is created. However, a third option, which I suspect may be the best, is to change the image editing so that it is clear to the user that a new image is being created whenever an image is changed. The simplest way to implement this that comes to mind is to not allow a user to edit an existing image, and force them to explicitly make a new image based off of an old image's graphics. Im still not sure whether this is best, but its the best I can think of so far.

_________________
"Artificial intelligence will never be a match for human stupidity" - "Jamos Kennedynos"

Ah, I see. But how would you implement this in code? What would it look like to the end-user?

OK, thought about it more, maybe this would be better and easier:


Then we could discourage (or deprecate?) offscreen rendering using the Image(int, int) constructors.

It would be implemented with these methods:


Then all you would need are some small changes to FBOGraphics, PBufferGraphics and PBufferUniqueGraphics. e.g.


We could also add a null texture check in GraphicsFactory, to reduce the need for an unnecessary lookup.

Wow, this really good oO I never thought on this problem but now I realize where my texture loads comes from

I have a look in it when coming back from holidays, sorry that I'm not "this" active lately :/

_________________
Current Projects:
Mr. Hat I
Vegan Vs. Zombies
Projects:
RadicalFish Engine - Build on top of Slick2D, Ideas, Bugs? Open an Issue ticket!

I may just be misreading your code, but it seems that this would require every new image to be based off another image, something that may not be easy for the user to work with in many situations. Suppose that instead, we depricated the getGraphics method and reaplaced it with a getEditableGraphics method and the createOffscreenImage funciton you proposed. The createOffscreen image would be able to explicitly create a new image, making it clear that another image would need to be destroyed, while getEditableGraphics could just edit/replace the existing texture, preventing it from creating memory leaks. Perhaps the names of these functions is misleading, but does the concept make sense. I honestly don't know if this strateg is reasonably applicable, but if it is, it would keep backwards compatibility.

_________________
"Artificial intelligence will never be a match for human stupidity" - "Jamos Kennedynos"

The method is static... It doesn't require another image instance.

It simply provides a better way to construct images for FBO/offscreen purposes.

Old style:


Fixed, new style:


You can still use the various Image constructors as before to create new images. And getGraphics will still have the same functionality (in some cases it can be useful; e.g. modifying sub/scaled/flipped copies without affecting the original).

The getEditableGraphics method you're suggesting fixes the memory leak, but doesn't address the actual problem: Slick is generating two textures when it's only necessary to generate one.