WIP: Reducing the number of methods in TextureManager

[MasDennis]

Problem

The TextureManager class currently has no less than 56 methods that do basically the same thing: add textures.
This is a proposal to simplify the API and to make the TextureManager more compact. Your feedback (especially @AndrewJo) is appreciated.

Solution

Leverage the existing TextureInfo class. Most of the parameters that are passed to the addTexture() methods are already part of the TextureInfo class.

A significant change is that TextureInfo will be used as input instead of output.
This way the number of addTexture() methods can be reduced from 56 to 10.

Implementation

TextureInfo class properties:

public class TextureInfo {
    int mTextureId;
    int mUniformHandle;
    int mWidth;
    int mHeight;
    int mInternalFormat;
    int mBitmapFormat;
    boolean mMipmap;
    boolean mShouldRecycle;
    boolean isCubeMap;
    boolean isExistingTexture;
    String mTextureName;
    TextureType mTextureType;
    WrapType mWrapType;
    FilterType mFilterType;
    Config mBitmapConfig;
    CompressionType mCompressionType;
    PaletteFormat mPaletteFormat;
    ThreeDcFormat mThreeDcFormat;
    AtcFormat mAtcFormat;
    Dxt1Format mDxt1Format;
    PvrtcFormat mPvrtcFormat;

    Bitmap mTexture;
    Bitmap[] mTextures;
    ByteBuffer[] mBuffer;
    ...

New TextureManager methods:

public class TextureManager {
    void addTexture(TextureInfo info);
    void addTexture(TextureInfo info, Bitmap texture);
    void addTexture(TextureInfo info, Bitmap[] textures);
    void addTexture(TextureInfo info, int resourceId);
    void addTexture(TextureInfo info, int[] resourceIds);
    void addTexture(TextureInfo info, ByteBuffer buffer);
    void addTexture(TextureInfo info, ByteBuffer[] buffers);
    void addTexture(TextureInfo info, ByteBuffer buffer, Bitmap texture); // manually mipmapped textures
    void addTexture(TextureInfo info, ByteBuffer[] buffers, Bitmap texture); // manually mipmapped textures
    void addTexture(TextureInfo info, InputStream compressedTexture, Bitmap fallbackTexture);
    ...

Usage

// -- simple texture config
mTextureManager.addTexture(new TextureInfo(TextureType.DIFFUSE), myTextureBitmap);

// -- advanced texture config
TextureInfo info = new TextureInfo();
info.setTextureType(TextureType.DIFFUSE);
info.shouldRecycle(true);
info.setWrapType(WrapType.REPEAT);
info.setCompressionType(CompressionType.ETC1);

mTextureManager.addTexture(info, compressedTexture, fallbackTexture);

// -- Cube maps
TextureInfo info = new TextureInfo(TextureType.CUBE_MAP);
info.shouldRecycle(true);

mTextureManager.addTexture(info, textures);

Still in Question

Should Bitmap, Bitmap[] and ByteBuffer still be part of the TextureInfo class? Semantically it is incorrect I think.

These properties are only used when the textures shouldn't be recycled. This way the texture can be uploaded again when the OpenGL context needs to be re-created. This can be flagged by the user by setting info.shouldRecycle() to false.

An alternative solution would be to store these in the TextureManager class.

Should the class be called TextureInfo or should it be renamed to TextureConfig?

[jwoolston]

I would say TextureConfig for two reasons. It forces developers into updating code so we wont get a ton of issues because it silently accepts TextureInfo in places it no longer should. Also, it just makes more sense, as it is an input now, and not really representing the same thing that it did.

As for the data members, I am not sure where they get used in the process. If they need to be reuploaded for a new context, how does info.shouldRecycle() make it so we don't need them as fields?

[MasDennis]

I guess I didn't explain it very well. It has been a long day ... 😴

shouldRecycle() indicates that the textures should be kept in memory for automatic texture management. If set to true the bitmaps won't be stored and the user has to do it manually. If set to false then the bitmaps will be stored.
When the OpenGL context is lost (when resuming or when rotating the screen) the texture bitmaps will be re-uploaded to the graphics hardware.

But I really think it shouldn't be in the TextureConfig class. This is definitely something that the TextureManager class should manage.

[jwoolston]

Ah, yes. I would agree.

[jwoolston]

@MasDennis Thanks for saving me the hassle of figuring out the texture stuff btw...I wasn't looking forward to it 😉

[MasDennis]

Well thank you for your hard work on the scene graph!
I saw that you committed your files. I will look at them tomorrow, I'm about to hit the sack 💤

[AndrewJo]

I think the ability to add custom texture names and override default names like "uDiffuseTexture", "uDiffuseTexture1", etc. would be a nice.

[MasDennis]

This turns out to be one of those things that ends up affecting the framework significantly. It also turns out to be a lot more work than I thought. But it is all for the Greater Good 🍻

Here's what I think should change. Please feed back on this.

• as mentioned in Discussion: Texture Packing and Texture Atlas #620: the addTexture() method should be removed from BaseObject3D. Under the hood this method calls AMaterial.addTexture() so it is redundant. From a logical point of view you should only add textures to materials anyway.
• TextureInfo has become TextureConfig but I think we should simply name it Texture. Because it is not only on object that stores the configuration but also the Bitmap and/or ByteBuffer.
• I was wondering if the Texture object should have convenience methods for setting the Bitmap. To allow for things like Damn simple texture setting (from resource ID) #228.
• I was also wondering if TextureManager should be a singleton. Opinions about singletons are divided but I think TextureManager makes a good case. There should only be one instance. At the moment it is possible to create a multiple instances which gives room for user error.
  Making it a singleton will also allow us to clean up the API and make it easier for the user. I'm thinking:

Texture texture = new Texture(TextureType.DIFFUSE);
texture.setBitmap(R.drawable.my_texture);
DiffuseMaterial material = new DiffuseMaterial();
material.addTexture(texture);

In this case TextureManager.addTexture() is called by AMaterial.addTexture(). By using a singleton the user doesn't have to pass TextureManager into the AMaterial constructor every time they create a material.

• This brings us to this problem where the texture coordinates should be updated after a texture atlas has been created. The original idea was to update the texture coordinates by issuing a call to BaseObject3D.addTexture() (the texture coordinates are stored in Geometry3D which is a member of BaseObject3D). I was thinking that BaseObject3D could find out if the texture coordinates should be updated by checking AMaterial.shouldUpdateTextureCoordinates() or something similar. This should happen just before rendering. Then BaseObject3D can retrieve the the Tile and pass it on to Geometry3D.

[ToxicBakery]

I don't think there is anything wrong with Singletons. It is the misuse of of them that upsets people. In this case I disagree though. The problem I see is having multiple GLES contexts such as having a wallpaper set and having a preview. This leads to a higher chance of unknowingly eating up all the application memory loading the textures in multiple times as the user switches back and forth. Next I will throw it out there that no caffeine has been applied this morning so I could very well be way off base here.

For setting a texture atlas, I believe the simple solution is something like this. Essentially TextureInfo would store the bounding box of each texture inside the atlas and users would only need to know the ID of the texture in the atlas to apply it to an object.

TextureInfo textureAtlasInfo = mTextureManage.addTexture(R.drawable.my_texuture_atlas);
int atlasPositionID = textureAtlasInfo.defineAtlasPosition(x, y, w, h);

SimpleMaterial material = new SimpleMaterial();
material.addTexture(textureAtlasInfo);

Plane plane = new Plane(1, 1, 1, 1);
plane.setAtlas(textureAtlasInfo, atlasPositionID);

I think a setup like that should allow for manipulation of the geometry. It should also be forward thinking by still giving objects a chance to manipulate data based on the TextureInfo. And finally, it does not require massive changes, or so I think.

[AndrewJo]

I'm all for singleton methods for the texture manager. I see nearly double memory usage and LogCat reports preview instance separately loading bitmaps into memory when the wallpaper is set and the preview is running at the same time. Using a singleton pattern should greatly help reduce resource usage.

I support the name change from TextureInfo/TextureConfig to Texture as well. Being able to have convenience methods to change bitmap for the Texture should allow users to easily update texture bitmaps. I'm thinking that for compressed textures, we can have CompressedTexture class inheriting from Texture class and adding ByteBuffer field which holds the raw compressed texture data. Various texture compression methods can be implemented by further inheriting from CompressedTexture class such as EtcTexture (ETC1/2), ImgTecTexture (PowerVR), DirectXTexture (DXT), etc. should further make the API more elegant.

[MasDennis]

@ToxicBakery Yes, that setAtlas() method makes sense and is much more clean. This way we give the user control over it. Is this solution convenient for your @Davhed?

@AndrewJo Thanks for your input. Inheritance makes perfect sense here.

As @AndrewJo said, the singleton could solve some memory issues. @ToxicBakery could you inject a double espresso and have another think about it?

Cheers for your input guys. Much appreciated.

[ToxicBakery]

Alright, if TextureManager is a singleton, how do we handle the loading the of textures in a way that prevents the same texture being loaded twice. It seems textures would need to be given names or something similar so that the texture manager would be able to identify when a texture already exists otherwise it will just keep accepting duplicate textures each time a preview is shown.

[MasDennis]

For bitmap resources we could use Resources. getResourceName(int resid) to get its unique name.
Not sure how that would work for compressed textures / byte arrays though.

[AndrewJo]

I think we should also allow custom texture names if the user decides to create a custom material that uses the textures instead of canned uDiffuseTexture as well. The texture name field could be added perhaps?

[jwoolston]

Now that you all have had a party while I was sleeping, I'll give my two cents....

• I also would like to see a singleton for the same reason as @AndrewJo. We have Preview and The real "Set Livewallpaper" service 2 instances #527 outstanding and I'm not sure there is any other solution to it. As @ToxicBakery points out, we will need to do something about duplicates...ordinarily I am a fan of "if the developer does something stupid, let him pay for it", however in this case Ian gives an example which would not at all be their fault, nor would they have a good way of knowing it was happening. For this I think every texture should be named. At this point it becomes up to the developer to make sure they have unique names for everything. Atlas tiles can be exempt from this provided two tiles in the same atlas do not have the same name but that is obvious 😉 I think the solution here then is to have every constructor of Texture take a name parameter, an offer lazy loading of the bitmaps until it determines the name is safe.
• This also means I support naming it Texture
• As for convenience methods, I have some concern here. Part of this rewrite is to facilitate texture addition/removal/changes being thread safe and fitting in with the task queue. I am concerned that convenience methods would allow for this to be violated by changing a bitmap in between frames. The queue can replace entire Texture objects, perhaps that is the better way to go.
• I like the inheritance suggestion from @AndrewJo. Queue @ToxicBakery for an encapsulation crusade MEME....

-Perhaps it has to do with the mutual lack of caffeine, but I am not sure I fully understand Ian's suggestion for handling the atlas. I think it is basically add the atlas to TextureManager as a special case of Texture. Then ask the atlas for position info and pass both the Texture holding the atlas and the position info? If this is the case, and integrating what I understand about @Davhed existing code, something like this makes sense:

Texture atlas = mTextureManager.packTexturesFromAssets("AwesomeAtlas", 1024, 1024, 0, false, "atlas");

SimpleMaterial material = new SimpleMaterial();
material.addTexture(atlas);

Plane plane = new Plane(1, 1, 1, 1);
plane.setAtlas(atlas, tile);

Some details here are a matter of opinion but basically setAtlas() has a signature of void setAtlas(TextureAtlas atlas, String tileName). Maybe it makes sense to set setAtlasTile() or something like that instead. The internal flow then would be to fetch the specified tile from the atlas, examine its info and adjust the uv accordingly, taking care to preserve data as I mention here.

• Finally:

I think we should also allow custom texture names if the user decides to create a custom material that uses the textures instead of canned uDiffuseTexture as well. The texture name field could be added perhaps?

@AndrewJo I'm guessing this stems from actual experience? I am not certain what the use case would be here.

[Davhed]

I do this this will work, and as @jwoolston points out the terminology needs some tweaking. This is how I have named thins so far.

• Atlas - A collection of pages and tile info
• Atlas Page - an individual atlas bitmap
• Atlas Tile - a set of coordinates, a target atlas page, and an original "pre-packed" file name

It seems like the overall theory is sound. Passing an atlas and tile name into setAtlasTile(String tileName) should just convert the coordinates of the tile to UVs for the BaseObject3D.

However, addTexture(TextureAtlas atlas) won't really work. I think the user will have to pass the tile name too since there could be a collection of atlas pages. Maybe addTexture(TextureAtlas atlas, String tileName) would?

[Davhed]

actually better yet, addTexture(String tileName) and then let AMaterial find the right atlas page?

[jwoolston]

actually better yet, addTexture(String tileName) and then let AMaterial find the right atlas page?

As long as you dont have two tiles of the same name in different atlases this would work fine.

[Davhed]

The tile name is set from the resourceID, so I believe you can't have duplicate names anyhow

[jwoolston]

Never thought of that. There ya go, problem solved. Yes, thats nice and simple

[MasDennis]

Thanks for your input guys. Very helpful.
I hope to make good progress on this the coming days. I will probably go into stealth mode to finally get this thing nailed.

[Davhed]

@MasDennis I will put a hold on the remaining tasks for the atlas until you are prepared to collaborate. I think we will need a little strategy to bring our two worlds together.

I start a new job at the end of the month, so I will have significantly more time in the next two weeks to accomplish this than I will later on.

[MasDennis]

Oh, by the way @jwoolston

I am concerned that convenience methods would allow for this to be violated by changing a bitmap in between frames.

These convenience methods are only used to store bitmaps and decode bitmaps. The OpenGL specific uploads will be thread safe and handled by TextureManager.

@Davhed Congratulations! That's great news. 🍻
Yes, lets discuss merging the atlas into the manager when I'm done.

[jwoolston]

Sounds good.

[MasDennis]

So. A lot of work. But it is getting there.

• meet skinny TextureManager. Completely @jwoolston proof!
• look at the new Texture classes
• see RajawaliBasicExample Anchor Steam style
• see Cube Maps Anchor Stream style
• check out Anchor Steamed Sphere Maps
• just one more then. Steamy Bump Maps!

I will be working on compressed textures next.

But the can is open. Worms are crawling up my trousers.
Materials aren't thread safe yet. I'm thinking about @jwoolston -izing them as well. It will probably be very similar to TextureManager. MaterialManager much?

[jwoolston]

Wow, TextureManager is so much more... managable 😆

Very awesome @MasDennis.

I'm thinking about @jwoolston -izing them as well.

So is @jwoolston-izing going to be code for thread safe now? Have I broken the library that much 😉 I suppose @ToxicBakery pointed out the crusade/blitzkrieg I have brought upon threads. Yes, they should be thread safe, especially with the deferred lighting prospect....I see things going all kinds of crazy on us there.

[MasDennis]

🤘 @jwoolston -ization is indeed code for the thread safety crusade.
But it was needed and it is awesome.

[AndrewJo]

Awesome work Dennis! 🍻

[jwoolston]

@jwoolston -ization is indeed code for the thread safety crusade.

Well now I can die happy, my imprint on the world has been made 🚶

@AndrewJo we have a cat who does that.

[MasDennis]

Well now I can die happy

Not yet, finish the SceneGraph first please.

we have a cat who does that

ANIMATED GIF PLEASE!!!!

[Davhed]

@MasDennis I am madly in love with the simplicity of this:

material.addTexture(new Texture(R.drawable.earthtruecolor_nasa_big));

[Davhed]

Also, since you've relegated Bump Maps to their own class do you plan to do the same for Alpha and Specular?

To be nitpicky... shouldn't they be called Normal Maps (for RGB) and Height Maps (for B&W)?

[jwoolston]

ANIMATED GIF PLEASE!!!!

I'll have to see if I can get him to cooperate.

[MasDennis]

I am madly in love with the simplicity of this

It is a lot cleaner isn't it?

Also, since you've relegated Bump Maps to their own class do you plan to do the same for Alpha and Specular?

I just did 😄

To be nitpicky... shouldn't they be called Normal Maps (for RGB) and Height Maps (for B&W)?

Mmmm good call. Now is our chance to rename stuff so I'll rename BumpMap to NormalMap. Makes sense.

[Davhed]

👍

[MasDennis]

Ok, so the overhaul seems to be done. There's no documentation yet but I'm going to work on something else for a moment. Need a break!
It is ready to be tested. I've updated the examples app as well so please go ahead and test the mofo.

• https://github.com/MasDennis/Rajawali/tree/texture-manager-rewrite
• https://github.com/MasDennis/RajawaliExamples/tree/texture-manager-rewrite

[jwoolston]

🍻 are in order.

[MasDennis]

Loads of them. Kegs.

[jwoolston]

Loads of them. Kegs.

If i had a resaonably way of getting my meade across the ocean to you, id send you some.

[jwoolston]

@MasDennis I don't seem to be able to get those branches to compile

[MasDennis]

What errors do you get?

[Davhed]

They're working fine for me 💃

[Davhed]

I lied, I'm getting "The method Texture(String, Bitmap) is undefined for the type WallpaperRenderer", but when I review the Texture class I indeed see:

    public Texture(String textureName, Bitmap bitmap)
    {
        super(TextureType.DIFFUSE, textureName, bitmap);
    }

And the imports appear correct...

import rajawali.materials.textures.Texture;

[MasDennis]

Strange. Did you do a clean before a rebuild all?

[MasDennis]

This actually reminded me to update the wallpaper template as well:

https://github.com/MasDennis/RajawaliWallpaperTemplate/tree/texture-manager-rewrite

Please let me know if you still have problems.

[Davhed]

It's still not working... I'm going to try and diagnose and treat this evening.

[MasDennis]

Thanks @Davhed

There are still some issues with the live wallpaper template (run time, not compile time). I'll investigate either this evening or tomorrow.

[MasDennis]

I just pushed another update. If nobody objects then I'll push it to master tomorrow.
But some of this first: 😴

[jwoolston]

I took a look through it, I see no issues to prevent it from being in master.

[MasDennis]

I've just pushed it to master. Please go ahead and test it.
I've also added a CHANGELOG.md file to track all the changes. This will make it easier for users to migrate to Anchor Steam.
💥💥💥 Finally closing this issue now 💥💥💥

[AndrewJo]

👍 You sir are a gentleman and a scholar. 🍻